[clangd] Downrank members from base class

[ilya-biryukov]

unittests/clangd/QualityTests.cpp

187 ↗

(On Diff #170839)

The test case was (accidentally?) removed. Maybe restore?

[ioeric]

unittests/clangd/QualityTests.cpp

187 ↗

(On Diff #170839)

Thanks for the catch!

[ioeric]

• restore accidentally removed test.

[sammccall]

clangd/Quality.cpp

380 ↗

(On Diff #170862)

This seems like a pretty light penalty to me, I'd consider 0.5...

[ioeric]

• adjust parameter

[ioeric]

clangd/Quality.cpp

380 ↗

(On Diff #170862)

0.5 sounds reasonable. I think we should penalize the non-instance member case more if this is 0.5 though. Made that 0.2. WDYT?

[sammccall]

clangd/Quality.cpp

380 ↗

(On Diff #170862)

Agree, LG!

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

 
 /// Attributes of a symbol-query pair that affect how much we like it.
 struct SymbolRelevanceSignals {
   /// 0-1+ fuzzy-match score for unqualified name. Must be explicitly assigned.
   float NameMatch = 1;
   bool Forbidden = false; // Unavailable (e.g const) or inaccessible (private).
   /// Whether fixits needs to be applied for that completion or not.
   bool NeedsFixIts = false;
+  bool InBaseClass = false; // A member from base class of the accessed class.
 
   URIDistance *FileProximityMatch = nullptr;
   /// These are used to calculate proximity between the index symbol and the
   /// query.
   llvm::StringRef SymbolURI;
   /// FIXME: unify with index proximity score - signals should be
   /// source-independent.
   /// Proximity between best declaration and the query. [0-1], 1 is closest.

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

if (SemaCCResult.Declaration) {
     // for all other declarations in sema as they are already included in the
     // translation unit.
     float DeclProximity = (hasDeclInMainFile(*SemaCCResult.Declaration) ||
                            hasUsingDeclInMainFile(SemaCCResult))
                               ? 1.0
                               : 0.6;
     SemaFileProximityScore = std::max(DeclProximity, SemaFileProximityScore);
     IsInstanceMember |= isInstanceMember(SemaCCResult.Declaration);
+    InBaseClass |= SemaCCResult.InBaseClass;
   }
 
   // Declarations are scoped, others (like macros) are assumed global.
   if (SemaCCResult.Declaration)
     Scope = std::min(Scope, computeScope(SemaCCResult.Declaration));
 
   NeedsFixIts = !SemaCCResult.FixIts.empty();
 }
case FunctionScope:
       break;
     }
   }
 
   // Penalize non-instance members when they are accessed via a class instance.
   if (!IsInstanceMember &&
       (Context == CodeCompletionContext::CCC_DotMemberAccess ||
        Context == CodeCompletionContext::CCC_ArrowMemberAccess)) {
-    Score *= 0.5;
+    Score *= 0.2;
   }
 
+  if (InBaseClass)
+    Score *= 0.5;
+
   // Penalize for FixIts.
   if (NeedsFixIts)
     Score *= 0.5;
 
   return Score;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolRelevanceSignals &S) {

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

TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   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);
+
+  Relevance = {};
+  EXPECT_FALSE(Relevance.InBaseClass);
+  auto BaseMember = CodeCompletionResult(&findAnyDecl(AST, "y"), 42);
+  BaseMember.InBaseClass = true;
+  Relevance.merge(BaseMember);
+  EXPECT_TRUE(Relevance.InBaseClass);
 }
 
 // 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 Instance;
   Instance.IsInstanceMember = false;
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
   Instance.Context = CodeCompletionContext::CCC_DotMemberAccess;
   EXPECT_LT(Instance.evaluate(), Default.evaluate());
   Instance.IsInstanceMember = true;
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
+
+  SymbolRelevanceSignals InBaseClass;
+  InBaseClass.InBaseClass = true;
+  EXPECT_LT(InBaseClass.evaluate(), Default.evaluate());
 }
 
 TEST(QualityTests, ScopeProximity) {
   SymbolRelevanceSignals Relevance;
   ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
   Relevance.ScopeProximityMatch = &ScopeProximity;
 
   Relevance.SymbolScope = "other::";