[clangd] Collect main file refs by default

[nridge]

Summary

This is needed for call hierarchy to be able to find callers of
main-file-only functions.

[kadircet]

i remember discussing this in the past, and having "negative" feelings about it. I couldn't find the discussion on the issues page tho, could you toss me the link if you can find it? (it might've been an offline discussion as well, sorry if that's the case for not dumping a summary).

But if there were no discussions before, it would be nice to see some numbers, for increase in memory and disk usage for LLVM.

[kadircet]

NVM, found the summary https://github.com/clangd/clangd/issues/162#issuecomment-653981038

[nridge]

Note, the call hierarchy feature is pretty significantly impaired without this. See the example of Selection::createEach() discussed in this comment.

[kadircet]

Thanks, LGTM.

clang-tools-extra/clangd/tool/ClangdMain.cpp

497

can we just change the default in ClangdServer::Options and also use that one in here (i.e. ClangdServer::Options().CollectMainFileRefs)? without plumbing the change to symbolcollector and fileindex ?

[nridge]

One thing that just occurred to me: does this need an index version bump?

[kadircet]

nope, as it doesn't change the serialization format. but any existing shards won't have refs from the main file, they'll accumulate over time as sources gets modified. (i don't think it is worth bumping the version to invalidate existing shards, if a user cares enough they can manually delete their cache)

[nridge]

Address review comment

clang-tools-extra/clangd/ClangdServer.h

struct Options {
     bool BuildDynamicSymbolIndex = false;
     /// Use a heavier and faster in-memory index implementation.
     bool HeavyweightDynamicSymbolIndex = true;
     /// If true, ClangdServer automatically indexes files in the current project
     /// on background threads. The index is stored in the project root.
     bool BackgroundIndex = false;
 
     /// Store refs to main-file symbols in the index.
-    bool CollectMainFileRefs = false;
+    bool CollectMainFileRefs = true;
 
     /// If set, use this index to augment code completion results.
     SymbolIndex *StaticIndex = nullptr;
 
     /// If set, queried to obtain the configuration to handle each request.
     config::Provider *ConfigProvider = nullptr;
 
     /// The Options provider to use when running clang-tidy. If null, clang-tidy

clang-tools-extra/clangd/tool/ClangdMain.cpp

desc(
         "Configuration is documented at https://clangd.llvm.org/config.html"),
     init(true),
 };
 
 opt<bool> CollectMainFileRefs{
     "collect-main-file-refs",
     cat(Misc),
     desc("Store references to main-file-only symbols in the index"),
-    init(false),
+    init(ClangdServer::Options().CollectMainFileRefs),
 };
 
 #if CLANGD_ENABLE_REMOTE
 opt<std::string> RemoteIndexAddress{
     "remote-index-address",
     cat(Features),
     desc("Address of the remote index server"),
 };

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp

FS.Files[testPath("root/B.cc")] =
         (void)common;
         (void)common;
         (void)common;
       })cpp";
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);
   OverlayCDB CDB(/*Base=*/nullptr);
-  BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
-                      /*Opts=*/{});
+  BackgroundIndex::Options Opts;
+  Opts.CollectMainFileRefs = true;
+  BackgroundIndex Idx(
+      FS, CDB, [&](llvm::StringRef) { return &MSS; }, Opts);
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   EXPECT_THAT(runFuzzyFind(Idx, ""),
               UnorderedElementsAre(AllOf(Named("common"), NumReferences(1U)),
                                    AllOf(Named("A_CC"), NumReferences(0U)),
-                                   AllOf(Named("g"), NumReferences(0U)),
+                                   AllOf(Named("g"), NumReferences(1U)),
                                    AllOf(Named("f_b"), Declared(),
                                          Not(Defined()), NumReferences(0U))));
 
   Cmd.Filename = testPath("root/B.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
   CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
 
   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(AllOf(Named("common"), NumReferences(5U)),
                                    AllOf(Named("A_CC"), NumReferences(0U)),
-                                   AllOf(Named("g"), NumReferences(0U)),
+                                   AllOf(Named("g"), NumReferences(1U)),
                                    AllOf(Named("f_b"), Declared(), Defined(),
                                          NumReferences(1U))));
 
   auto Syms = runFuzzyFind(Idx, "common");
   EXPECT_THAT(Syms, UnorderedElementsAre(Named("common")));
   auto Common = *Syms.begin();
   EXPECT_THAT(getRefs(Idx, Common.ID),
               RefsAre({FileURI("unittest:///root/A.h"),
                        FileURI("unittest:///root/A.cc"),
                        FileURI("unittest:///root/B.cc"),
                        FileURI("unittest:///root/B.cc"),
                        FileURI("unittest:///root/B.cc"),
                        FileURI("unittest:///root/B.cc")}));
 }
 
 TEST_F(BackgroundIndexTest, MainFileRefs) {
   MockFS FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
       void header_sym();
       )cpp";
   FS.Files[testPath("root/A.cc")] =
       "#include \"A.h\"\nstatic void main_sym() { (void)header_sym; }";
 
-  // Check the behaviour with CollectMainFileRefs = false (the default).
+  // Check the behaviour with CollectMainFileRefs = false (the default
+  // at the SymbolCollector level).
   {
     llvm::StringMap<std::string> Storage;
     size_t CacheHits = 0;
     MemoryShardStorage MSS(Storage, CacheHits);
     OverlayCDB CDB(/*Base=*/nullptr);
     BackgroundIndex Idx(FS, CDB, [&](llvm::StringRef) { return &MSS; },
                         /*Opts=*/{});