[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 ↗

(On Diff #170006)

nit: should start with lowercase: isSpelledInSourceCode?

clangd/Quality.cpp

182 ↗

(On Diff #170006)

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 ↗

(On Diff #170006)

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 ↗

(On Diff #170006)

ImplementationDetail sounds great.

192 ↗

(On Diff #170006)

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 ↗

(On Diff #170046)

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

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

 #include "index/Index.h"
 
 namespace clang {
 class SourceManager;
 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
 /// 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.

clang-tools-extra/trunk/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();
-  // 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 (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;
+// Returns true if the complete name of decl \p D is spelled in the source code.
+// This is not the case for
+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.
+  if (Loc.isMacroID()) {
+    std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
+    if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
+        llvm::StringRef(PrintLoc).startswith("<command line>"))
+      return false;
+  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) {
   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

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

 // 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 _.
+  bool ImplementationDetail = false;
   unsigned References = 0;
 
   enum SymbolCategory {
     Unknown = 0,
     Variable,
     Macro,
     Type,
     Function,

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 "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) {
+    ImplementationDetail |= isImplementationDetail(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) {
   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);
 }
 
 float SymbolQualitySignals::evaluate() const {
   float Score = 1;
 
if (References >= 10) {
     float S = std::pow(References, -0.06);
     Score *= 6.0 * (1 - S) / (1 + S) + 0.59;
   }
 
   if (Deprecated)
     Score *= 0.1f;
   if (ReservedName)
     Score *= 0.1f;
+  if (ImplementationDetail)
+    Score *= 0.2f;
 
   switch (Category) {
   case Keyword: // Often relevant, but misses most signals.
     Score *= 4; // FIXME: important keywords should have specific boosts.
     break;
   case Type:
   case Function:
   case Variable:

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

struct Symbol {
 
   enum SymbolFlag : uint8_t {
     None = 0,
     /// Whether or not this symbol is meant to be used for the code completion.
     /// See also isIndexedForCodeCompletion().
     IndexedForCodeCompletion = 1 << 0,
     /// Indicates if the symbol is deprecated.
     Deprecated = 1 << 1,
+    // Symbol is an implementation detail.
+    ImplementationDetail = 1 << 2,
   };
 
   SymbolFlag Flags = SymbolFlag::None;
   /// FIXME: also add deprecation message and fixit?
 };
 inline Symbol::SymbolFlag  operator|(Symbol::SymbolFlag A, Symbol::SymbolFlag  B) {
   return static_cast<Symbol::SymbolFlag>(static_cast<uint8_t>(A) |
                                          static_cast<uint8_t>(B));

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

const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
   S.ID = std::move(ID);
   std::string QName = printQualifiedName(ND);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
   // FIXME: this returns foo:bar: for objective-C methods, we prefer only foo:
   // for consistency with CodeCompletionString and a clean name/signature split.
 
   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,
                                       ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.

clang-tools-extra/trunk/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_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 = {};
   Quality.merge(F);
   EXPECT_TRUE(Quality.Deprecated);
   EXPECT_FALSE(Quality.ReservedName);
   EXPECT_EQ(Quality.References, 24u);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Function);
TEST(QualityTests, SymbolQualitySignalsSanity) {
   SymbolQualitySignals Deprecated;
   Deprecated.Deprecated = true;
   EXPECT_LT(Deprecated.evaluate(), Default.evaluate());
 
   SymbolQualitySignals ReservedName;
   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;
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
   SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
   Keyword.Category = SymbolQualitySignals::Keyword;

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

return std::make_tuple(
                          Pos.end.character);
 }
 MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
 MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
   return static_cast<bool>(arg.Flags & Symbol::IndexedForCodeCompletion) ==
          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);
   return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
                          Pos.Location.End.line(), Pos.Location.End.column()) ==
          std::make_tuple(Range.start.line, Range.start.character,
                          Range.end.line, Range.end.character);
 }
const std::string Header = R"(
     void TestClangd();
   )";
   runSymbolCollector(Header, /**/ "");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("TestClangc"), Deprecated()),
                            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