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

[sammccall]

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

[ilya-biryukov]

Maybe we could add the test cases that the blacklisted members still show up in completion that don't involve member qualifiers? For example,

struct X : std::vector<int> { 
  int test( ){
     // <-- 'vector' might be a useful completion here.
  }
};

[ioeric]

clangd/CodeComplete.cpp

542 ↗

(On Diff #150290)

!Context.getBaseType().isNull() probably deserves a comment.

[sammccall]

Added explicit tests for this feature.

[sammccall]

Added tests and comments.

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

bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
   case CodeCompletionContext::CCC_ObjCInstanceMessage:
   case CodeCompletionContext::CCC_ObjCClassMessage:
   case CodeCompletionContext::CCC_Recovery:
     return false;
   }
   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
 // merge with index results first.
 // Generally the fields and methods of this object should only be used from
 // within the callback.
 struct CompletionRecorder : public CodeCompleteConsumer {
   CompletionRecorder(const CodeCompleteOptions &Opts,
for (unsigned I = 0; I < NumResults; ++I) {
       // Drop hidden items which cannot be found by lookup after completion.
       // Exception: some items can be named by using a qualifier.
       if (Result.Hidden && (!Result.Qualifier || Result.QualifierIsInformative))
         continue;
       if (!Opts.IncludeIneligibleResults &&
           (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;
       Results.push_back(Result);
     }
     ResultsCallback();
   }
 

clang-tools-extra/trunk/clangd/Quality.h

 struct Symbol;
 
 // Signals structs are designed to be aggregated from 0 or more sources.
 // A default instance has neutral signals, and sources are merged into it.
 // They can be dumped for debugging, and evaluate()d into a score.
 
 /// 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;
 
   enum SymbolCategory {
     Variable,
     Macro,
     Type,
     Function,

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

switch (D.Kind) {
     case index::SymbolKind::Module:
     case index::SymbolKind::Unknown:
       return SymbolQualitySignals::Unknown;
   }
   llvm_unreachable("Unknown index::SymbolKind");
 }
 
 void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
-  SemaCCPriority = SemaCCResult.Priority;
   if (SemaCCResult.Availability == CXAvailability_Deprecated)
     Deprecated = true;
 
   if (SemaCCResult.Declaration)
     Category = categorize(*SemaCCResult.Declaration);
   else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
     Category = Macro;
 }
 
 void SymbolQualitySignals::merge(const Symbol &IndexResult) {
   References = std::max(IndexResult.References, References);
   Category = categorize(IndexResult.SymInfo);
 }
 
 float SymbolQualitySignals::evaluate() const {
   float Score = 1;
 
   // This avoids a sharp gradient for tail symbols, and also neatly avoids the
   // question of whether 0 references means a bad symbol or missing data.
   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;
 
   switch (Category) {
     case Type:
     case Function:
     case Variable:
       Score *= 1.1f;
       break;
     case Namespace:
       Score *= 0.8f;
       break;
     case Macro:
       Score *= 0.2f;
       break;
     case Unknown:
       break;
   }
 
   return Score;
 }
 
 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));
   return OS;
 }
 
 static SymbolRelevanceSignals::AccessibleScope
 ComputeScope(const NamedDecl &D) {

clang-tools-extra/trunk/test/clangd/completion.test

 # CHECK-NEXT:    {
 # CHECK-NEXT:      "detail": "int",
 # CHECK-NEXT:      "filterText": "a",
 # CHECK-NEXT:      "insertText": "a",
 # CHECK-NEXT:      "insertTextFormat": 1,
 # 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}"}]}}
 ---
 {"jsonrpc":"2.0","id":3,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":4}}}
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "detail": "int",
 # CHECK-NEXT:      "filterText": "b",
 # CHECK-NEXT:      "insertText": "b",
 # CHECK-NEXT:      "insertTextFormat": 1,
 # 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"}

clang-tools-extra/trunk/test/clangd/protocol.test

 {"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 # Test message with Content-Type before Content-Length
 #
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # 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:  }
 
 X-Test: Testing
 Content-Type: application/vscode-jsonrpc; charset-utf-8
 Content-Length: 146
 Content-Type: application/vscode-jsonrpc; charset-utf-8
 X-Testing: Test
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 
 Content-Type: application/vscode-jsonrpc; charset-utf-8
 Content-Length: 10
 Content-Length: 146
 
 {"jsonrpc":"2.0","id":3,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 # Test message with duplicate Content-Length headers
 #
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # 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:  }
 # STDERR: Warning: Duplicate Content-Length header received. The previous value for this message (10) was ignored.
 
 Content-Type: application/vscode-jsonrpc; charset-utf-8
 Content-Length: 10
 
 {"jsonrpc":"2.0","id":4,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 # Test message with malformed Content-Length
 #
 # STDERR: JSON parse error
 # Ensure we recover by sending another (valid) message
 
 Content-Length: 146
 
 {"jsonrpc":"2.0","id":5,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 # Test message with Content-Type before Content-Length
 #
 #      CHECK:  "id": 5,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # 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:  }
 Content-Length: 1024
 
 {"jsonrpc":"2.0","id":5,"method":"textDocument/completion","params":{"textDocument":{"uri":"test:/main.cpp"},"position":{"line":3,"character":5}}}
 # Test message which reads beyond the end of the stream.
 #
 # Ensure this is the last test in the file!
 # STDERR: Input was aborted. Read only {{[0-9]+}} bytes of expected {{[0-9]+}}.
 

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

std::string Body = R"cpp(
     };
   )cpp";
   EXPECT_THAT(completions(Body + "int main() { S().Foba^ }").items,
               AllOf(Has("FooBar"), Has("FooBaz"), Not(Has("Qux"))));
 
   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")));
 
   EXPECT_THAT(completions(Body + "int main() { _a^ }").items,
               AllOf(Has("static_cast"), Not(Has("Abracadabra"))));
 }
 
 void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
auto Results = completions(
 
         ClassWithMembers().^
       }
       )cpp",
       {cls("IndexClass"), var("index_var"), func("index_func")}, 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.
   EXPECT_THAT(
       Results.items,
       Not(AnyOf(Has("global_var"), Has("index_var"), Has("global_func"),
                 Has("global_func()"), Has("index_func"), Has("GlobalClass"),
                 Has("IndexClass"), Has("MACRO"), Has("LocalClass"))));
auto Results = completions(R"cpp(
       };
       void test() { Bar().^ }
   )cpp");
   EXPECT_THAT(Results.items, HasSubsequence(Labeled("bar() const"),
                                             Labeled("Foo::foo() const")));
   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;
   auto Results = completions(
       R"cpp(
       struct fake {
         int a;
         int f(int i, const float f) const;

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

auto Header = TestTU::withHeaderCode(R"cpp(
     int f() { return x; }
   )cpp");
   auto Symbols = Header.headerSymbols();
   auto AST = Header.build();
 
   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);
 
   Symbol F = findSymbol(Symbols, "f");
   F.References = 24; // TestTU doesn't count references, so fake it.
   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);
 }
 
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
     int header();
TEST(QualityTests, SymbolQualitySignalsSanity) {
   EXPECT_LT(Deprecated.evaluate(), Default.evaluate());
 
   SymbolQualitySignals WithReferences, ManyReferences;
   WithReferences.References = 10;
   ManyReferences.References = 1000;
   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;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {