[clangd] Ignore sema code complete callback with recovery context.

[ioeric]

Summary

Sema code complete in the recovery mode is generally useless. For many
cases, sema first completes in recovery context and then recovers to more useful
context, in which it's favorable to ignore results from recovery (as results are
often bad e.g. all builtin symbols and top-level symbols). There is also case
where only sema would fail to recover e.g. completions in excluded #if block.
Sema would try to give results, but the results are often useless (see the updated
excluded #if block test).

[sammccall]

I like this idea, of course hard to know how it will affect all practical cases.

Is it easy to get have internal stats on how many wins/losses this has?

clangd/CodeComplete.cpp

596

might

596

This might also be a case where we can trial identifier-based completion. (Eventually I think we'd want it in other contexts too)

[ioeric]

• Addressed review comments.

[ilya-biryukov]

Thanks! That's a simple fix that makes things better overall!
LGTM with a NIT about the comment in the test.

unittests/clangd/CodeCompleteTests.cpp

797

NIT.

I would argue the results were actually useful here and we introduced a regression.
This patches fixes the more important cases, e.g. if (int x = id^), so it's actually improving things overall. However, I'm not sure about the comment that says clangd is doing the right thing here (it actually gives a worse UX). WDYT?

[ioeric]

In D49175#1158562, @sammccall wrote:

I like this idea, of course hard to know how it will affect all practical cases.

Is it easy to get have internal stats on how many wins/losses this has?

This would fix bad completion results when sema could recover to the normal mode (e.g. completion in macros like EXPECT_THAT, expressions in if statements). From the code completion metrics I collected from existing code, the number of completions where no result was found dropped by 30%. The loss seems to be we will get no result in recovery mode rather than bad results (e.g. in excluded preprocessor branches).

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 {
+    // Results from recovery mode are generally useless, and the callback after
+    // recovery (if any) is usually more interesting. To make sure we handle the
+    // future callback from sema, we just ignore all callbacks in recovery mode,
+    // as taking only results from recovery mode results in poor completion
+    // results.
+    // FIXME: in case there is no future sema completion callback after the
+    // recovery mode, we migth still want to provide some results.
+    if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {
+      log("Code complete: Ignoring sema code complete callback with Recovery "
+          "context.");
+      return;
+    }
     // If a callback is called without any sema result and the context does not
     // support index-based completion, we simply skip it to give way to
     // potential future callbacks with results.
     if (NumResults == 0 && !contextAllowsIndex(Context.getKind()))
       return;
     if (CCSema) {
       log(llvm::formatv(
           "Multiple code complete callbacks (parser backtracked?). "

unittests/clangd/CodeCompleteTests.cpp

int f(int input_num) {
   F(^)
 }
 }  // namespace ns
 )cpp");
 
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
     int bar(int param_in_bar) {
     }
 
     int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
     }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar"))));
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
   auto File = testPath("foo.cpp");
TEST(CompletionTest, Render) {
   R = C.render(Opts);
   EXPECT_EQ(R.label, "^[AS]Foo::x(bool) const");
 
   C.BundleSize = 2;
   R = C.render(Opts);
   EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\"");
 }
 
+TEST(CompletionTest, IgnoreRecoveryResults) {
+  auto Results = completions(
+      R"cpp(
+          namespace ns { int NotRecovered() { return 0; } }
+          void f() {
+            // Sema enters recovery mode first and then normal mode.
+            if (auto x = ns::NotRecover^)
+          }
+      )cpp");
+  EXPECT_THAT(Results.Completions, UnorderedElementsAre(Named("NotRecovered")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang