From 485074f9870020df212d467f56dd26b87d4859df Mon Sep 17 00:00:00 2001
From: Eric Liu <ioeric@google.com>
Date: Wed, 11 Jul 2018 13:15:31 +0000
Subject: [PATCH] [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     | 13 +++++++++++
 .../unittests/clangd/CodeCompleteTests.cpp    | 22 ++++++++++++++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 7392529a00d4..f4f6c39e2929 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/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.
diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
index cc35c4f62320..86a5f415163e 100644
--- a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ b/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