[clangd] Code completion: drop explicit injected names/operators, ignore Sema priority

Summary:
Now we have most of Sema's code completion signals incorporated in Quality,
which will allow us to give consistent ranking to sema/index results.

Therefore we can/should stop using Sema priority as an explicit signal.
This fixes some issues like namespaces always having a terrible score.

The most important missing signals are:
 - Really dumb/rarely useful completions like:
    SomeStruct().^SomeStruct
    SomeStruct().^operator=
    SomeStruct().~SomeStruct()
   We already filter out destructors, this patch adds injected names and
   operators to that list.
 - type matching the expression context.
   Ilya has a plan to add this in a way that's compatible with indexes
   (design doc should be shared real soon now!)

Reviewers: ioeric

Subscribers: ilya-biryukov, MaskRay, jkorous, cfe-commits

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

llvm-svn: 334192

7 files changed, 61 insertions, 45 deletions

diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 6fb1d9a186c..0859d632ac6 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -467,6 +467,25 @@ bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
   llvm_unreachable("unknown code completion context");
 }
 
+// Some member calls are blacklisted because they're so rarely useful.
+static bool isBlacklistedMember(const NamedDecl &D) {
+  // Destructor completion is rarely useful, and works inconsistently.
+  // (s.^ completes ~string, but s.~st^ is an error).
+  if (D.getKind() == Decl::CXXDestructor)
+    return true;
+  // Injected name may be useful for A::foo(), but who writes A::A::foo()?
+  if (auto *R = dyn_cast_or_null<RecordDecl>(&D))
+    if (R->isInjectedClassName())
+      return true;
+  // Explicit calls to operators are also rare.
+  auto NameKind = D.getDeclName().getNameKind();
+  if (NameKind == DeclarationName::CXXOperatorName ||
+      NameKind == DeclarationName::CXXLiteralOperatorName ||
+      NameKind == DeclarationName::CXXConversionFunctionName)
+    return true;
+  return false;
+}
+
 // The CompletionRecorder captures Sema code-complete output, including context.
 // It filters out ignored results (but doesn't apply fuzzy-filtering yet).
 // It doesn't do scoring or conversion to CompletionItem yet, as we want to
@@ -520,9 +539,9 @@ struct CompletionRecorder : public CodeCompleteConsumer {
           (Result.Availability == CXAvailability_NotAvailable ||
            Result.Availability == CXAvailability_NotAccessible))
         continue;
-      // Destructor completion is rarely useful, and works inconsistently.
-      // (s.^ completes ~string, but s.~st^ is an error).
-      if (dyn_cast_or_null<CXXDestructorDecl>(Result.Declaration))
+      if (Result.Declaration &&
+          !Context.getBaseType().isNull() // is this a member-access context?
+          && isBlacklistedMember(*Result.Declaration))
         continue;
       // We choose to never append '::' to completion results in clangd.
       Result.StartsNestedNameSpecifier = false;
diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp
index ba453955c9c..00490274ad3 100644
--- a/clang-tools-extra/clangd/Quality.cpp
+++ b/clang-tools-extra/clangd/Quality.cpp
@@ -94,7 +94,6 @@ categorize(const index::SymbolInfo &D) {
 }
 
 void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
-  SemaCCPriority = SemaCCResult.Priority;
   if (SemaCCResult.Availability == CXAvailability_Deprecated)
     Deprecated = true;
 
@@ -117,11 +116,6 @@ float SymbolQualitySignals::evaluate() const {
   if (References >= 3)
     Score *= std::log(References);
 
-  if (SemaCCPriority)
-    // Map onto a 0-2 interval, so we don't reward/penalize non-Sema results.
-    // Priority 80 is a really bad score.
-    Score *= 2 - std::min<float>(80, SemaCCPriority) / 40;
-
   if (Deprecated)
     Score *= 0.1f;
 
@@ -146,8 +140,6 @@ float SymbolQualitySignals::evaluate() const {
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolQualitySignals &S) {
   OS << formatv("=== Symbol quality: {0}\n", S.evaluate());
-  if (S.SemaCCPriority)
-    OS << formatv("\tSemaCCPriority: {0}\n", S.SemaCCPriority);
   OS << formatv("\tReferences: {0}\n", S.References);
   OS << formatv("\tDeprecated: {0}\n", S.Deprecated);
   OS << formatv("\tCategory: {0}\n", static_cast<int>(S.Category));
diff --git a/clang-tools-extra/clangd/Quality.h b/clang-tools-extra/clangd/Quality.h
index 99731d9408a..c6068628675 100644
--- a/clang-tools-extra/clangd/Quality.h
+++ b/clang-tools-extra/clangd/Quality.h
@@ -44,9 +44,6 @@ struct Symbol;
 
 /// Attributes of a symbol that affect how much we like it.
 struct SymbolQualitySignals {
-  unsigned SemaCCPriority = 0; // 1-80, 1 is best. 0 means absent.
-                               // FIXME: this is actually a mix of symbol
-                               //        quality and relevance. Untangle this.
   bool Deprecated = false;
   unsigned References = 0;
 
diff --git a/clang-tools-extra/test/clangd/completion.test b/clang-tools-extra/test/clangd/completion.test
index da00b566faa..441cc7df63d 100644
--- a/clang-tools-extra/test/clangd/completion.test
+++ b/clang-tools-extra/test/clangd/completion.test
@@ -18,8 +18,8 @@
 # CHECK-NEXT:      "kind": 5,
 # CHECK-NEXT:      "label": "a",
 # CHECK-NEXT:      "sortText": "{{.*}}a"
-# CHECK-NEXT:    },
-#      CHECK:  ]
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
 ---
 # Update the source file and check for completions again.
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///main.cpp","version":2},"contentChanges":[{"text":"struct S { int b; };\nint main() {\nS().\n}"}]}}
@@ -38,7 +38,7 @@
 # CHECK-NEXT:      "kind": 5,
 # CHECK-NEXT:      "label": "b",
 # CHECK-NEXT:      "sortText": "{{.*}}b"
-# CHECK-NEXT:    },
-#      CHECK:  ]
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
 ---
 {"jsonrpc":"2.0","id":4,"method":"shutdown"}
diff --git a/clang-tools-extra/test/clangd/protocol.test b/clang-tools-extra/test/clangd/protocol.test
index 46af4edc7cd..fcbba15b4cc 100644
--- a/clang-tools-extra/test/clangd/protocol.test
+++ b/clang-tools-extra/test/clangd/protocol.test
@@ -34,11 +34,11 @@ Content-Length: 146
 # CHECK-NEXT:  "result": {

 # CHECK-NEXT:    "isIncomplete": false,

 # CHECK-NEXT:    "items": [

-#      CHECK:        "filterText": "fake",

-# CHECK-NEXT:        "insertText": "fake",

+#      CHECK:        "filterText": "a",

+# CHECK-NEXT:        "insertText": "a",

 # CHECK-NEXT:        "insertTextFormat": 1,

-# CHECK-NEXT:        "kind": 7,

-# CHECK-NEXT:        "label": "fake",

+# CHECK-NEXT:        "kind": 5,

+# CHECK-NEXT:        "label": "a",

 # CHECK-NEXT:        "sortText": "{{.*}}"

 #      CHECK:    ]

 # CHECK-NEXT:  }

@@ -63,11 +63,11 @@ Content-Length: 146
 # CHECK-NEXT:  "result": {

 # CHECK-NEXT:    "isIncomplete": false,

 # CHECK-NEXT:    "items": [

-#      CHECK:        "filterText": "fake",

-# CHECK-NEXT:        "insertText": "fake",

+#      CHECK:        "filterText": "a",

+# CHECK-NEXT:        "insertText": "a",

 # CHECK-NEXT:        "insertTextFormat": 1,

-# CHECK-NEXT:        "kind": 7,

-# CHECK-NEXT:        "label": "fake",

+# CHECK-NEXT:        "kind": 5,

+# CHECK-NEXT:        "label": "a",

 # CHECK-NEXT:        "sortText": "{{.*}}"

 #      CHECK:    ]

 # CHECK-NEXT:  }

@@ -92,11 +92,11 @@ Content-Length: 146
 # CHECK-NEXT:  "result": {

 # CHECK-NEXT:    "isIncomplete": false,

 # CHECK-NEXT:    "items": [

-#      CHECK:        "filterText": "fake",

-# CHECK-NEXT:        "insertText": "fake",

+#      CHECK:        "filterText": "a",

+# CHECK-NEXT:        "insertText": "a",

 # CHECK-NEXT:        "insertTextFormat": 1,

-# CHECK-NEXT:        "kind": 7,

-# CHECK-NEXT:        "label": "fake",

+# CHECK-NEXT:        "kind": 5,

+# CHECK-NEXT:        "label": "a",

 # CHECK-NEXT:        "sortText": "{{.*}}"

 #      CHECK:    ]

 # CHECK-NEXT:  }

diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
index 22653e7c10b..5f7cbb7957e 100644
--- a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -207,9 +207,6 @@ TEST(CompletionTest, Filter) {
   EXPECT_THAT(completions(Body + "int main() { S().FR^ }").items,
               AllOf(Has("FooBar"), Not(Has("FooBaz")), Not(Has("Qux"))));
 
-  EXPECT_THAT(completions(Body + "int main() { S().opr^ }").items,
-              Has("operator="));
-
   EXPECT_THAT(completions(Body + "int main() { aaa^ }").items,
               AllOf(Has("Abracadabra"), Has("Alakazam")));
 
@@ -250,9 +247,10 @@ void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
 
   // Class members. The only items that must be present in after-dot
   // completion.
-  EXPECT_THAT(
-      Results.items,
-      AllOf(Has(Opts.EnableSnippets ? "method()" : "method"), Has("field")));
+  EXPECT_THAT(Results.items,
+              AllOf(Has(Opts.EnableSnippets ? "method()" : "method"),
+                    Has("field"), Not(Has("ClassWithMembers")),
+                    Not(Has("operator=")), Not(Has("~ClassWithMembers"))));
   EXPECT_IFF(Opts.IncludeIneligibleResults, Results.items,
              Has("private_field"));
   // Global items.
@@ -379,6 +377,25 @@ TEST(CompletionTest, Qualifiers) {
   EXPECT_THAT(Results.items, Not(Contains(Labeled("foo() const")))); // private
 }
 
+TEST(CompletionTest, InjectedTypename) {
+  // These are suppressed when accessed as a member...
+  EXPECT_THAT(completions("struct X{}; void foo(){ X().^ }").items,
+              Not(Has("X")));
+  EXPECT_THAT(completions("struct X{ void foo(){ this->^ } };").items,
+              Not(Has("X")));
+  // ...but accessible in other, more useful cases.
+  EXPECT_THAT(completions("struct X{ void foo(){ ^ } };").items, Has("X"));
+  EXPECT_THAT(completions("struct Y{}; struct X:Y{ void foo(){ ^ } };").items,
+              Has("Y"));
+  EXPECT_THAT(
+      completions(
+          "template<class> struct Y{}; struct X:Y<int>{ void foo(){ ^ } };")
+          .items,
+      Has("Y"));
+  // This case is marginal (`using X::X` is useful), we allow it for now.
+  EXPECT_THAT(completions("struct X{}; void foo(){ X::^ }").items, Has("X"));
+}
+
 TEST(CompletionTest, Snippets) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
diff --git a/clang-tools-extra/unittests/clangd/QualityTests.cpp b/clang-tools-extra/unittests/clangd/QualityTests.cpp
index cb8ace267ae..4b16dd59c01 100644
--- a/clang-tools-extra/unittests/clangd/QualityTests.cpp
+++ b/clang-tools-extra/unittests/clangd/QualityTests.cpp
@@ -39,7 +39,6 @@ TEST(QualityTests, SymbolQualitySignalExtraction) {
   SymbolQualitySignals Quality;
   Quality.merge(findSymbol(Symbols, "x"));
   EXPECT_FALSE(Quality.Deprecated);
-  EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
   EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
 
@@ -48,14 +47,12 @@ TEST(QualityTests, SymbolQualitySignalExtraction) {
   Quality = {};
   Quality.merge(F);
   EXPECT_FALSE(Quality.Deprecated); // FIXME: Include deprecated bit in index.
-  EXPECT_EQ(Quality.SemaCCPriority, SymbolQualitySignals().SemaCCPriority);
   EXPECT_EQ(Quality.References, 24u);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
 
   Quality = {};
   Quality.merge(CodeCompletionResult(&findDecl(AST, "f"), /*Priority=*/42));
   EXPECT_TRUE(Quality.Deprecated);
-  EXPECT_EQ(Quality.SemaCCPriority, 42u);
   EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
 }
@@ -121,12 +118,6 @@ TEST(QualityTests, SymbolQualitySignalsSanity) {
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
-  SymbolQualitySignals LowPriority, HighPriority;
-  LowPriority.SemaCCPriority = 60;
-  HighPriority.SemaCCPriority = 20;
-  EXPECT_GT(HighPriority.evaluate(), Default.evaluate());
-  EXPECT_LT(LowPriority.evaluate(), Default.evaluate());
-
   SymbolQualitySignals Variable, Macro;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;