[clangd] Handle multiple callbacks from Sema's completion

[ilya-biryukov]

Summary

When parser backtracks, we might receive multiple code completion
callbacks.
Previously we had a failing assertion there, now we take first results
and hope they are good enough.

[ilya-biryukov]

As a follow-up to an offline discussion: I opted for not excluding results from 'Recovery' contexts, because they are actually somewhat semantically relevant (i.e. contain only local variables from the current function). One example where this is useful are skipped PP branches (see CompleteInExcludedPPBranch test).

[sammccall]

clangd/CodeComplete.cpp

454 ↗

(On Diff #138702)

Suggest some hint in the log that helps readers who don't know the code structure. (It's easy to find the code structure by searching.

E.g. "Multiple code complete callbacks (parser backtracked?). Dropping results from context {0}, keeping results from {1}."

unittests/clangd/CodeCompleteTests.cpp

623 ↗

(On Diff #138702)

dead code? delete it or add an assertion

[ilya-biryukov]

• Address review comments

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

struct CompletionRecorder : public CodeCompleteConsumer {
   std::vector<CodeCompletionResult> Results;
   CodeCompletionContext CCContext;
   Sema *CCSema = nullptr; // Sema that created the results.
   // FIXME: Sema is scary. Can we store ASTContext and Preprocessor, instead?
 
   void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
                                   CodeCompletionResult *InResults,
                                   unsigned NumResults) override final {
+    if (CCSema) {
+      log(llvm::formatv(
+          "Multiple code complete callbacks (parser backtracked?). "
+          "Dropping results from context {0}, keeping results from {1}.",
+          getCompletionKindString(this->CCContext.getKind()),
+          getCompletionKindString(Context.getKind())));
+      return;
+    }
     // Record the completion context.
-    assert(!CCSema && "ProcessCodeCompleteResults called multiple times!");
     CCSema = &S;
     CCContext = Context;
 
     // Retain the results we might want.
     for (unsigned I = 0; I < NumResults; ++I) {
       auto &Result = InResults[I];
       // Drop hidden items which cannot be found by lookup after completion.
       // Exception: some items can be named by using a qualifier.

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

void f() {
       clan^
     }
   )cpp");
 
   EXPECT_THAT(Results.items, Contains(Labeled("clang")));
   EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::"))));
 }
 
+TEST(CompletionTest, BacktrackCrashes) {
+  // Sema calls code completion callbacks twice in these cases.
+  auto Results = completions(R"cpp(
+      namespace ns {
+      struct FooBarBaz {};
+      } // namespace ns
+
+     int foo(ns::FooBar^
+  )cpp");
+
+  EXPECT_THAT(Results.items, ElementsAre(Labeled("FooBarBaz")));
+
+  // Check we don't crash in that case too.
+  completions(R"cpp(
+    struct FooBarBaz {};
+    void test() {
+      if (FooBarBaz * x^) {}
+    }
+)cpp");
+}
+
+TEST(CompletionTest, CompleteInExcludedPPBranch) {
+  auto Results = completions(R"cpp(
+    int bar(int param_in_bar) {
+    }
+
+    int foo(int param_in_foo) {
+#if 0
+  par^
+#endif
+    }
+)cpp");
+
+  EXPECT_THAT(Results.items, Contains(Labeled("param_in_foo")));
+  EXPECT_THAT(Results.items, Not(Contains(Labeled("param_in_bar"))));
+}
+
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());