[clangd] Penalize non-instance members when accessed via class instances.

[ioeric]

Summary

The following are metrics for explicit member access completions. There is no
noticeable impact on other completion types.

Before:
EXPLICIT_MEMBER_ACCESS

Total measurements: 24382
All measurements: MRR: 62.27	Top10: 80.21%	Top-100: 94.48%
Full identifiers: MRR: 98.81	Top10: 99.89%	Top-100: 99.95%
0-5 filter len:

MRR: 13.25 46.31 62.47 67.77 70.40 81.91
Top-10: 29% 74% 84% 91% 91% 97%
Top-100: 67% 99% 99% 99% 99% 100%

After:
EXPLICIT_MEMBER_ACCESS

Total measurements: 24382
All measurements: MRR: 63.18	Top10: 80.58%	Top-100: 95.07%
Full identifiers: MRR: 98.79	Top10: 99.89%	Top-100: 99.95%
0-5 filter len:

MRR: 13.84 48.39 63.55 68.83 71.28 82.64
Top-10: 30% 75% 84% 91% 91% 97%
Top-100: 70% 99% 99% 99% 99% 100%

• Top-N: wanted result is found in the first N completion results.
• MRR: Mean reciprocal rank.

Remark: the change seems to have minor positive impact. Although the improvement
is relatively small, down-ranking non-instance members in instance member access
should reduce noise in the completion results.

[sammccall]

clangd/CodeComplete.h

148 ↗

(On Diff #156253)

nit: "sema" is a bit down-in-the-weeds here, maybe just 'Context' or 'DetectedContext'?

148 ↗

(On Diff #156253)

can you add an a test to CodeCompleteTests for this new API?

clangd/Quality.cpp

146 ↗

(On Diff #156253)

I think we also have to consider template functions too?
And ideally I think we want to exclude constructors (but include destructors).

325 ↗

(On Diff #156253)

could be even harsher here, but this is fine for now

clangd/Quality.h

102 ↗

(On Diff #156253)

again, I'd drop "sema" here. It's not as though we can get several contexts from different sources.

[ioeric]

• addressed review comments.

[ioeric]

clangd/Quality.cpp

146 ↗

(On Diff #156253)

Done for template functions.

I'm not very sure whether constructors are instance members in general, but they should already be filtered out by sema in dot/arror member accessed.

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

 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H
 
 #include "Headers.h"
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "index/Index.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/CodeCompleteOptions.h"
 #include "clang/Tooling/CompilationDatabase.h"
 
 namespace clang {
 class NamedDecl;
 class PCHContainerOperations;
 namespace clangd {
 
struct CodeCompletion {
 
   // Serialize this to an LSP completion item. This is a lossy operation.
   CompletionItem render(const CodeCompleteOptions &) const;
 };
 raw_ostream &operator<<(raw_ostream &, const CodeCompletion &);
 struct CodeCompleteResult {
   std::vector<CodeCompletion> Completions;
   bool HasMore = false;
+  CodeCompletionContext::Kind Context = CodeCompletionContext::CCC_Other;
 };
 raw_ostream &operator<<(raw_ostream &, const CodeCompleteResult &);
 
 /// Get code completions at a specified \p Pos in \p FileName.
 CodeCompleteResult codeComplete(PathRef FileName,
                                 const tooling::CompileCommand &Command,
                                 PrecompiledPreamble const *Preamble,
                                 const IncludeStructure &PreambleInclusions,

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

CodeCompleteResult runWithSema() {
     auto Top = mergeResults(Recorder->Results, IndexResults);
     // Convert the results to final form, assembling the expensive strings.
     CodeCompleteResult Output;
     for (auto &C : Top) {
       Output.Completions.push_back(toCodeCompletion(C.first));
       Output.Completions.back().Score = C.second;
     }
     Output.HasMore = Incomplete;
+    Output.Context = Recorder->CCContext.getKind();
     return Output;
   }
 
   SymbolSlab queryIndex() {
     trace::Span Tracer("Query index");
     SPAN_ATTACH(Tracer, "limit", int64_t(Opts.Limit));
 
     SymbolSlab::Builder ResultsBuilder;
Optional<float> fuzzyScore(const CompletionCandidate &C) {
     return Filter->match(C.Name);
   }
 
   // Scores a candidate and adds it to the TopN structure.
   void addCandidate(TopN<ScoredBundle, ScoredBundleGreater> &Candidates,
                     CompletionCandidate::Bundle Bundle) {
     SymbolQualitySignals Quality;
     SymbolRelevanceSignals Relevance;
+    Relevance.Context = Recorder->CCContext.getKind();
     Relevance.Query = SymbolRelevanceSignals::CodeComplete;
     Relevance.FileProximityMatch = FileProximity.getPointer();
     auto &First = Bundle.front();
     if (auto FuzzyScore = fuzzyScore(First))
       Relevance.NameMatch = *FuzzyScore;
     else
       return;
     SymbolOrigin Origin = SymbolOrigin::Unknown;

 
 raw_ostream &operator<<(raw_ostream &OS, const CodeCompletion &C) {
   // For now just lean on CompletionItem.
   return OS << C.render(CodeCompleteOptions());
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const CodeCompleteResult &R) {
   OS << "CodeCompleteResult: " << R.Completions.size() << (R.HasMore ? "+" : "")
+     << " (" << getCompletionKindString(R.Context) << ")"
      << " items:\n";
   for (const auto &C : R.Completions)
     OS << C << "\n";
   return OS;
 }
 
 } // namespace clangd
 } // namespace clang

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

 ///     consistent regardless of the source.
 ///   - compute scores from scoring signals. These are suitable for sorting.
 ///   - sorting utilities like the TopN container.
 /// These could be split up further to isolate dependencies if we care.
 ///
 //===---------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include <algorithm>
 #include <functional>
 #include <vector>
 namespace llvm {
 class raw_ostream;
 }
enum AccessibleScope {
     GlobalScope,
   } Scope = GlobalScope;
 
   enum QueryType {
     CodeComplete,
     Generic,
   } Query = Generic;
 
+  CodeCompletionContext::Kind Context = CodeCompletionContext::CCC_Other;
+
+  // Whether symbol is an instance member of a class.
+  bool IsInstanceMember = false;
+
   void merge(const CodeCompletionResult &SemaResult);
   void merge(const Symbol &IndexResult);
 
   // Condense these signals down to a single number, higher is better.
   float evaluate() const;
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &,
                               const SymbolRelevanceSignals &);

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

 //===--- Quality.cpp --------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 #include "Quality.h"
 #include "FileDistance.h"
 #include "URI.h"
 #include "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
categorize(const index::SymbolInfo &D) {
   case index::SymbolKind::Using:
   case index::SymbolKind::Module:
   case index::SymbolKind::Unknown:
     return SymbolQualitySignals::Unknown;
   }
   llvm_unreachable("Unknown index::SymbolKind");
 }
 
+static bool isInstanceMember(const NamedDecl *ND) {
+  if (!ND)
+    return false;
+  if (const auto *TP = dyn_cast<FunctionTemplateDecl>(ND))
+    ND = TP->TemplateDecl::getTemplatedDecl();
+  if (const auto *CM = dyn_cast<CXXMethodDecl>(ND))
+    return !CM->isStatic();
+  return isa<FieldDecl>(ND); // Note that static fields are VarDecl.
+}
+
+static bool isInstanceMember(const index::SymbolInfo &D) {
+  switch (D.Kind) {
+  case index::SymbolKind::InstanceMethod:
+  case index::SymbolKind::InstanceProperty:
+  case index::SymbolKind::Field:
+    return true;
+  default:
+    return false;
+  }
+}
+
 void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
   if (SemaCCResult.Availability == CXAvailability_Deprecated)
     Deprecated = true;
 
   Category = categorize(SemaCCResult);
 
   if (SemaCCResult.Declaration) {
     if (auto *ID = SemaCCResult.Declaration->getIdentifier())
ComputeScope(const NamedDecl *D) {
   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;
+  IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);
 }
 
 void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
   if (SemaCCResult.Availability == CXAvailability_NotAvailable ||
       SemaCCResult.Availability == CXAvailability_NotAccessible)
     Forbidden = true;
 
   if (SemaCCResult.Declaration) {
     // We boost things that have decls in the main file. We give a fixed score
     // 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;
     SemaProximityScore = std::max(DeclProximity, SemaProximityScore);
+    IsInstanceMember |= isInstanceMember(SemaCCResult.Declaration);
   }
 
   // Declarations are scoped, others (like macros) are assumed global.
   if (SemaCCResult.Declaration)
     Scope = std::min(Scope, ComputeScope(SemaCCResult.Declaration));
 }
 
 static std::pair<float, unsigned> proximityScore(llvm::StringRef SymbolURI,
case ClassScope:
       Score *= 2;
       break;
     case FunctionScope:
       Score *= 4;
       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;
+  }
+
   return Score;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolRelevanceSignals &S) {
   OS << formatv("=== Symbol relevance: {0}\n", S.evaluate());
   OS << formatv("\tName match: {0}\n", S.NameMatch);
   OS << formatv("\tForbidden: {0}\n", S.Forbidden);
+  OS << formatv("\tIsInstanceMember: {0}\n", S.IsInstanceMember);
+  OS << formatv("\tContext: {0}\n", getCompletionKindString(S.Context));
   OS << formatv("\tSymbol URI: {0}\n", S.SymbolURI);
   if (S.FileProximityMatch) {
     auto Score = proximityScore(S.SymbolURI, S.FileProximityMatch);
     OS << formatv("\tIndex proximity: {0} (distance={1})\n", Score.first,
                   Score.second);
   }
   OS << formatv("\tSema proximity: {0}\n", S.SemaProximityScore);
   OS << formatv("\tQuery type: {0}\n", static_cast<int>(S.Query));

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

 #include "Compiler.h"
 #include "Matchers.h"
 #include "Protocol.h"
 #include "Quality.h"
 #include "SourceCode.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "index/MemIndex.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
auto Results = completions(
           class X { public: X(); int x_; };
           X::X() : x_^(0) {}
         }
       )cpp");
   EXPECT_THAT(Results.Completions,
               UnorderedElementsAre(AllOf(Scope("ns::X::"), Named("x_"))));
 }
 
+TEST(CompletionTest, CodeCompletionContext) {
+  auto Results = completions(
+      R"cpp(
+        namespace ns {
+          class X { public: X(); int x_; };
+          void f() {
+            X x;
+            x.^;
+          }
+        }
+      )cpp");
+
+  EXPECT_THAT(Results.Context, CodeCompletionContext::CCC_DotMemberAccess);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 //===----------------------------------------------------------------------===//
 
 #include "FileDistance.h"
 #include "Quality.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
 // Force the unittest URI scheme to be linked,
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());
   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());
+  Instance.IsInstanceMember = true;
+  EXPECT_EQ(Instance.evaluate(), Default.evaluate());
 }
 
 TEST(QualityTests, SortText) {
   EXPECT_LT(sortText(std::numeric_limits<float>::infinity()),
             sortText(1000.2f));
   EXPECT_LT(sortText(1000.2f), sortText(1));
   EXPECT_LT(sortText(1), sortText(0.3f));
   EXPECT_LT(sortText(0.3f), sortText(0));
TEST(QualityTests, NoBoostForClassConstructor) {
   });
   SymbolRelevanceSignals Ctor;
   Ctor.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
 
   EXPECT_EQ(Cls.Scope, SymbolRelevanceSignals::GlobalScope);
   EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope);
 }
 
+TEST(QualityTests, IsInstanceMember) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+    class Foo {
+    public:
+      static void foo() {}
+
+      template <typename T> void tpl(T *t) {}
+
+      void bar() {}
+    };
+  )cpp");
+  auto Symbols = Header.headerSymbols();
+
+  SymbolRelevanceSignals Rel;
+  const Symbol &FooSym = findSymbol(Symbols, "Foo::foo");
+  Rel.merge(FooSym);
+  EXPECT_FALSE(Rel.IsInstanceMember);
+  const Symbol &BarSym = findSymbol(Symbols, "Foo::bar");
+  Rel.merge(BarSym);
+  EXPECT_TRUE(Rel.IsInstanceMember);
+
+  Rel.IsInstanceMember =false;
+  const Symbol &TplSym = findSymbol(Symbols, "Foo::tpl");
+  Rel.merge(TplSym);
+  EXPECT_TRUE(Rel.IsInstanceMember);
+
+  auto AST = Header.build();
+  const NamedDecl *Foo = &findDecl(AST, "Foo::foo");
+  const NamedDecl *Bar = &findDecl(AST, "Foo::bar");
+  const NamedDecl *Tpl = &findDecl(AST, "Foo::tpl");
+
+  Rel.IsInstanceMember = false;
+  Rel.merge(CodeCompletionResult(Foo, /*Priority=*/0));
+  EXPECT_FALSE(Rel.IsInstanceMember);
+  Rel.merge(CodeCompletionResult(Bar, /*Priority=*/0));
+  EXPECT_TRUE(Rel.IsInstanceMember);
+  Rel.IsInstanceMember = false;
+  Rel.merge(CodeCompletionResult(Tpl, /*Priority=*/0));
+  EXPECT_TRUE(Rel.IsInstanceMember);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang