[clangd] Names that are not spelled in source code are reserved.

Summary:
These are often not expected to be used directly e.g.
```
TEST_F(Fixture, X) {
  ^  // "Fixture_X_Test" expanded in the macro should be down ranked.
}
```

Only doing this for sema for now, as such symbols are mostly coming from sema
e.g. gtest macros expanded in the main file. We could also add a similar field
for the index symbol.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D53374

llvm-svn: 344736

clang-tools-extra/clangd/AST.cpp

@@ -11,6 +11,7 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
 
@@ -18,25 +19,34 @@ namespace clang {
 namespace clangd {
 using namespace llvm;
 
-SourceLocation findNameLoc(const clang::Decl* D) {
-  const auto& SM = D->getASTContext().getSourceManager();
+// Returns true if the complete name of decl \p D is spelled in the source code.
+// This is not the case for
+//   * symbols formed via macro concatenation, the spelling location will
+//     be "<scratch space>"
+//   * symbols controlled and defined by a compile command-line option
+//     `-DName=foo`, the spelling location will be "<command line>".
+bool isSpelledInSourceCode(const Decl *D) {
+  const auto &SM = D->getASTContext().getSourceManager();
+  auto Loc = D->getLocation();
   // FIXME: Revisit the strategy, the heuristic is limitted when handling
   // macros, we should use the location where the whole definition occurs.
-  SourceLocation SpellingLoc = SM.getSpellingLoc(D->getLocation());
-  if (D->getLocation().isMacroID()) {
-    std::string PrintLoc = SpellingLoc.printToString(SM);
+  if (Loc.isMacroID()) {
+    std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
     if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
-        llvm::StringRef(PrintLoc).startswith("<command line>")) {
-      // We use the expansion location for the following symbols, as spelling
-      // locations of these symbols are not interesting to us:
-      //   * symbols formed via macro concatenation, the spelling location will
-      //     be "<scratch space>"
-      //   * symbols controlled and defined by a compile command-line option
-      //     `-DName=foo`, the spelling location will be "<command line>".
-      SpellingLoc = SM.getExpansionRange(D->getLocation()).getBegin();
-    }
+        llvm::StringRef(PrintLoc).startswith("<command line>"))
+      return false;
   }
-  return SpellingLoc;
+  return true;
+}
+
+bool isImplementationDetail(const Decl *D) { return !isSpelledInSourceCode(D); }
+
+SourceLocation findNameLoc(const clang::Decl* D) {
+  const auto &SM = D->getASTContext().getSourceManager();
+  if (!isSpelledInSourceCode(D))
+    // Use the expansion location as spelling location is not interesting.
+    return SM.getExpansionRange(D->getLocation()).getBegin();
+  return SM.getSpellingLoc(D->getLocation());
 }
 
 std::string printQualifiedName(const NamedDecl &ND) {

clang-tools-extra/clangd/AST.h

@@ -24,6 +24,11 @@ class Decl;
 
 namespace clangd {
 
+/// Returns true if the declaration is considered implementation detail based on
+/// heuristics. For example, a declaration whose name is not explicitly spelled
+/// in code is considered implementation detail.
+bool isImplementationDetail(const Decl *D);
+
 /// Find the identifier source location of the given D.
 ///
 /// The returned location is usually the spelling location where the name of the

clang-tools-extra/clangd/Quality.cpp

@@ -7,6 +7,7 @@
 //
 //===----------------------------------------------------------------------===//
 #include "Quality.h"
+#include "AST.h"
 #include "FileDistance.h"
 #include "URI.h"
 #include "index/Index.h"
@@ -177,6 +178,7 @@ void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
   Category = categorize(SemaCCResult);
 
   if (SemaCCResult.Declaration) {
+    ImplementationDetail |= isImplementationDetail(SemaCCResult.Declaration);
     if (auto *ID = SemaCCResult.Declaration->getIdentifier())
       ReservedName = ReservedName || isReserved(ID->getName());
   } else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
@@ -185,6 +187,7 @@ void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
 
 void SymbolQualitySignals::merge(const Symbol &IndexResult) {
   Deprecated |= (IndexResult.Flags & Symbol::Deprecated);
+  ImplementationDetail |= (IndexResult.Flags & Symbol::ImplementationDetail);
   References = std::max(IndexResult.References, References);
   Category = categorize(IndexResult.SymInfo);
   ReservedName = ReservedName || isReserved(IndexResult.Name);
@@ -212,6 +215,8 @@ float SymbolQualitySignals::evaluate() const {
     Score *= 0.1f;
   if (ReservedName)
     Score *= 0.1f;
+  if (ImplementationDetail)
+    Score *= 0.2f;
 
   switch (Category) {
   case Keyword: // Often relevant, but misses most signals.

clang-tools-extra/clangd/Quality.h

@@ -57,6 +57,7 @@ struct SymbolQualitySignals {
   bool Deprecated = false;
   bool ReservedName = false; // __foo, _Foo are usually implementation details.
                              // FIXME: make these findable once user types _.
+  bool ImplementationDetail = false;
   unsigned References = 0;
 
   enum SymbolCategory {

clang-tools-extra/clangd/index/Index.h

@@ -259,6 +259,8 @@ struct Symbol {
     IndexedForCodeCompletion = 1 << 0,
     /// Indicates if the symbol is deprecated.
     Deprecated = 1 << 1,
+    // Symbol is an implementation detail.
+    ImplementationDetail = 1 << 2,
   };
 
   SymbolFlag Flags = SymbolFlag::None;

clang-tools-extra/clangd/index/SymbolCollector.cpp

@@ -536,6 +536,8 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
 
   if (isIndexedForCodeCompletion(ND, Ctx))
     S.Flags |= Symbol::IndexedForCodeCompletion;
+  if (isImplementationDetail(&ND))
+    S.Flags |= Symbol::ImplementationDetail;
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   if (auto DeclLoc = getTokenLocation(findNameLoc(&ND), SM, Opts,

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

@@ -45,17 +45,34 @@ TEST(QualityTests, SymbolQualitySignalExtraction) {
 
     [[deprecated]]
     int _f() { return _X; }
+
+    #define DECL_NAME(x, y) x##_##y##_Decl
+    #define DECL(x, y) class DECL_NAME(x, y) {};
+    DECL(X, Y); // X_Y_Decl
+
+    class MAC {};
   )cpp");
+  Header.ExtraArgs = {"-DMAC=mac_name"};
+
   auto Symbols = Header.headerSymbols();
   auto AST = Header.build();
 
   SymbolQualitySignals Quality;
   Quality.merge(findSymbol(Symbols, "_X"));
   EXPECT_FALSE(Quality.Deprecated);
+  EXPECT_FALSE(Quality.ImplementationDetail);
   EXPECT_TRUE(Quality.ReservedName);
   EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
 
+  Quality.merge(findSymbol(Symbols, "X_Y_Decl"));
+  EXPECT_TRUE(Quality.ImplementationDetail);
+
+  Quality.ImplementationDetail = false;
+  Quality.merge(
+      CodeCompletionResult(&findDecl(AST, "mac_name"), /*Priority=*/42));
+  EXPECT_TRUE(Quality.ImplementationDetail);
+
   Symbol F = findSymbol(Symbols, "_f");
   F.References = 24; // TestTU doesn't count references, so fake it.
   Quality = {};
@@ -182,6 +199,10 @@ TEST(QualityTests, SymbolQualitySignalsSanity) {
   ReservedName.ReservedName = true;
   EXPECT_LT(ReservedName.evaluate(), Default.evaluate());
 
+  SymbolQualitySignals ImplementationDetail;
+  ImplementationDetail.ImplementationDetail = true;
+  EXPECT_LT(ImplementationDetail.evaluate(), Default.evaluate());
+
   SymbolQualitySignals WithReferences, ManyReferences;
   WithReferences.References = 20;
   ManyReferences.References = 1000;

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

@@ -83,6 +83,9 @@ MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
          IsIndexedForCodeCompletion;
 }
 MATCHER(Deprecated, "") { return arg.Flags & Symbol::Deprecated; }
+MATCHER(ImplementationDetail, "") {
+  return arg.Flags & Symbol::ImplementationDetail;
+}
 MATCHER(RefRange, "") {
   const Ref &Pos = testing::get<0>(arg);
   const Range &Range = testing::get<1>(arg);
@@ -1037,6 +1040,21 @@ TEST_F(SymbolCollectorTest, DeprecatedSymbols) {
                            AllOf(QName("TestClangd"), Not(Deprecated()))));
 }
 
+TEST_F(SymbolCollectorTest, ImplementationDetail) {
+  const std::string Header = R"(
+    #define DECL_NAME(x, y) x##_##y##_Decl
+    #define DECL(x, y) class DECL_NAME(x, y) {};
+    DECL(X, Y); // X_Y_Decl
+
+    class Public {};
+  )";
+  runSymbolCollector(Header, /**/ "");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("X_Y_Decl"), ImplementationDetail()),
+                  AllOf(QName("Public"), Not(ImplementationDetail()))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang