[clangd] Fix invalid UTF8 when extracting doc comments.

[kadircet]

thanks, LGTM!

Should we also have another test for SymbolCollector, to ensure we don't regress this somehow in the future?

clang-tools-extra/clangd/CodeCompletionStrings.cpp

93

it is always surprising to have these helpers in json library :D (just talking out loud)

[sammccall]

In D88567#2303332, @kadircet wrote:

thanks, LGTM!

Should we also have another test for SymbolCollector, to ensure we don't regress this somehow in the future?

We had a SymbolCollector test for the boost case so I modified it to add a doc comment.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

93

Yeah. They're just wrappers around functions from ConvertUTF.h.
Do you want a patch to move them there?

[kadircet]

clang-tools-extra/clangd/CodeCompletionStrings.cpp

93

nah, let's wait for the next time we use those :D

clang-tools-extra/clangd/CodeCompletionStrings.cpp

 //===--- CodeCompletionStrings.cpp -------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/RawCommentList.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "llvm/Support/JSON.h"
 #include <limits>
 #include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 bool isInformativeQualifierChunk(CodeCompletionString::Chunk const &Chunk) {
std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
   const RawComment *RC = getCompletionComment(Ctx, &Decl);
   if (!RC)
     return "";
   // Sanity check that the comment does not come from the PCH. We choose to not
   // write them into PCH, because they are racy and slow to load.
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc =
       RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
-  return looksLikeDocComment(Doc) ? Doc : "";
+  if (!looksLikeDocComment(Doc))
+    return "";
+  // Clang requires source to be UTF-8, but doesn't enforce this in comments.
+  if (!llvm::json::isUTF8(Doc))
+    Doc = llvm::json::fixUTF8(Doc);
+  return Doc;
 }
 
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
                   std::string *Snippet, std::string *RequiredQualifiers,
                   bool CompletingPattern) {
   // Placeholder with this index will be ${0:…} to mark final cursor position.
   // Usually we do not add $0, so the cursor is placed at end of completed text.
   unsigned CursorSnippetArg = std::numeric_limits<unsigned>::max();

clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp

 //===-- CodeCompletionStringsTests.cpp --------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
+#include "TestTU.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 

 
 TEST_F(CompletionStringTest, DocumentationWithAnnotation) {
   Builder.addBriefComment("This is ignored");
   Builder.AddAnnotation("Ano");
   EXPECT_EQ(formatDocumentation(*Builder.TakeString(), "Is this brief?"),
             "Annotation: Ano\n\nIs this brief?");
 }
 
+TEST_F(CompletionStringTest, GetDeclCommentBadUTF8) {
+  // <ff> is not a valid byte here, should be replaced by encoded <U+FFFD>.
+  auto TU = TestTU::withCode("/*x\xffy*/ struct X;");
+  auto AST = TU.build();
+  EXPECT_EQ("x\xef\xbf\xbdy",
+            getDeclComment(AST.getASTContext(), findDecl(AST, "X")));
+}
+
 TEST_F(CompletionStringTest, MultipleAnnotations) {
   Builder.AddAnnotation("Ano1");
   Builder.AddAnnotation("Ano2");
   Builder.AddAnnotation("Ano3");
 
   EXPECT_EQ(formatDocumentation(*Builder.TakeString(), ""),
             "Annotations: Ano1 Ano2 Ano3\n");
 }

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

TEST_F(SymbolCollectorTest, InvalidSourceLoc) {
   runSymbolCollector(Header, /**/ "");
   EXPECT_THAT(Symbols, Contains(QName("operator delete")));
 }
 
 TEST_F(SymbolCollectorTest, BadUTF8) {
   // Extracted from boost/spirit/home/support/char_encoding/iso8859_1.hpp
   // This looks like UTF-8 and fools clang, but has high-ISO-8859-1 comments.
   const char *Header = "int PUNCT = 0;\n"
-                       "int types[] = { /* \xa1 */PUNCT };";
+                       "/* \xa1 */ int types[] = { /* \xa1 */PUNCT };";
   CollectorOpts.RefFilter = RefKind::All;
   CollectorOpts.RefsInHeaders = true;
   runSymbolCollector(Header, "");
-  EXPECT_THAT(Symbols, Contains(QName("types")));
+  EXPECT_THAT(Symbols, Contains(AllOf(QName("types"), Doc("\xef\xbf\xbd "))));
   EXPECT_THAT(Symbols, Contains(QName("PUNCT")));
   // Reference is stored, although offset within line is not reliable.
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "PUNCT").ID, _)));
 }
 
 TEST_F(SymbolCollectorTest, MacrosInHeaders) {
   CollectorOpts.CollectMacro = true;
   TestFileName = testPath("test.h");