[clangd] Collect references for externally visible main-file symbols

[ArcsinX]

Summary

Without this patch clangd does not collect references for main-file symbols if there is no public declaration in preamble.
Example:
test1.c

void f1() {}

test2.c

extern void f1();
void f2() {
  f^1();
}

Find all references does not show definition of f1() in the result, but GTD works OK.

[kadircet]

thanks, this looks good in general, but it would be nice to make sure we are not blowing the index memory and disk usage.

could you grab some numbers for these two before/after this patch?

clang-tools-extra/clangd/index/Serialization.cpp

422 ↗

(On Diff #280405)

this isn't really necessary as we are not changing the serialization format. you would need to delete the existing clangd caches from your machine instead.

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

317–318

this will result in calculation and caching of linkage. it should be OK to do so though, as we run indexing after parsing finishes. (no action needed, just some notes for future travellers).

[ArcsinX]

Remove version bump

[ArcsinX]

In D84513#2172028, @kadircet wrote:

thanks, this looks good in general, but it would be nice to make sure we are not blowing the index memory and disk usage.

could you grab some numbers for these two before/after this patch?

Project: LLVM with clang and clang-tools-extra (~3700 records in compile_commands.json)

Before:
Index size on disk: 96461310 bytes
BackgroundIndex: serving version 39 (596622326 bytes)

After:
Index size of disk: 97425346 bytes.
BackgroundIndex: serving version 39 (601094341 bytes)

Diff:
Size on disk: +1%
Memory: +0.7%

this isn't really necessary as we are not changing the serialization format. you would need to delete the existing clangd caches from your machine instead.

Removed version bump.

[kadircet]

thanks! numbers and the patch LGTM.

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

bool SymbolCollector::handleDeclOccurrence(
   if (IsOnlyRef && !CollectRef)
     return true;
 
   // Do not store references to main-file symbols.
   // Unlike other fields, e.g. Symbols (which use spelling locations), we use
   // file locations for references (as it aligns the behavior of clangd's
   // AST-based xref).
   // FIXME: we should try to use the file locations for other fields.
-  if (CollectRef && !IsMainFileOnly && !isa<NamespaceDecl>(ND) &&
+  if (CollectRef && (!IsMainFileOnly || ND->isExternallyVisible()) &&
+      !isa<NamespaceDecl>(ND) &&
       (Opts.RefsInHeaders ||
        SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
     DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;
 
   // FIXME: ObjCPropertyDecl are not properly indexed here:

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

FS.Files[testPath("root/A.h")] = R"cpp(
       void f_b();
       #if A
         class A_CC {};
       #else
         class B_CC{};
       #endif
       )cpp";
   FS.Files[testPath("root/A.cc")] =
-      "#include \"A.h\"\nvoid g() { (void)common; }";
+      "#include \"A.h\"\nstatic void g() { (void)common; }";
   FS.Files[testPath("root/B.cc")] =
       R"cpp(
       #define A 0
       #include "A.h"
       void f_b() {
         (void)common;
         (void)common;
         (void)common;

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

EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
                                   HaveRanges(Main.ranges("foo")))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Bar").ID,
                                   HaveRanges(Main.ranges("bar")))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "func").ID,
                                   HaveRanges(Main.ranges("func")))));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "NS").ID, _))));
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "MACRO").ID,
                                   HaveRanges(Main.ranges("macro")))));
-  // Symbols *only* in the main file (a, b, c, FUNC) had no refs collected.
+  // Symbols *only* in the main file:
+  // - (a, b) externally visible and should have refs.
+  // - (c, FUNC) externally invisible and had no refs collected.
   auto MainSymbols =
       TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "a").ID, _))));
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "b").ID, _))));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "a").ID, _)));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "b").ID, _)));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _))));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "FUNC").ID, _))));
 }
 
 TEST_F(SymbolCollectorTest, MacroRefInHeader) {
   Annotations Header(R"(
   #define $foo[[FOO]](X) (X + 1)
   #define $bar[[BAR]](X) (X + 2)
TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
   class $Foo[[Foo]] {};
 
   void $Func[[Func]]() {
     $Foo[[Foo]] fo;
   }
   )");
-  // The main file is normal .cpp file, we shouldn't collect any refs of symbols
-  // which are not declared in the preamble.
+  // The main file is normal .cpp file, we should collect the refs
+  // for externally visible symbols.
   TestFileName = testPath("foo.cpp");
   runSymbolCollector("", Header.code());
-  EXPECT_THAT(Refs, UnorderedElementsAre());
+  EXPECT_THAT(Refs,
+              UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
+                                        HaveRanges(Header.ranges("Foo"))),
+                                   Pair(findSymbol(Symbols, "Func").ID,
+                                        HaveRanges(Header.ranges("Func")))));
 
   // Run the .h file as main file, we should collect the refs.
   TestFileName = testPath("foo.h");
   runSymbolCollector("", Header.code(),
                      /*ExtraArgs=*/{"-xobjective-c++-header"});
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Func")));
   EXPECT_THAT(Refs,
               UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,