[clangd] Serve comments for headers decls from dynamic index only

[ilya-biryukov]

Summary

To fix a crash in code completion that occurrs when reading doc
comments from files that were updated after the preamble was
computed. In that case, the files on disk could've been changed and we
can't rely on finding the comment text with the same range anymore.

The current workaround is to not provide comments from the headers at
all and rely on the dynamic index instead.

A more principled solution would be to store contents of the files
read inside the preamble, but it is way harder to implement properly,
given that it would definitely increase the sizes of the preamble.

Together with D47272, this should all preamble-related crashes we're
aware of.

[sammccall]

clangd/CodeCompletionStrings.h

46 ↗

(On Diff #148266)

this invites double negation.
Also it doesn't seem like a great default, violates principle of least surprise.

prefer bool CommentsFromHeaders = true, or with no default.
(or even consider an enum)

[ilya-biryukov]

• Invert flag, remove the default.

clangd/CodeCompletionStrings.h

46 ↗

(On Diff #148266)

Thanks for spotting this!
I opted for a positive bool flag with no defaults.

clang-tools-extra/trunk/clangd/CodeComplete.cpp

void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg,
     assert(CurrentArg <= (unsigned)std::numeric_limits<int>::max() &&
            "too many arguments");
     SigHelp.activeParameter = static_cast<int>(CurrentArg);
     for (unsigned I = 0; I < NumCandidates; ++I) {
       const auto &Candidate = Candidates[I];
       const auto *CCS = Candidate.CreateSignatureString(
           CurrentArg, S, *Allocator, CCTUInfo, true);
       assert(CCS && "Expected the CodeCompletionString to be non-null");
+      // FIXME: for headers, we need to get a comment from the index.
       SigHelp.signatures.push_back(ProcessOverloadCandidate(
           Candidate, *CCS,
-          getParameterDocComment(S.getASTContext(), Candidate, CurrentArg)));
+          getParameterDocComment(S.getASTContext(), Candidate, CurrentArg,
+                                 /*CommentsFromHeader=*/false)));
     }
   }
 
   GlobalCodeCompletionAllocator &getAllocator() override { return *Allocator; }
 
   CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
 
 private:
private:
   CompletionItem toCompletionItem(const CompletionCandidate &Candidate,
                                   const CompletionItemScores &Scores) {
     CodeCompletionString *SemaCCS = nullptr;
     std::string DocComment;
     if (auto *SR = Candidate.SemaResult) {
       SemaCCS = Recorder->codeCompletionString(*SR);
       if (Opts.IncludeComments) {
         assert(Recorder->CCSema);
-        DocComment = getDocComment(Recorder->CCSema->getASTContext(), *SR);
+        DocComment = getDocComment(Recorder->CCSema->getASTContext(), *SR,
+                                   /*CommentsFromHeader=*/false);
       }
     }
     return Candidate.build(FileName, Scores, Opts, SemaCCS, Includes.get(), DocComment);
   }
 };
 
 CompletionList codeComplete(PathRef FileName,
                             const tooling::CompileCommand &Command,

clang-tools-extra/trunk/clangd/CodeCompletionStrings.h

 class ASTContext;
 
 namespace clangd {
 
 /// Gets a minimally formatted documentation comment of \p Result, with comment
 /// markers stripped. See clang::RawComment::getFormattedText() for the detailed
 /// explanation of how the comment text is transformed.
 /// Returns empty string when no comment is available.
+/// If \p CommentsFromHeaders parameter is set, only comments from the main
+/// file will be returned. It is used to workaround crashes when parsing
+/// comments in the stale headers, coming from completion preamble.
 std::string getDocComment(const ASTContext &Ctx,
-                          const CodeCompletionResult &Result);
+                          const CodeCompletionResult &Result,
+                          bool CommentsFromHeaders);
 
 /// Gets a minimally formatted documentation for parameter of \p Result,
 /// corresponding to argument number \p ArgIndex.
 /// This currently looks for comments attached to the parameter itself, and
 /// doesn't extract them from function documentation.
 /// Returns empty string when no comment is available.
+/// If \p CommentsFromHeaders parameter is set, only comments from the main
+/// file will be returned. It is used to workaround crashes when parsing
+/// comments in the stale headers, coming from completion preamble.
 std::string
 getParameterDocComment(const ASTContext &Ctx,
                        const CodeCompleteConsumer::OverloadCandidate &Result,
-                       unsigned ArgIndex);
+                       unsigned ArgIndex, bool CommentsFromHeaders);
 
 /// Gets label and insert text for a completion item. For example, for function
 /// `Foo`, this returns <"Foo(int x, int y)", "Foo"> without snippts enabled.
 ///
 /// If \p EnableSnippets is true, this will try to use snippet for the insert
 /// text. Otherwise, the insert text will always be plain text.
 void getLabelAndInsertText(const CodeCompletionString &CCS, std::string *Label,
                            std::string *InsertText, bool EnableSnippets);

clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp

 //===--- CodeCompletionStrings.cpp -------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RawCommentList.h"
+#include "clang/Basic/SourceManager.h"
 #include <utility>
 
 namespace clang {
 namespace clangd {
 
 namespace {
 
 bool isInformativeQualifierChunk(CodeCompletionString::Chunk const &Chunk) {
for (const auto &Chunk : CCS) {
     case CodeCompletionString::CK_VerticalSpace:
       InsertText += Chunk.Text;
       // Don't even add a space to the label.
       break;
     }
   }
 }
 
+std::string getFormattedComment(const ASTContext &Ctx, const RawComment &RC,
+                                bool CommentsFromHeaders) {
+  auto &SourceMgr = Ctx.getSourceManager();
+  // Parsing comments from invalid preamble can lead to crashes. So we only
+  // return comments from the main file when doing code completion. For
+  // indexing, we still read all the comments.
+  // FIXME: find a better fix, e.g. store file contents in the preamble or get
+  // doc comments from the index.
+  if (!CommentsFromHeaders && !SourceMgr.isWrittenInMainFile(RC.getLocStart()))
+    return "";
+  return RC.getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+}
 } // namespace
 
 std::string getDocComment(const ASTContext &Ctx,
-                          const CodeCompletionResult &Result) {
+                          const CodeCompletionResult &Result,
+                          bool CommentsFromHeaders) {
   // FIXME: clang's completion also returns documentation for RK_Pattern if they
   // contain a pattern for ObjC properties. Unfortunately, there is no API to
   // get this declaration, so we don't show documentation in that case.
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
     return "";
   auto Decl = Result.getDeclaration();
   if (!Decl)
     return "";
   const RawComment *RC = getCompletionComment(Ctx, Decl);
   if (!RC)
     return "";
-  return RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+  return getFormattedComment(Ctx, *RC, CommentsFromHeaders);
 }
 
 std::string
 getParameterDocComment(const ASTContext &Ctx,
                        const CodeCompleteConsumer::OverloadCandidate &Result,
-                       unsigned ArgIndex) {
+                       unsigned ArgIndex, bool CommentsFromHeaders) {
   auto Func = Result.getFunction();
   if (!Func)
     return "";
   const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
   if (!RC)
     return "";
-  return RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+  return getFormattedComment(Ctx, *RC, CommentsFromHeaders);
 }
 
 void getLabelAndInsertText(const CodeCompletionString &CCS, std::string *Label,
                            std::string *InsertText, bool EnableSnippets) {
   return EnableSnippets ? processSnippetChunks(CCS, Label, InsertText)
                         : processPlainTextChunks(CCS, Label, InsertText);
 }
 

clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
   std::string IgnoredLabel;
   std::string PlainInsertText;
   getLabelAndInsertText(*CCS, &Label, &SnippetInsertText,
                         /*EnableSnippets=*/true);
   getLabelAndInsertText(*CCS, &IgnoredLabel, &PlainInsertText,
                         /*EnableSnippets=*/false);
   std::string FilterText = getFilterText(*CCS);
   std::string Documentation =
-      formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion));
+      formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
+                                              /*CommentsFromHeaders=*/true));
   std::string CompletionDetail = getDetail(*CCS);
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
     // Use the expansion location to get the #include header since this is
     // where the symbol is exposed.
     if (auto Header = getIncludeHeader(
             QName, SM, SM.getExpansionLoc(ND.getLocation()), Opts))

clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

 #include "CodeComplete.h"
 #include "Compiler.h"
 #include "Matchers.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "index/MemIndex.h"
+#include "llvm/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
 namespace {
 using namespace llvm;
auto Completions = completions(
       )cpp",
         {func("::SomeNameInTheIndex")});
 
     EXPECT_THAT(Completions.items,
                 Not(Contains(Labeled("SomeNameInTheIndex"))));
   }
 }
 
+TEST(CompletionTest, DocumentationFromChangedFileCrash) {
+  MockFSProvider FS;
+  auto FooH = testPath("foo.h");
+  auto FooCpp = testPath("foo.cpp");
+  FS.Files[FooH] = R"cpp(
+    // this is my documentation comment.
+    int func();
+  )cpp";
+  FS.Files[FooCpp] = "";
+
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  Annotations Source(R"cpp(
+    #include "foo.h"
+    int func() {
+      // This makes sure we have func from header in the AST.
+    }
+    int a = fun^
+  )cpp");
+  Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+  // We need to wait for preamble to build.
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  // Change the header file. Completion will reuse the old preamble!
+  FS.Files[FooH] = R"cpp(
+    int func();
+  )cpp";
+
+  clangd::CodeCompleteOptions Opts;
+  Opts.IncludeComments = true;
+  CompletionList Completions =
+      cantFail(runCodeComplete(Server, FooCpp, Source.point(), Opts));
+  // We shouldn't crash. Unfortunately, current workaround is to not produce
+  // comments for symbols from headers.
+  EXPECT_THAT(Completions.items,
+              Contains(AllOf(Not(IsDocumented()), Named("func"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang