[clangd] Index main-file symbols (bug 39761)

Patch by Nathan Ridge!

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

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@351041 91177308-0d34-0410-b5e6-96231b3b80d8

6 files changed, 122 insertions, 54 deletions

diff --git a/clangd/index/Index.h b/clangd/index/Index.h
index 1ab9ecd6..3957a7f6 100644
--- a/clangd/index/Index.h
+++ b/clangd/index/Index.h
@@ -241,6 +241,8 @@ struct Symbol {
     Deprecated = 1 << 1,
     // Symbol is an implementation detail.
     ImplementationDetail = 1 << 2,
+    // Symbol is visible to other files (not e.g. a static helper function).
+    VisibleOutsideFile = 1 << 3,
   };
 
   SymbolFlag Flags = SymbolFlag::None;
diff --git a/clangd/index/SymbolCollector.cpp b/clangd/index/SymbolCollector.cpp
index 23b1776a..d3ce7712 100644
--- a/clangd/index/SymbolCollector.cpp
+++ b/clangd/index/SymbolCollector.cpp
@@ -240,22 +240,20 @@ void SymbolCollector::initialize(ASTContext &Ctx) {
 
 bool SymbolCollector::shouldCollectSymbol(const NamedDecl &ND,
                                           const ASTContext &ASTCtx,
-                                          const Options &Opts) {
+                                          const Options &Opts,
+                                          bool IsMainFileOnly) {
   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())
+  // Skip main-file symbols if we are not collecting them.
+  if (IsMainFileOnly && !Opts.CollectMainFileSymbols)
+    return false;
+
+  // Skip symbols in anonymous namespaces in header files.
+  if (!IsMainFileOnly && ND.isInAnonymousNamespace())
     return false;
 
   // We want most things but not "local" symbols such as symbols inside
@@ -285,10 +283,6 @@ bool SymbolCollector::shouldCollectSymbol(const NamedDecl &ND,
       explicitTemplateSpecialization<VarDecl>(ND))
     return false;
 
-  const auto &SM = ASTCtx.getSourceManager();
-  // Skip decls in the main file.
-  if (SM.isInMainFile(SM.getExpansionLoc(ND.getBeginLoc())))
-    return false;
   // Avoid indexing internal symbols in protobuf generated headers.
   if (isPrivateProtoDecl(ND))
     return false;
@@ -335,9 +329,15 @@ bool SymbolCollector::handleDeclOccurence(
 
   if (IsOnlyRef && !CollectRef)
     return true;
-  if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
+
+  // ND is the canonical (i.e. first) declaration. If it's in the main file,
+  // then no public declaration was visible, so assume it's main-file only.
+  bool IsMainFileOnly = SM.isWrittenInMainFile(SM.getExpansionLoc(
+    ND->getBeginLoc()));
+  if (!shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
     return true;
-  if (CollectRef && !isa<NamespaceDecl>(ND) &&
+  // Do not store references to main-file symbols.
+  if (CollectRef && !IsMainFileOnly && !isa<NamespaceDecl>(ND) &&
       (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
     DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
@@ -351,13 +351,13 @@ bool SymbolCollector::handleDeclOccurence(
   const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
   const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-    BasicSymbol = addDeclaration(*ND, std::move(*ID));
+    BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
   else if (isPreferredDeclaration(OriginalDecl, Roles))
     // If OriginalDecl is preferred, replace the existing canonical
     // declaration (e.g. a class forward declaration). There should be at most
     // one duplicate as we expect to see only one preferred declaration per
     // TU, because in practice they are definitions.
-    BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID));
+    BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID), IsMainFileOnly);
 
   if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
     addDefinition(OriginalDecl, *BasicSymbol);
@@ -506,7 +506,8 @@ void SymbolCollector::finish() {
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
-                                              SymbolID ID) {
+                                              SymbolID ID,
+                                              bool IsMainFileOnly) {
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
@@ -517,10 +518,13 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
   // 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))
+  // We collect main-file symbols, but do not use them for code completion.
+  if (!IsMainFileOnly && isIndexedForCodeCompletion(ND, Ctx))
     S.Flags |= Symbol::IndexedForCodeCompletion;
   if (isImplementationDetail(&ND))
     S.Flags |= Symbol::ImplementationDetail;
+  if (!IsMainFileOnly)
+    S.Flags |= Symbol::VisibleOutsideFile;
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   auto Loc = findNameLoc(&ND);
diff --git a/clangd/index/SymbolCollector.h b/clangd/index/SymbolCollector.h
index 01f2b0ad..157735d0 100644
--- a/clangd/index/SymbolCollector.h
+++ b/clangd/index/SymbolCollector.h
@@ -28,13 +28,14 @@ namespace clangd {
 /// It collects most declarations except:
 /// - Implicit declarations
 /// - Anonymous declarations (anonymous enum/class/struct, etc)
-/// - Declarations in anonymous namespaces
+/// - Declarations in anonymous namespaces in headers
 /// - 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)
 ///
+/// References to main-file symbols are not collected.
+///
 /// See also shouldCollectSymbol(...).
 ///
 /// Clients (e.g. clangd) can use SymbolCollector together with
@@ -72,6 +73,9 @@ public:
     /// collect macros. For example, `indexTopLevelDecls` will not index any
     /// macro even if this is true.
     bool CollectMacro = false;
+    /// Collect symbols local to main-files, such as static functions
+    /// and symbols inside an anonymous namespace.
+    bool CollectMainFileSymbols = true;
     /// If this is set, only collect symbols/references from a file if
     /// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
     std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;
@@ -81,7 +85,7 @@ public:
 
   /// Returns true is \p ND should be collected.
   static bool shouldCollectSymbol(const NamedDecl &ND, const ASTContext &ASTCtx,
-                                  const Options &Opts);
+                                  const Options &Opts, bool IsMainFileSymbol);
 
   void initialize(ASTContext &Ctx) override;
 
@@ -105,7 +109,7 @@ public:
   void finish() override;
 
 private:
-  const Symbol *addDeclaration(const NamedDecl &, SymbolID);
+  const Symbol *addDeclaration(const NamedDecl &, SymbolID, bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
 
   // All Symbols collected from the AST.
diff --git a/unittests/clangd/BackgroundIndexTests.cpp b/unittests/clangd/BackgroundIndexTests.cpp
index c66f4429..70c1e9c0 100644
--- a/unittests/clangd/BackgroundIndexTests.cpp
+++ b/unittests/clangd/BackgroundIndexTests.cpp
@@ -125,7 +125,7 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   EXPECT_THAT(
       runFuzzyFind(Idx, ""),
-      UnorderedElementsAre(Named("common"), Named("A_CC"),
+      UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
                            AllOf(Named("f_b"), Declared(), Not(Defined()))));
 
   Cmd.Filename = testPath("root/B.cc");
@@ -135,7 +135,7 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   // B_CC is dropped as we don't collect symbols from A.h in this compilation.
   EXPECT_THAT(runFuzzyFind(Idx, ""),
-              UnorderedElementsAre(Named("common"), Named("A_CC"),
+              UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
                                    AllOf(Named("f_b"), Declared(), Defined())));
 
   auto Syms = runFuzzyFind(Idx, "common");
@@ -198,7 +198,7 @@ TEST_F(BackgroundIndexTest, ShardStorageTest) {
 
   auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
   EXPECT_NE(ShardSource, nullptr);
-  EXPECT_THAT(*ShardSource->Symbols, UnorderedElementsAre());
+  EXPECT_THAT(*ShardSource->Symbols, UnorderedElementsAre(Named("g")));
   EXPECT_THAT(*ShardSource->Refs, RefsAre({FileURI("unittest:///root/A.cc")}));
 }
 
diff --git a/unittests/clangd/FindSymbolsTests.cpp b/unittests/clangd/FindSymbolsTests.cpp
index e4909103..9b631596 100644
--- a/unittests/clangd/FindSymbolsTests.cpp
+++ b/unittests/clangd/FindSymbolsTests.cpp
@@ -62,6 +62,7 @@ public:
       : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {
     // Make sure the test root directory is created.
     FSProvider.Files[testPath("unused")] = "";
+    CDB.ExtraClangFlags = {"-xc++"};
   }
 
 protected:
@@ -141,10 +142,11 @@ TEST_F(WorkspaceSymbolsTest, Unnamed) {
 
 TEST_F(WorkspaceSymbolsTest, InMainFile) {
   addFile("foo.cpp", R"cpp(
-      int test() {
-      }
+      int test() {}
+      static test2() {}
       )cpp");
-  EXPECT_THAT(getSymbols("test"), IsEmpty());
+  EXPECT_THAT(getSymbols("test"), 
+              ElementsAre(QName("test"), QName("test2")));
 }
 
 TEST_F(WorkspaceSymbolsTest, Namespaces) {
@@ -184,7 +186,7 @@ TEST_F(WorkspaceSymbolsTest, AnonymousNamespace) {
   addFile("foo.cpp", R"cpp(
       #include "foo.h"
       )cpp");
-  EXPECT_THAT(getSymbols("test"), IsEmpty());
+  EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test")));
 }
 
 TEST_F(WorkspaceSymbolsTest, MultiFile) {
diff --git a/unittests/clangd/SymbolCollectorTests.cpp b/unittests/clangd/SymbolCollectorTests.cpp
index ea84461c..6f76b175 100644
--- a/unittests/clangd/SymbolCollectorTests.cpp
+++ b/unittests/clangd/SymbolCollectorTests.cpp
@@ -87,6 +87,9 @@ MATCHER(Deprecated, "") { return arg.Flags & Symbol::Deprecated; }
 MATCHER(ImplementationDetail, "") {
   return arg.Flags & Symbol::ImplementationDetail;
 }
+MATCHER(VisibleOutsideFile, "") {
+  return static_cast<bool>(arg.Flags & Symbol::VisibleOutsideFile);
+}
 MATCHER(RefRange, "") {
   const Ref &Pos = testing::get<0>(arg);
   const Range &Range = testing::get<1>(arg);
@@ -113,9 +116,13 @@ public:
   // build() must have been called.
   bool shouldCollect(llvm::StringRef Name, bool Qualified = true) {
     assert(AST.hasValue());
+    const NamedDecl& ND = Qualified ? findDecl(*AST, Name) 
+                                    : findUnqualifiedDecl(*AST, Name);
+    ASTContext& Ctx = AST->getASTContext();
+    const SourceManager& SM = Ctx.getSourceManager();
+    bool MainFile = SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
     return SymbolCollector::shouldCollectSymbol(
-        Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name),
-        AST->getASTContext(), SymbolCollector::Options());
+        ND, Ctx, SymbolCollector::Options(), MainFile);
   }
 
 protected:
@@ -131,18 +138,22 @@ TEST_F(ShouldCollectSymbolTest, ShouldCollectSymbol) {
     class X{};
     auto f() { int Local; } // auto ensures function body is parsed.
     struct { int x; } var;
-    namespace { class InAnonymous {}; }
     }
   )",
-        "class InMain {};");
+        R"(
+    class InMain {};
+    namespace { class InAnonymous {}; }
+    static void g();
+  )");
   auto AST = File.build();
   EXPECT_TRUE(shouldCollect("nx"));
   EXPECT_TRUE(shouldCollect("nx::X"));
   EXPECT_TRUE(shouldCollect("nx::f"));
+  EXPECT_TRUE(shouldCollect("InMain"));
+  EXPECT_TRUE(shouldCollect("InAnonymous", /*Qualified=*/false));
+  EXPECT_TRUE(shouldCollect("g"));
 
-  EXPECT_FALSE(shouldCollect("InMain"));
   EXPECT_FALSE(shouldCollect("Local", /*Qualified=*/false));
-  EXPECT_FALSE(shouldCollect("InAnonymous", /*Qualified=*/false));
 }
 
 TEST_F(ShouldCollectSymbolTest, NoPrivateProtoSymbol) {
@@ -347,6 +358,35 @@ TEST_F(SymbolCollectorTest, CollectSymbols) {
                    AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
 }
 
+TEST_F(SymbolCollectorTest, FileLocal) {
+  const std::string Header = R"(
+    class Foo {};
+    namespace {
+      class Ignored {};
+    }
+    void bar();
+  )";
+  const std::string Main = R"(
+    class ForwardDecl;
+    void bar() {}
+    static void a();
+    class B {};
+    namespace {
+      void c();
+    }
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  AllOf(QName("Foo"), VisibleOutsideFile()),
+                  AllOf(QName("bar"), VisibleOutsideFile()),
+                  AllOf(QName("a"), Not(VisibleOutsideFile())),
+                  AllOf(QName("B"), Not(VisibleOutsideFile())),
+                  AllOf(QName("c"), Not(VisibleOutsideFile())),
+                  // FIXME: ForwardDecl likely *is* visible outside.
+                  AllOf(QName("ForwardDecl"), Not(VisibleOutsideFile()))));
+}
+
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
     // Template is indexed, specialization and instantiation is not.
@@ -417,7 +457,7 @@ o]]();
     void $printdef[[print]]() {}
 
     // Declared/defined in main only.
-    int Y;
+    int $ydecl[[Y]];
   )cpp");
   runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(Symbols,
@@ -429,7 +469,8 @@ o]]();
                   AllOf(QName("print"), DeclRange(Header.range("printdecl")),
                         DefRange(Main.range("printdef"))),
                   AllOf(QName("Z"), DeclRange(Header.range("zdecl"))),
-                  AllOf(QName("foo"), DeclRange(Header.range("foodecl")))));
+                  AllOf(QName("foo"), DeclRange(Header.range("foodecl"))),
+                  AllOf(QName("Y"), DeclRange(Main.range("ydecl")))));
 }
 
 TEST_F(SymbolCollectorTest, Refs) {
@@ -508,17 +549,25 @@ TEST_F(SymbolCollectorTest, References) {
     W* w2 = nullptr; // only one usage counts
     X x();
     class V;
-    V* v = nullptr; // Used, but not eligible for indexing.
     class Y{}; // definition doesn't count as a reference
+    V* v = nullptr;
     GLOBAL_Z(z); // Not a reference to Z, we don't spell the type.
   )";
   CollectorOpts.CountReferences = true;
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(AllOf(QName("W"), RefCount(1)),
-                                   AllOf(QName("X"), RefCount(1)),
-                                   AllOf(QName("Y"), RefCount(0)),
-                                   AllOf(QName("Z"), RefCount(0)), QName("y")));
+              UnorderedElementsAreArray(
+                {AllOf(QName("W"), RefCount(1)),
+                 AllOf(QName("X"), RefCount(1)),
+                 AllOf(QName("Y"), RefCount(0)),
+                 AllOf(QName("Z"), RefCount(0)), 
+                 AllOf(QName("y"), RefCount(0)),
+                 AllOf(QName("z"), RefCount(0)),
+                 AllOf(QName("x"), RefCount(0)),
+                 AllOf(QName("w"), RefCount(0)),
+                 AllOf(QName("w2"), RefCount(0)),
+                 AllOf(QName("V"), RefCount(1)),
+                 AllOf(QName("v"), RefCount(0))}));
 }
 
 TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) {
@@ -621,22 +670,28 @@ TEST_F(SymbolCollectorTest, SymbolFormedByCLI) {
                            DeclURI(TestHeaderURI))));
 }
 
-TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
-  const std::string Header = R"(
+TEST_F(SymbolCollectorTest, SymbolsInMainFile) {
+  const std::string Main = R"(
     class Foo {};
     void f1();
     inline void f2() {}
-  )";
-  const std::string Main = R"(
+
     namespace {
-    void ff() {} // ignore
+    void ff() {}
     }
-    void main_f() {} // ignore
+    namespace foo {
+    namespace {
+    class Bar {};
+    }
+    }
+    void main_f() {}
     void f1() {}
   )";
-  runSymbolCollector(Header, Main);
+  runSymbolCollector(/*Header=*/"", Main);
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2")));
+              UnorderedElementsAre(QName("Foo"), QName("f1"), QName("f2"),
+                                   QName("ff"), QName("foo"), QName("foo::Bar"),
+                                   QName("main_f")));
 }
 
 TEST_F(SymbolCollectorTest, ClassMembers) {
@@ -978,7 +1033,8 @@ TEST_F(SymbolCollectorTest, ReferencesInFriendDecl) {
   CollectorOpts.CountReferences = true;
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), RefCount(1)),
-                                            AllOf(QName("Y"), RefCount(1))));
+                                            AllOf(QName("Y"), RefCount(1)),
+                                            AllOf(QName("C"), RefCount(0))));
 }
 
 TEST_F(SymbolCollectorTest, Origin) {
@@ -1005,7 +1061,7 @@ TEST_F(SymbolCollectorTest, CollectMacros) {
   CollectorOpts.CollectMacro = true;
   runSymbolCollector(Header.code(), Main);
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("p"),
+              UnorderedElementsAre(QName("p"), QName("t"),
                                    AllOf(QName("X"), DeclURI(TestHeaderURI),
                                          IncludeHeader(TestHeaderURI)),
                                    AllOf(Labeled("MAC(x)"), RefCount(0),