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

[ioeric]

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.

[sammccall]

Idea looks good, I think it needs some renames and index support.

clangd/AST.h

33

nit: should start with lowercase: isSpelledInSourceCode?

clangd/Quality.cpp

182

This doesn't match the current definition of ReservedName.
I'd suggest either:

• adding a new boolean (ImplementationDetail? maybe we'll add other heuristics) and treating this as independent of reserved-ness
• renaming the current ReservedName flag to cover this expanded scope (again, ImplementationDetail is a reasonable name)

192

The new ReservedName cases don't survive a round-trip through the index (unlike the existing ones, which get recomputed from the name).

I think you want to add a new flag bit to Symbol, set it in SymbolCollector, and read it here. (IIRC new flags in the Flags field are handled transparently by yaml and binary serialization).

[ioeric]

• address comments

[ioeric]

clangd/Quality.cpp

182

ImplementationDetail sounds great.

192

Yeah, I wasn't sure what the name to use here and wanted to get a a second opinion on adding the new flag. Thanks for the name!

[sammccall]

clangd/Quality.cpp

181

We could consider putting this in a one-liner function in AST.h or so, in case we want to add more heuristics - so SymbolCollector and Quality remain consistent. Up to you.

[ioeric]

• add isImplementationDetail helper

clangd/AST.h

 #include "index/Index.h"
 
 namespace clang {
 class SourceManager;
 class Decl;
 
 namespace clangd {
 
+// 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 SpelledInSourceCode(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
 /// decl occurs in the code.
 SourceLocation findNameLoc(const clang::Decl *D);
 
 /// Returns the qualified name of ND. The scope doesn't contain unwritten scopes
 /// like inline namespaces.

clangd/AST.cpp

 //===--- AST.cpp - Utility AST functions  -----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 
 #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"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
-SourceLocation findNameLoc(const clang::Decl* D) {
-  const auto& SM = D->getASTContext().getSourceManager();
+bool SpelledInSourceCode(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();
-    }
-  }
-  return SpellingLoc;
+        llvm::StringRef(PrintLoc).startswith("<command line>"))
+      return false;
+  return true;
+
+SourceLocation findNameLoc(const clang::Decl* D) {
+  const auto &SM = D->getASTContext().getSourceManager();
+  if (!SpelledInSourceCode(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) {
   std::string QName;
   llvm::raw_string_ostream OS(QName);
   PrintingPolicy Policy(ND.getASTContext().getLangOpts());
   // Note that inline namespaces are treated as transparent scopes. This
   // reflects the way they're most commonly used for lookup. Ideally we'd

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 "AST.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"
static bool isInstanceMember(const index::SymbolInfo &D) {
   }
 }
 
 void SymbolQualitySignals::merge(const CodeCompletionResult &SemaCCResult) {
   Deprecated |= (SemaCCResult.Availability == CXAvailability_Deprecated);
   Category = categorize(SemaCCResult);
 
   if (SemaCCResult.Declaration) {
     if (auto *ID = SemaCCResult.Declaration->getIdentifier())
-      ReservedName = ReservedName || isReserved(ID->getName());
+      ReservedName = ReservedName || isReserved(ID->getName()) ||
+                     !SpelledInSourceCode(SemaCCResult.Declaration);
   } else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
     ReservedName = ReservedName || isReserved(SemaCCResult.Macro->getName());
 }
 
 void SymbolQualitySignals::merge(const Symbol &IndexResult) {
   Deprecated |= (IndexResult.Flags & Symbol::Deprecated);
   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 >= 10) {

unittests/clangd/QualityTests.cpp

 namespace {
 
 TEST(QualityTests, SymbolQualitySignalExtraction) {
   auto Header = TestTU::withHeaderCode(R"cpp(
     int _X;
 
     [[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_TRUE(Quality.ReservedName);
   EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
 
+  Quality.ReservedName = false;
+  Quality.merge(
+      CodeCompletionResult(&findDecl(AST, "X_Y_Decl"), /*Priority=*/42));
+  EXPECT_TRUE(Quality.ReservedName);
+
+  Quality.ReservedName = false;
+  Quality.merge(
+      CodeCompletionResult(&findDecl(AST, "mac_name"), /*Priority=*/42));
+  EXPECT_TRUE(Quality.ReservedName);
+
   Symbol F = findSymbol(Symbols, "_f");
   F.References = 24; // TestTU doesn't count references, so fake it.
   Quality = {};
   Quality.merge(F);
   EXPECT_TRUE(Quality.Deprecated);
   EXPECT_FALSE(Quality.ReservedName);
   EXPECT_EQ(Quality.References, 24u);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);