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

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).

Reviewers: sammccall, ilya-biryukov

Subscribers: MaskRay, jkorous, cfe-commits

Differential Revision: https://reviews.llvm.org/D49175

llvm-svn: 336801

clang-tools-extra/clangd/CodeComplete.cpp

@@ -586,6 +586,19 @@ struct CompletionRecorder : public CodeCompleteConsumer {
   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 might still want to provide some results (e.g. trivial
+    // identifier-based completion).
+    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.

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

@@ -787,20 +787,24 @@ int f(int input_num) {
   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) {
@@ -1293,6 +1297,18 @@ TEST(CompletionTest, Render) {
   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