[clangd] Parse all comments in Sema and completion.

[ilya-biryukov]

Summary

And add tests for the comment extraction code.

clangd will now show non-doxygen comments in completion for results
coming from Sema and Dynamic index.
Static index does not include the comments yet, I will enable it in
a separate commit after investigating which implications it has for
the size of the index.

[sammccall]

Code looks good/simple, just a couple of design questions

clangd/ClangdUnit.cpp

320

Any idea about whether this will affect performance significantly?
Less for this patch, and more for whether this should be an option in the future that we might e.g. only do during indexing.

clangd/CodeComplete.cpp

652

Are we sure we want to do this in code complete? I would have thought the more natural approach would be to implement resolve in terms of lookup in the index, and only provide it there.
The lack of good support for resolve in YCM LSP shouldn't be a problem as YCM doesn't actually use doc comments (I think?).

[ilya-biryukov]

clangd/ClangdUnit.cpp

320

Hopefully, there should be no significant difference, but I'll do some benchmarks with both real code and artificial comment-heave code to make sure that's the case.
It would certainly eat a bit more memory, but it shouldn't be significant as we only store an extra list of sourceranges for comments.
Even for this patch, maybe we would want to only enable it in the AST but not in the preamble. I'll get some numbers and come back to this.

clangd/CodeComplete.cpp

652

It seems to give a better user-experience:

• Some items come only from Sema, so getting documentation for local variables and class members is currently only possible through Sema. We could probably omit the docs for the local vars and put class members into the index, though.
• If the comment for the completion is in the current file, we should prefer the latest version, not one from the index that might be stale.

However, it does mean sending docs along with the results, I don't think LSP will allow us to properly handle the lifetime of docs from the completion AST, so we won't be able to delay their result until resolve time.

[sammccall]

LG (once the dependencies are done!)

clangd/ClangdUnit.cpp

320

No need to block on the numbers from my point of view, though I would be interested.

maybe we would want to only enable it in the AST but not in the preamble

You're talking about gathering the information, right?
How would this work? Given that e.g. class members come from the preamble and we need the comments from there.

clangd/CodeComplete.cpp

652

Yeah, Sema-only results. I wasn't thinking.

We can send eager docs from sema results and still fill ind index docs at resolve time. This all sounds good.

[ilya-biryukov]

• Moved tests to clang

[ilya-biryukov]

• Added tests that all comments are being parsed.

clangd/ClangdUnit.cpp

 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace clang::clangd;
 using namespace clang;
std::unique_ptr<CompilerInvocation> CI;
     if (!CI) {
       log("Could not build CompilerInvocation for file " + FileName);
       AST = llvm::None;
       Preamble = nullptr;
       return llvm::None;
     }
     // createInvocationFromCommandLine sets DisableFree.
     CI->getFrontendOpts().DisableFree = false;
+    CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   }
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, FileName);
 
   // Compute updated Preamble.
   std::shared_ptr<const PreambleData> NewPreamble =
       rebuildPreamble(*CI, Inputs.CompileCommand, Inputs.FS, *ContentsBuffer);

clangd/CodeComplete.cpp

 #include "FuzzyMatch.h"
 #include "Headers.h"
 #include "Logger.h"
 #include "Quality.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "URI.h"
 #include "index/Index.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/Support/Format.h"
auto CI = createInvocationFromCommandLine(
       CompilerInstance::createDiagnostics(new DiagnosticOptions,
                                           &DummyDiagsConsumer, false),
       Input.VFS);
   if (!CI) {
     log("Couldn't create CompilerInvocation");;
     return false;
   }
   CI->getFrontendOpts().DisableFree = false;
+  CI->getLangOpts()->CommentOpts.ParseAllComments = true;
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Input.Contents, Input.FileName);
 
   // We reuse the preamble whether it's valid or not. This is a
   // correctness/performance tradeoff: building without a preamble is slow, and
   // completion is latency-sensitive.
   if (Input.Preamble) {

unittests/clangd/CodeCompleteTests.cpp

TEST(CompletionTest, DynamicIndexMultiFile) {
   EXPECT_THAT(Results.items, Has("XYZ", CompletionItemKind::Class));
   EXPECT_THAT(Results.items, Has("foo", CompletionItemKind::Function));
   EXPECT_THAT(Results.items, Has("XXX", CompletionItemKind::Class));
   EXPECT_THAT(Results.items, Contains(AllOf(Named("fooooo"), Filter("fooooo"),
                                             Kind(CompletionItemKind::Function),
                                             Doc("Doooc"), Detail("void"))));
 }
 
+TEST(CompletionTest, Documentation) {
+  auto Results = completions(
+      R"cpp(
+      // Non-doxygen comment.
+      int foo();
+      /// Doxygen comment.
+      /// \param int a
+      int bar(int a);
+      /* Multi-line
+         block comment
+      */
+      int baz();
+
+      int x = ^
+     )cpp");
+  EXPECT_THAT(Results.items,
+              Contains(AllOf(Named("foo"), Doc("Non-doxygen comment."))));
+  EXPECT_THAT(
+      Results.items,
+      Contains(AllOf(Named("bar"), Doc("Doxygen comment.\n\\param int a"))));
+  EXPECT_THAT(Results.items,
+              Contains(AllOf(Named("baz"), Doc("Multi-line\nblock comment"))));
+}
+
 TEST(CodeCompleteTest, DisableTypoCorrection) {
   auto Results = completions(R"cpp(
      namespace clang { int v; }
      void f() { clangd::^
   )cpp");
   EXPECT_TRUE(Results.items.empty());
 }