[clangd] Downrank symbols with reserved names (score *= 0.1)

[sammccall]

Forgot to add to debug string.

[ilya-biryukov]

Is there a way to only downrank them if the query does not start with _? That would cover the cases when I do want the symbols starting with underscore.

[sammccall]

In D47707#1120734, @ilya-biryukov wrote:

Is there a way to only downrank them if the query does not start with _? That would cover the cases when I do want the symbols starting with underscore.

This is hard for a few reasons:

• LSP's filter-in-the-editor isn't designed for this kind of case: once the server fails to return a result, it's basically gone
• a remote index now has to know the details of this scoring function (this is true of most things that are querystring-dependent)

This is a problem though. Added a couple of FIXMEs and will ponder a bit.

[sammccall]

FIXMEs pertaining to queries beginning with _, and standard _Names

[ilya-biryukov]

After chatting offline: a FIXME mentioning some special cases seems enough here, we do improve the results in most cases (i.e. for queries that don't start with an underscore).

Another NIT:
Maybe we could add unittests for names that start with underscore, but are not considered reserved by our implementation?
I'm thinking about "_" and "_non_upper_case_leading_char".

[sammccall]

Add test that _f isn't a reserved name.

[sammccall]

@ilya-biryukov Ping, anything left for me to do here?

[ilya-biryukov]

LGTM, sorry for the delay

clangd/Quality.h

 
 // 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 {
   bool Deprecated = false;
+  bool ReservedName = false; // __foo, _Foo are usually implementation details.
+                             // FIXME: make these findable once user types _.
   unsigned References = 0;
 
   enum SymbolCategory {
     Variable,
     Macro,
     Type,
     Function,
     Namespace,

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 "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/Basic/CharInfo.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
+static bool IsReserved(StringRef Name) {
+  // FIXME: Should we exclude _Bool and others recognized by the standard?
+  return Name.size() >= 2 && Name[0] == '_' &&
+         (isUppercase(Name[1]) || Name[1] == '_');
+}
 
 static bool hasDeclInMainFile(const Decl &D) {
   auto &SourceMgr = D.getASTContext().getSourceManager();
   for (auto *Redecl : D.redecls()) {
     auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation());
     if (SourceMgr.isWrittenInMainFile(Loc))
       return true;
   }

 void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
   if (SemaCCResult.Availability == CXAvailability_Deprecated)
     Deprecated = true;
 
   if (SemaCCResult.Declaration)
     Category = categorize(*SemaCCResult.Declaration);
   else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
     Category = Macro;
+
+  if (SemaCCResult.Declaration) {
+    if (auto *ID = SemaCCResult.Declaration->getIdentifier())
+      ReservedName = ReservedName || IsReserved(ID->getName());
+  } else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
+    ReservedName = ReservedName || IsReserved(SemaCCResult.Macro->getName());
 }
 
 void SymbolQualitySignals::merge(const Symbol &IndexResult) {
   References = std::max(IndexResult.References, References);
   Category = categorize(IndexResult.SymInfo);
+  ReservedName = ReservedName || IsReserved(IndexResult.Name);
 }
 
 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 (Deprecated)
     Score *= 0.1f;
+  if (ReservedName)
+    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());
   OS << formatv("\tReferences: {0}\n", S.References);
   OS << formatv("\tDeprecated: {0}\n", S.Deprecated);
+  OS << formatv("\tReserved name: {0}\n", S.ReservedName);
   OS << formatv("\tCategory: {0}\n", static_cast<int>(S.Category));
   return OS;
 }
 
 static SymbolRelevanceSignals::AccessibleScope
 ComputeScope(const NamedDecl &D) {
   bool InClass = false;
   for (const DeclContext *DC = D.getDeclContext(); !DC->isFileContext();

unittests/clangd/QualityTests.cpp

 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 TEST(QualityTests, SymbolQualitySignalExtraction) {
   auto Header = TestTU::withHeaderCode(R"cpp(
-    int x;
+    int _X;
 
     [[deprecated]]
-    int f() { return x; }
+    int _f() { return _X; }
   )cpp");
   auto Symbols = Header.headerSymbols();
   auto AST = Header.build();
 
   SymbolQualitySignals Quality;
-  Quality.merge(findSymbol(Symbols, "x"));
+  Quality.merge(findSymbol(Symbols, "_X"));
   EXPECT_FALSE(Quality.Deprecated);
+  EXPECT_TRUE(Quality.ReservedName);
   EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
 
-  Symbol F = findSymbol(Symbols, "f");
+  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_FALSE(Quality.ReservedName);
   EXPECT_EQ(Quality.References, 24u);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
 
   Quality = {};
-  Quality.merge(CodeCompletionResult(&findDecl(AST, "f"), /*Priority=*/42));
+  Quality.merge(CodeCompletionResult(&findDecl(AST, "_f"), /*Priority=*/42));
   EXPECT_TRUE(Quality.Deprecated);
+  EXPECT_FALSE(Quality.ReservedName);
   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) {
   SymbolQualitySignals Default;
   EXPECT_EQ(Default.evaluate(), 1);
 
   SymbolQualitySignals Deprecated;
   Deprecated.Deprecated = true;
   EXPECT_LT(Deprecated.evaluate(), Default.evaluate());
 
+  SymbolQualitySignals ReservedName;
+  ReservedName.ReservedName = true;
+  EXPECT_LT(ReservedName.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 Variable, Macro;
   Variable.Category = SymbolQualitySignals::Variable;