[clangd] Expose 'shouldCollectSymbol' helper from SymbolCollector.

[ioeric]

Summary

This allows tools to examine symbols that would be collected in a symbol index. For example, a tool that measures index-based completion quality would be interested in references to symbols that are collected in the index.

[sammccall]

Can you motivate this change a bit, and add tests?

clangd/index/SymbolCollector.h

61 ↗

(On Diff #152241)

in principle, this should take const Options&?

[ioeric]

• added tests.

[ioeric]

In D48418#1138984, @sammccall wrote:

Can you motivate this change a bit, and add tests?

Done.

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

 /// - Anonymous declarations (anonymous enum/class/struct, etc)
 /// - Declarations in anonymous namespaces
 /// - Local declarations (in function bodies, blocks, etc)
 /// - Declarations in main files
 /// - Template specializations
 /// - Library-specific private declarations (e.g. private declaration generated
 /// by protobuf compiler)
 ///
-/// See also shouldFilterDecl().
+/// See also shouldCollectSymbol(...).
 ///
 /// Clients (e.g. clangd) can use SymbolCollector together with
 /// index::indexTopLevelDecls to retrieve all symbols when the source file is
 /// changed.
 class SymbolCollector : public index::IndexDataConsumer {
 public:
   struct Options {
     /// When symbol paths cannot be resolved to absolute paths (e.g. files in
struct Options {
     /// different #include path.
     const CanonicalIncludes *Includes = nullptr;
     // Populate the Symbol.References field.
     bool CountReferences = false;
   };
 
   SymbolCollector(Options Opts);
 
+  /// Returns true is \p ND should be collected.
+  /// AST matchers require non-const ASTContext.
+  static bool shouldCollectSymbol(const NamedDecl &ND, ASTContext &ASTCtx,
+                                  const Options &Opts);
+
   void initialize(ASTContext &Ctx) override;
 
   void setPreprocessor(std::shared_ptr<Preprocessor> PP) override {
     this->PP = std::move(PP);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,

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

bool isPrivateProtoDecl(const NamedDecl &ND) {
   // indexed. Outer_INNER is private. This heuristic relies on naming style, it
   // will include OUTER_INNER and exclude some_enum_constant.
   // FIXME: the heuristic relies on naming style (i.e. no underscore in
   // user-defined names) and can be improved.
   return (ND.getKind() != Decl::EnumConstant) ||
          std::any_of(Name.begin(), Name.end(), islower);
 }
 
-bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
-                      const SymbolCollector::Options &Opts) {
-  using namespace clang::ast_matchers;
-  if (ND->isImplicit())
-    return true;
-  // Skip anonymous declarations, e.g (anonymous enum/class/struct).
-  if (ND->getDeclName().isEmpty())
-    return true;
-
-  // FIXME: figure out a way to handle internal linkage symbols (e.g. static
-  // variables, function) defined in the .cc files. Also we skip the symbols
-  // in anonymous namespace as the qualifier names of these symbols are like
-  // `foo::<anonymous>::bar`, which need a special handling.
-  // In real world projects, we have a relatively large set of header files
-  // that define static variables (like "static const int A = 1;"), we still
-  // want to collect these symbols, although they cause potential ODR
-  // violations.
-  if (ND->isInAnonymousNamespace())
-    return true;
-
-  // We want most things but not "local" symbols such as symbols inside
-  // FunctionDecl, BlockDecl, ObjCMethodDecl and OMPDeclareReductionDecl.
-  // FIXME: Need a matcher for ExportDecl in order to include symbols declared
-  // within an export.
-  auto InNonLocalContext = hasDeclContext(anyOf(
-      translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
-      enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
-      objcCategoryImplDecl(), objcImplementationDecl()));
-  // Don't index template specializations and expansions in main files.
-  auto IsSpecialization =
-      anyOf(functionDecl(isExplicitTemplateSpecialization()),
-            cxxRecordDecl(isExplicitTemplateSpecialization()),
-            varDecl(isExplicitTemplateSpecialization()));
-  if (match(decl(allOf(unless(isExpansionInMainFile()), InNonLocalContext,
-                       unless(IsSpecialization))),
-            *ND, *ASTCtx)
-          .empty())
-    return true;
-
-  // Avoid indexing internal symbols in protobuf generated headers.
-  if (isPrivateProtoDecl(*ND))
-    return true;
-  return false;
-}
-
 // We only collect #include paths for symbols that are suitable for global code
 // completion, except for namespaces since #include path for a namespace is hard
 // to define.
 bool shouldCollectIncludePath(index::SymbolKind Kind) {
   using SK = index::SymbolKind;
   switch (Kind) {
   case SK::Macro:
   case SK::Enum:

 
 void SymbolCollector::initialize(ASTContext &Ctx) {
   ASTCtx = &Ctx;
   CompletionAllocator = std::make_shared<GlobalCodeCompletionAllocator>();
   CompletionTUInfo =
       llvm::make_unique<CodeCompletionTUInfo>(CompletionAllocator);
 }
 
+bool SymbolCollector::shouldCollectSymbol(const NamedDecl &ND,
+                                          ASTContext &ASTCtx,
+                                          const Options &Opts) {
+  using namespace clang::ast_matchers;
+  if (ND.isImplicit())
+    return false;
+  // Skip anonymous declarations, e.g (anonymous enum/class/struct).
+  if (ND.getDeclName().isEmpty())
+    return false;
+
+  // FIXME: figure out a way to handle internal linkage symbols (e.g. static
+  // variables, function) defined in the .cc files. Also we skip the symbols
+  // in anonymous namespace as the qualifier names of these symbols are like
+  // `foo::<anonymous>::bar`, which need a special handling.
+  // In real world projects, we have a relatively large set of header files
+  // that define static variables (like "static const int A = 1;"), we still
+  // want to collect these symbols, although they cause potential ODR
+  // violations.
+  if (ND.isInAnonymousNamespace())
+    return false;
+
+  // We want most things but not "local" symbols such as symbols inside
+  // FunctionDecl, BlockDecl, ObjCMethodDecl and OMPDeclareReductionDecl.
+  // FIXME: Need a matcher for ExportDecl in order to include symbols declared
+  // within an export.
+  auto InNonLocalContext = hasDeclContext(anyOf(
+      translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+      enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+      objcCategoryImplDecl(), objcImplementationDecl()));
+  // Don't index template specializations and expansions in main files.
+  auto IsSpecialization =
+      anyOf(functionDecl(isExplicitTemplateSpecialization()),
+            cxxRecordDecl(isExplicitTemplateSpecialization()),
+            varDecl(isExplicitTemplateSpecialization()));
+  if (match(decl(allOf(unless(isExpansionInMainFile()), InNonLocalContext,
+                       unless(IsSpecialization))),
+            ND, ASTCtx)
+          .empty())
+    return false;
+
+  // Avoid indexing internal symbols in protobuf generated headers.
+  if (isPrivateProtoDecl(ND))
+    return false;
+  return true;
+}
+
 // Always return true to continue indexing.
 bool SymbolCollector::handleDeclOccurence(
     const Decl *D, index::SymbolRoleSet Roles,
     ArrayRef<index::SymbolRelation> Relations, SourceLocation Loc,
     index::IndexDataConsumer::ASTNodeInfo ASTNode) {
   assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
   assert(CompletionAllocator && CompletionTUInfo);
   assert(ASTNode.OrigD);
if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
       SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   // Don't continue indexing if this is a mere reference.
   if (!(Roles & static_cast<unsigned>(index::SymbolRole::Declaration) ||
         Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
-  if (shouldFilterDecl(ND, ASTCtx, Opts))
+  if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
     return true;
 
   llvm::SmallString<128> USR;
   if (index::generateUSRForDecl(ND, USR))
     return true;
   SymbolID ID(USR);
 
   const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);

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

 //===-- SymbolCollectorTests.cpp  -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "TestFS.h"
+#include "TestTU.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/Tooling.h"

 MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
   return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
 }
 
 namespace clang {
 namespace clangd {
 
 namespace {
+
+class ShouldCollectSymbolTest : public ::testing::Test {
+public:
+  void build(StringRef HeaderCode, StringRef Code = "") {
+    File.HeaderFilename = HeaderName;
+    File.Filename = FileName;
+    File.HeaderCode = HeaderCode;
+    File.Code = Code;
+    AST = File.build();
+  }
+
+  // build() must have been called.
+  bool shouldCollect(StringRef Name, bool Qualified = true) {
+    assert(AST.hasValue());
+    return SymbolCollector::shouldCollectSymbol(
+        Qualified ? findDecl(*AST, Name) : findAnyDecl(*AST, Name),
+        AST->getASTContext(), SymbolCollector::Options());
+  }
+
+protected:
+  std::string HeaderName = "f.h";
+  std::string FileName = "f.cpp";
+  TestTU File;
+  Optional<ParsedAST> AST;  // Initialized after build.
+};
+
+TEST_F(ShouldCollectSymbolTest, ShouldCollectSymbol) {
+  build(R"(
+    namespace nx {
+    class X{}
+    void f() { int Local; }
+    struct { int x } var;
+    namespace { class InAnonymous {}; }
+    }
+  )",
+        "class InMain {};");
+  auto AST = File.build();
+  EXPECT_TRUE(shouldCollect("nx"));
+  EXPECT_TRUE(shouldCollect("nx::X"));
+  EXPECT_TRUE(shouldCollect("nx::f"));
+
+  EXPECT_FALSE(shouldCollect("InMain"));
+  EXPECT_FALSE(shouldCollect("Local", /*Qualified=*/false));
+  EXPECT_FALSE(shouldCollect("InAnonymous", /*Qualified=*/false));
+}
+
+TEST_F(ShouldCollectSymbolTest, NoPrivateProtoSymbol) {
+  HeaderName = "f.proto.h";
+  build(
+      R"(// Generated by the protocol buffer compiler.  DO NOT EDIT!
+         namespace nx {
+           class Top_Level {};
+           class TopLevel {};
+           enum Kind {
+             KIND_OK,
+             Kind_Not_Ok,
+           };
+         })");
+  EXPECT_TRUE(shouldCollect("nx::TopLevel"));
+  EXPECT_TRUE(shouldCollect("nx::Kind::KIND_OK"));
+  EXPECT_TRUE(shouldCollect("nx::Kind"));
+
+  EXPECT_FALSE(shouldCollect("nx::Top_Level"));
+  EXPECT_FALSE(shouldCollect("nx::Kind::Kind_Not_Ok"));
+}
+
+TEST_F(ShouldCollectSymbolTest, DoubleCheckProtoHeaderComment) {
+  HeaderName = "f.proto.h";
+  build(R"(
+    namespace nx {
+      class Top_Level {};
+      enum Kind {
+        Kind_Fine
+      };
+    }
+  )");
+  EXPECT_TRUE(shouldCollect("nx::Top_Level"));
+  EXPECT_TRUE(shouldCollect("nx::Kind_Fine"));
+}
+
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts,
                            CommentHandler *PragmaHandler)
       : COpts(std::move(COpts)), PragmaHandler(PragmaHandler) {}
 
   clang::FrontendAction *create() override {
     class WrappedIndexAction : public WrapperFrontendAction {

 TEST_F(SymbolCollectorTest, UTF16Character) {
   // ö is 2-bytes.
   Annotations Header(/*Header=*/"class [[pörk]] {};");
   runSymbolCollector(Header.code(), /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("pörk"), DeclRange(Header.range()))));
 }
 
-TEST_F(SymbolCollectorTest, FilterPrivateProtoSymbols) {
-  TestHeaderName = testPath("x.proto.h");
-  const std::string Header =
-      R"(// Generated by the protocol buffer compiler.  DO NOT EDIT!
-         namespace nx {
-           class Top_Level {};
-           class TopLevel {};
-           enum Kind {
-             KIND_OK,
-             Kind_Not_Ok,
-           };
-           bool operator<(const TopLevel &, const TopLevel &);
-         })";
-  runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
-                                   QName("nx::Kind"), QName("nx::KIND_OK"),
-                                   QName("nx::operator<")));
-}
-
-TEST_F(SymbolCollectorTest, DoubleCheckProtoHeaderComment) {
-  TestHeaderName = testPath("x.proto.h");
-  const std::string Header = R"(
-  namespace nx {
-    class Top_Level {};
-    enum Kind {
-      Kind_Fine
-    };
-  }
-  )";
-  runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
-                                   QName("nx::Kind"), QName("nx::Kind_Fine")));
-}
-
 TEST_F(SymbolCollectorTest, DoNotIndexSymbolsInFriendDecl) {
   Annotations Header(R"(
     namespace nx {
       class $z[[Z]] {};
       class X {
         friend class Y;
         friend class Z;
         friend void foo();