[clangd] Sema ranking tweaks: downrank keywords and injected names.

[sammccall]

Summary

Injected names being ranked too high was just a bug.
The high boost for keywords was intended, but was too much given how useless
keywords are. We should probably boost them on a case-by-case basis eventually.

Before:

After:

[hokein]

The code looks good to me.

It'd be nicer if you can put ranking screenshots (before vs after) in the patch summary.

clangd/Quality.cpp

160 ↗

(On Diff #152088)

I'm curious on how we choose these magic numbers, by experience?

[sammccall]

Added before/after

clangd/Quality.cpp

160 ↗

(On Diff #152088)

Yeah, the constraint being satisfied here is boost(Keyword) < boost(FunctionScope) * boost(Variable)

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

if (References >= 3)
     Score *= std::log(References);
 
   if (Deprecated)
     Score *= 0.1f;
   if (ReservedName)
     Score *= 0.1f;
 
   switch (Category) {
-    case Keyword:  // Usually relevant, but misses most signals.
-      Score *= 10;
+    case Keyword:  // Often relevant, but misses most signals.
+      Score *= 4;  // FIXME: important keywords should have specific boosts.
       break;
     case Type:
     case Function:
     case Variable:
       Score *= 1.1f;
       break;
     case Namespace:
       Score *= 0.8f;
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
                               const FileProximityMatcher &M) {
   OS << formatv("File proximity matcher: ");
   OS << formatv("ProximityPaths[{0}]", llvm::join(M.ProximityPaths.begin(),
                                                   M.ProximityPaths.end(), ","));
   return OS;
 }
 
 static SymbolRelevanceSignals::AccessibleScope
-ComputeScope(const NamedDecl &D) {
+ComputeScope(const NamedDecl *D) {
+  // Injected "Foo" within the class "Foo" has file scope, not class scope.
+  const DeclContext *DC = D->getDeclContext();
+  if (auto *R = dyn_cast_or_null<RecordDecl>(D))
+    if (R->isInjectedClassName())
+      DC = DC->getParent();
   bool InClass = false;
-  for (const DeclContext *DC = D.getDeclContext(); !DC->isFileContext();
-       DC = DC->getParent()) {
+  for (; !DC->isFileContext(); DC = DC->getParent()) {
     if (DC->isFunctionOrMethod())
       return SymbolRelevanceSignals::FunctionScope;
     InClass = InClass || DC->isRecord();
   }
   if (InClass)
     return SymbolRelevanceSignals::ClassScope;
   // This threshold could be tweaked, e.g. to treat module-visible as global.
-  if (D.getLinkageInternal() < ExternalLinkage)
+  if (D->getLinkageInternal() < ExternalLinkage)
     return SymbolRelevanceSignals::FileScope;
   return SymbolRelevanceSignals::GlobalScope;
 }
 
 void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
   // FIXME: Index results always assumed to be at global scope. If Scope becomes
   // relevant to non-completion requests, we should recognize class members etc.
 
if (SemaCCResult.Declaration) {
     // translation unit.
     float DeclProximity =
         hasDeclInMainFile(*SemaCCResult.Declaration) ? 1.0 : 0.6;
     SemaProximityScore = std::max(DeclProximity, SemaProximityScore);
   }
 
   // Declarations are scoped, others (like macros) are assumed global.
   if (SemaCCResult.Declaration)
-    Scope = std::min(Scope, ComputeScope(*SemaCCResult.Declaration));
+    Scope = std::min(Scope, ComputeScope(SemaCCResult.Declaration));
 }
 
 float SymbolRelevanceSignals::evaluate() const {
   float Score = 1;
 
   if (Forbidden)
     return 0;
 

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

TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   Test.Code = R"cpp(
     int ::header_main() {}
     int main();
 
     [[deprecated]]
     int deprecated() { return 0; }
 
     namespace { struct X { void y() { int z; } }; }
+    struct S{}
   )cpp";
   auto AST = Test.build();
 
   SymbolRelevanceSignals Relevance;
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "deprecated"),
                                        /*Priority=*/42, nullptr, false,
                                        /*Accessible=*/false));
   EXPECT_EQ(Relevance.NameMatch, SymbolRelevanceSignals().NameMatch);
TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "X"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "y"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::ClassScope);
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "z"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FunctionScope);
+  // The injected class name is treated as the outer class name.
+  Relevance = {};
+  Relevance.merge(CodeCompletionResult(&findDecl(AST, "S::S"), 42));
+  EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
 }
 
 // Do the signals move the scores in the direction we expect?
 TEST(QualityTests, SymbolQualitySignalsSanity) {
   SymbolQualitySignals Default;
   EXPECT_EQ(Default.evaluate(), 1);
 
   SymbolQualitySignals Deprecated;