[clangd] Add more filters for collected symbols.

[ioeric]

Summary

o We only collect symbols in namespace or translation unit scopes.
o Add an option to only collect symbols in included headers.

[hokein]

With this patch, the index-based code completion will not provide any candidates for ns1::MyClass::^ (as the index-based completion drops all results from clang's completion engine), we need a way to combine symbols from 3 sources (static index, dynamic index and clang's completion engine).

clangd/index/SymbolCollector.cpp

80 ↗

(On Diff #128938)

These ast matchers seems to be relatively simple, maybe we can use the Decl methods to implement them at the moment. Or will we add more complex filters in the future?

116 ↗

(On Diff #128938)

How about moving the this function inside the if below? so we don't duplicate the dyn_cast twice.

clangd/index/SymbolCollector.h

28 ↗

(On Diff #128938)

We need documentation for the options.

[ilya-biryukov]

In D41823#970725, @hokein wrote:

With this patch, the index-based code completion will not provide any candidates for ns1::MyClass::^ (as the index-based completion drops all results from clang's completion engine), we need a way to combine symbols from 3 sources (static index, dynamic index and clang's completion engine).

Given that index-based completion is still experimental I think it's still fine to address this in a follow-up change. (In order to keep both changes simpler and more focused).

clangd/index/FileIndex.cpp

22 ↗

(On Diff #128938)

NIT: why not SymbolCollector::Options CollectorOpts? It's seems more concise.

Or even SymbolCollector::Options{/*IndexMainFiles=*/true};
The last one does not play nicely with moving the fields of the struct around, though

clangd/index/SymbolCollector.cpp

80 ↗

(On Diff #128938)

I second this. Also the code would probably be more readable without matchers in this particular example.

86 ↗

(On Diff #128938)

This should probably include changes from @hokein 's patch D41759.

clangd/index/SymbolCollector.h

28 ↗

(On Diff #128938)

Also, would naming it OnlyMainFiles make the intent of the option clearer?

unittests/clangd/FileIndexTests.cpp

173 ↗

(On Diff #128938)

Why do we need to remove static int m1?
Perhaps simply adding void f() is enough?

unittests/clangd/SymbolCollectorTests.cpp

107 ↗

(On Diff #128938)

Why do we remove void f()? Maybe assert that it's not in the list of symbols instead?

182 ↗

(On Diff #128938)

Why should we change this test?

[ioeric]

• Addressed comments.
• Merged with origin/master
• Merge with diff base.

[ioeric]

Thanks for the reviews!

clangd/index/FileIndex.cpp

22 ↗

(On Diff #128938)

I think I had too much shared/unique pointers... :P

The last one does not play nicely with moving the fields of the struct around, though

Yeah, this is the reason I didn't use initializer.

clangd/index/SymbolCollector.cpp

80 ↗

(On Diff #128938)

Yes, I expect to borrow more matchers from include-fixer's find-all-symbols. Also, although isExpansionInMainFile seems easy to implement, I am inclined to reuse the existing code if possible.

clangd/index/SymbolCollector.h

28 ↗

(On Diff #128938)

Actually, OnlyMainFiles is the opposite. We always index header files but optionally index main files. I've added comment to clarify...

unittests/clangd/SymbolCollectorTests.cpp

107 ↗

(On Diff #128938)

I was trying to limit the scope of each test case (there is a separate test case IgnoreClassMembers). UnorderedElementsAre should be able to capture unexpected symbols.

182 ↗

(On Diff #128938)

By default, main files are not indexed, so I put the code in header.

[ioeric]

• Rebase on origin/master

[hokein]

LGTM

clangd/index/SymbolCollector.h

20 ↗

(On Diff #129112)

We should also document what kinds of symbols we are collecting, since this patch has changed the behavior (only collect top-level symbols).

unittests/clangd/SymbolCollectorTests.cpp

165 ↗

(On Diff #129112)

nit: although the default value is false, I'd explicitly specify CollectorOpts.IndexMainFiles = false; (readers don't need to jump to another file to see the default value).

[ilya-biryukov]

LGTM. I'd add a comment pointing to code in include-fixer we're borrowing, though. (See the relevant comment)

clangd/index/SymbolCollector.cpp

80 ↗

(On Diff #128938)

Could you add a comment pointing to the code in include fixer, so that everyone reading the code knows where the code comes from?
(It's generally useful in case there are bugs and borrowing more code from include-fixer fixes that)

clangd/index/SymbolCollector.h

28 ↗

(On Diff #128938)

The comment looks good, thanks!

unittests/clangd/SymbolCollectorTests.cpp

107 ↗

(On Diff #128938)

Oh, sorry, I missed that the name of the test changed too.
IMO, generally, adding new tests in seems like a better option if the original tests aren't broken.

182 ↗

(On Diff #128938)

But we still get the symbols from main file in clangd, because we're asking to index main files in indexAST...
Makes sense. Thanks for clarifying.

[ioeric]

• Merge branch 'master' of http://llvm.org/git/clang-tools-extra into collector
• Address review comments.

clangd/index/SymbolCollector.cpp

80 ↗

(On Diff #128938)

Actually, I'd say we are borrowing ideas of filtering instead of the actual code from include-fixer, and the code behavior is also quite different, so it's probably not worth adding pointers to another project in the code.

(But as a reference for us, the include-fixer code is .../extra/include-fixer/find-all-symbols/FindAllSymbols.cpp:119 :)

[ilya-biryukov]

clangd/index/SymbolCollector.cpp

80 ↗

(On Diff #128938)

I thought we were borrowing code after reading this comment:

I expect to borrow more matchers from include-fixer's find-all-symbols.

Thanks for the reference, I'd say it's still useful to have a reference in the code:

// The code is similar to include fixer, see include-fixer/find-all-symbols/FindAllSymbols.cpp for more details.

But it's up to you, of course.

clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

private:
       tooling::ExecutionContext *Ctx;
       std::shared_ptr<SymbolCollector> Collector;
     };
 
     index::IndexingOptions IndexOpts;
     IndexOpts.SystemSymbolFilter =
         index::IndexingOptions::SystemSymbolFilterKind::All;
     IndexOpts.IndexFunctionLocals = false;
-    return new WrappedIndexAction(std::make_shared<SymbolCollector>(),
-                                  IndexOpts, Ctx);
+    return new WrappedIndexAction(
+        std::make_shared<SymbolCollector>(SymbolCollector::Options()),
   }
 
   tooling::ExecutionContext *Ctx;
 };
 
 } // namespace clangd
 } // namespace clang
 

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

 namespace clang {
 namespace clangd {
 namespace {
 
 /// Retrieves namespace and class level symbols in \p Decls.
 std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx,
                                      std::shared_ptr<Preprocessor> PP,
                                      llvm::ArrayRef<const Decl *> Decls) {
-  auto Collector = std::make_shared<SymbolCollector>();
+  SymbolCollector::Options CollectorOpts;
+  CollectorOpts.IndexMainFiles = true;
+  auto Collector = std::make_shared<SymbolCollector>(std::move(CollectorOpts));
   Collector->setPreprocessor(std::move(PP));
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = false;
 
   index::indexTopLevelDecls(Ctx, Decls, Collector, IndexOpts);
   auto Symbols = llvm::make_unique<SymbolSlab>();

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

 #include "clang/AST/Decl.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 
 namespace clang {
 namespace clangd {
 
-// Collect all symbols from an AST.
-//
-// Clients (e.g. clangd) can use SymbolCollector together with
-// index::indexTopLevelDecls to retrieve all symbols when the source file is
-// changed.
+/// \brief Collect top-level symbols from an AST. These are symbols defined
+/// immediately inside a namespace or a translation unit scope. For example,
+/// symbols in classes or functions are not collected.
+///
+/// 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 {
+    /// Whether to collect symbols in main files (e.g. the source file
+    /// corresponding to a TU).
+    bool IndexMainFiles = false;
+  };
+
+  SymbolCollector(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,
                       ArrayRef<index::SymbolRelation> Relations, FileID FID,
                       unsigned Offset,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols).build(); }
 
 private:
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   ASTContext *ASTCtx;
   std::shared_ptr<Preprocessor> PP;
   std::shared_ptr<GlobalCodeCompletionAllocator> CompletionAllocator;
   std::unique_ptr<CodeCompletionTUInfo> CompletionTUInfo;
+  Options Opts;
 };
 
 } // namespace clangd
 } // namespace clang

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

 //===--- SymbolCollector.cpp -------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {

 splitQualifiedName(llvm::StringRef QName) {
   assert(!QName.startswith("::") && "Qualified names should not start with ::");
   size_t Pos = QName.rfind("::");
   if (Pos == llvm::StringRef::npos)
     return {StringRef(), QName};
   return {QName.substr(0, Pos), QName.substr(Pos + 2)};
 }
 
+bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
+                      const SymbolCollector::Options &Opts) {
+  using namespace clang::ast_matchers;
+  if (ND->isImplicit())
+    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 only want symbols in namespaces or translation unit scopes (e.g. no
+  // class members).
+  if (match(decl(allOf(
+                Opts.IndexMainFiles ? decl()
+                                    : decl(unless(isExpansionInMainFile())),
+                hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())))),
+            *ND, *ASTCtx)
+          .empty())
+    return true;
+
+  return false;
+}
+
 } // namespace
 
+SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
+
 void SymbolCollector::initialize(ASTContext &Ctx) {
   ASTCtx = &Ctx;
   CompletionAllocator = std::make_shared<GlobalCodeCompletionAllocator>();
   CompletionTUInfo =
       llvm::make_unique<CodeCompletionTUInfo>(CompletionAllocator);
 }
 
 // Always return true to continue indexing.
 bool SymbolCollector::handleDeclOccurence(
     const Decl *D, index::SymbolRoleSet Roles,
     ArrayRef<index::SymbolRelation> Relations, FileID FID, unsigned Offset,
     index::IndexDataConsumer::ASTNodeInfo ASTNode) {
+  assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
+
   // FIXME: collect all symbol references.
   if (!(Roles & static_cast<unsigned>(index::SymbolRole::Declaration) ||
         Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
 
   assert(CompletionAllocator && CompletionTUInfo);
 
   if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
-    // 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())
+    if (shouldFilterDecl(ND, ASTCtx, Opts))
       return true;
-
     llvm::SmallString<128> USR;
     if (index::generateUSRForDecl(ND, USR))
       return true;
 
     auto ID = SymbolID(USR);
     if (Symbols.find(ID) != nullptr)
       return true;
 

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

 
 TEST(FileIndexTest, RemoveNonExisting) {
   FileIndex M;
   auto Ctx = Context::empty();
   M.update(Ctx, "no", nullptr);
   EXPECT_THAT(match(M, FuzzyFindRequest()), UnorderedElementsAre());
 }
 
-TEST(FileIndexTest, ClassMembers) {
+TEST(FileIndexTest, IgnoreClassMembers) {
   FileIndex M;
   auto Ctx = Context::empty();
   M.update(Ctx, "f1",
-           build("f1", "class X { static int m1; int m2;};").getPointer());
+           build("f1", "class X { static int m1; int m2; static void f(); };")
+               .getPointer());
 
   FuzzyFindRequest Req;
   Req.Query = "";
-  EXPECT_THAT(match(M, Req), UnorderedElementsAre("X", "X::m1", "X::m2"));
+  EXPECT_THAT(match(M, Req), UnorderedElementsAre("X"));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 }
 
 namespace clang {
 namespace clangd {
 
 namespace {
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
-  SymbolIndexActionFactory() = default;
+  SymbolIndexActionFactory(SymbolCollector::Options COpts)
+      : COpts(std::move(COpts)) {}
 
   clang::FrontendAction *create() override {
     index::IndexingOptions IndexOpts;
     IndexOpts.SystemSymbolFilter =
         index::IndexingOptions::SystemSymbolFilterKind::All;
     IndexOpts.IndexFunctionLocals = false;
-    Collector = std::make_shared<SymbolCollector>();
+    Collector = std::make_shared<SymbolCollector>(COpts);
     FrontendAction *Action =
         index::createIndexingAction(Collector, IndexOpts, nullptr).release();
     return Action;
   }
 
   std::shared_ptr<SymbolCollector> Collector;
+  SymbolCollector::Options COpts;
 };
 
 class SymbolCollectorTest : public ::testing::Test {
 public:
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
     llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
         new vfs::InMemoryFileSystem);
     llvm::IntrusiveRefCntPtr<FileManager> Files(
         new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
     const std::string FileName = "symbol.cc";
     const std::string HeaderName = "symbols.h";
-    auto Factory = llvm::make_unique<SymbolIndexActionFactory>();
+    auto Factory = llvm::make_unique<SymbolIndexActionFactory>(CollectorOpts);
 
     tooling::ToolInvocation Invocation(
         {"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
         Factory->create(), Files.get(),
         std::make_shared<PCHContainerOperations>());
 
     InMemoryFileSystem->addFile(HeaderName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
     std::string Content = "#include\"" + std::string(HeaderName) + "\"";
     Content += "\n" + MainCode.str();
     InMemoryFileSystem->addFile(FileName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(Content));
     Invocation.run();
     Symbols = Factory->Collector->takeSymbols();
     return true;
   }
 
 protected:
   SymbolSlab Symbols;
+  SymbolCollector::Options CollectorOpts;
 };
 
-TEST_F(SymbolCollectorTest, CollectSymbol) {
+TEST_F(SymbolCollectorTest, CollectSymbols) {
+  CollectorOpts.IndexMainFiles = true;
   const std::string Header = R"(
     class Foo {
       void f();
     };
     void f1();
     inline void f2() {}
     static const int KInt = 2;
     const char* kStr = "123";
const std::string Main = R"(
     // and there is no way to customize this behavior at the moment.
     using bar::v2;
     } // namespace foo
   )";
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
               UnorderedElementsAreArray(
                   {QName("Foo"),
-                   QName("Foo::f"),
                    QName("f1"),
                    QName("f2"),
                    QName("KInt"),
                    QName("kStr"),
                    QName("foo"),
                    QName("foo::bar"),
                    QName("foo::int32"),
                    QName("foo::int32_t"),
                    QName("foo::v1"),
                    QName("foo::bar::v2"),
                    QName("foo::baz")}));
 }
 
-TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
+TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
+  CollectorOpts.IndexMainFiles = false;
+  const std::string Header = R"(
+    class Foo {};
+    void f1();
+    inline void f2() {}
+  )";
   const std::string Main = R"(
+    namespace {
+    void ff() {} // ignore
+    }
+    void main_f() {} // ignore
+    void f1() {}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2")));
+}
+
+TEST_F(SymbolCollectorTest, IncludeSymbolsInMainFile) {
+  CollectorOpts.IndexMainFiles = true;
+  const std::string Header = R"(
+    class Foo {};
+    void f1();
+    inline void f2() {}
+  )";
+  const std::string Main = R"(
+    namespace {
+    void ff() {} // ignore
+    }
+    void main_f() {}
+    void f1() {}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("f1"),
+                                            QName("f2"), QName("main_f")));
+}
+
+TEST_F(SymbolCollectorTest, IgnoreClassMembers) {
+  const std::string Header = R"(
+    class Foo {
+      void f() {}
+      void g();
+      static void sf() {}
+      static void ssf();
+      static int x;
+    };
+  )";
+  const std::string Main = R"(
+    void Foo::g() {}
+    void Foo::ssf() {}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
+}
+
+TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
+  const std::string Header = R"(
     namespace nx {
     /// Foo comment.
     int ff(int x, double y) { return 0; }
     }
   )";
-  runSymbolCollector(/*Header=*/"", Main);
+  runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(QName("nx"),
                                    AllOf(QName("nx::ff"),
                                          Labeled("ff(int x, double y)"),
                                          Detail("int"), Doc("Foo comment."))));
 }
 
 TEST_F(SymbolCollectorTest, PlainAndSnippet) {
-  const std::string Main = R"(
+  const std::string Header = R"(
     namespace nx {
     void f() {}
     int ff(int x, double y) { return 0; }
     }
   )";
-  runSymbolCollector(/*Header=*/"", Main);
+  runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(
       Symbols,
       UnorderedElementsAre(
           QName("nx"),
           AllOf(QName("nx::f"), Labeled("f()"), Plain("f"), Snippet("f()")),
           AllOf(QName("nx::ff"), Labeled("ff(int x, double y)"), Plain("ff"),
                 Snippet("ff(${1:int x}, ${2:double y})"))));
 }