[clangd] Penalize file-scope symbols in the ranking for non-completion queries

[sammccall]

clang-tools-extra/clangd/Quality.cpp

289 ↗

(On Diff #181487)

(needs update after I inverted this flag to VisibleOutsideFile, sorry!)

385 ↗

(On Diff #181487)

I'd suggest being a bit more aggressive here: 0.5 or 0.4. Just guessing though, up to you.

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

104 ↗

(On Diff #181487)

can you add a main-file symbol here, and verify the scope property gets set on the SymbolRelevanceSignals correctly?

[nridge]

Address review comments

[nridge]

@sammccall, could you commit this patch for me? Thank you!

[nridge]

Friendly reminder than this is ready to be committed :)

[sammccall]

In D56653#1376664, @nridge wrote:

Friendly reminder than this is ready to be committed :)

Sorry, I lost track of this. Landed now. Thanks again!

clangd/Quality.cpp

if (InClass)
     return SymbolRelevanceSignals::ClassScope;
   // This threshold could be tweaked, e.g. to treat module-visible as global.
   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.
-
   SymbolURI = IndexResult.CanonicalDeclaration.FileURI;
   SymbolScope = IndexResult.Scope;
   IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);
+  if (!(IndexResult.Flags & Symbol::VisibleOutsideFile)) {
+    Scope = AccessibleScope::FileScope;
+  }
 }
 
 void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
   if (SemaCCResult.Availability == CXAvailability_NotAvailable ||
       SemaCCResult.Availability == CXAvailability_NotAccessible)
     Forbidden = true;
 
   if (SemaCCResult.Declaration) {
float SymbolRelevanceSignals::evaluate() const {
   // Conversely if we're in that scope, it's likely we'll reference them.
   if (Query == CodeComplete) {
     // The narrower the scope where a symbol is visible, the more likely it is
     // to be relevant when it is available.
     switch (Scope) {
     case GlobalScope:
       break;
     case FileScope:
-      Score *= 1.5;
+      Score *= 1.5f;
       break;
     case ClassScope:
       Score *= 2;
       break;
     case FunctionScope:
       Score *= 4;
       break;
     }
+  } else {
+    // For non-completion queries, the wider the scope where a symbol is
+    // visible, the more likely it is to be relevant.
+    switch (Scope) {
+    case GlobalScope:
+      break;
+    case FileScope:
+      Score *= 0.5f;
+      break;
+    default:
+      // TODO: Handle other scopes as we start to use them for index results.
+      break;
+    }
   }
 
   if (TypeMatchesPreferred)
     Score *= 5.0;
 
   // Penalize non-instance members when they are accessed via a class instance.
   if (!IsInstanceMember &&
       (Context == CodeCompletionContext::CCC_DotMemberAccess ||

unittests/clangd/QualityTests.cpp

TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
 
   Relevance = {};
   EXPECT_FALSE(Relevance.InBaseClass);
   auto BaseMember = CodeCompletionResult(&findUnqualifiedDecl(AST, "y"), 42);
   BaseMember.InBaseClass = true;
   Relevance.merge(BaseMember);
   EXPECT_TRUE(Relevance.InBaseClass);
+
+  auto Index = Test.index();
+  Symbol X;
+  FuzzyFindRequest Req;
+  Req.Query = "X";
+  Req.AnyScope = true;
+  Index->fuzzyFind(Req, [&X](const Symbol& S){ X = S; });
+  Relevance = {};
+  Relevance.merge(X);
+  EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
 }
 
 // Do the signals move the scores in the direction we expect?
 TEST(QualityTests, SymbolQualitySignalsSanity) {
   SymbolQualitySignals Default;
   EXPECT_EQ(Default.evaluate(), 1);
 
   SymbolQualitySignals Deprecated;
TEST(QualityTests, SymbolRelevanceSignalsSanity) {
   SymbolRelevanceSignals IndexDistant = IndexProximate;
   IndexDistant.SymbolURI = "unittest:/elsewhere/path.h";
   EXPECT_GT(IndexProximate.evaluate(), IndexDistant.evaluate())
       << IndexProximate << IndexDistant;
   EXPECT_GT(IndexDistant.evaluate(), Default.evaluate());
 
   SymbolRelevanceSignals Scoped;
   Scoped.Scope = SymbolRelevanceSignals::FileScope;
-  EXPECT_EQ(Scoped.evaluate(), Default.evaluate());
+  EXPECT_LT(Scoped.evaluate(), Default.evaluate());
   Scoped.Query = SymbolRelevanceSignals::CodeComplete;
   EXPECT_GT(Scoped.evaluate(), Default.evaluate());
 
   SymbolRelevanceSignals Instance;
   Instance.IsInstanceMember = false;
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
   Instance.Context = CodeCompletionContext::CCC_DotMemberAccess;
   EXPECT_LT(Instance.evaluate(), Default.evaluate());