[clangd] Use expansion location when the ref is inside macros.

[hokein]

Summary

Previously, xrefs has inconsistent behavior when the reference is inside
macro body:

• AST-based xrefs (for main file) uses the expansion location;
• our index uses the spelling location;

This patch makes our index use expansion location, which is consistent with
AST-based xrefs, and kythe as well.

[ilya-biryukov]

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

280

We're using getSpellingLoc here and getFileLoc later. Why not use getFileLoc everywhere?

Having a variable (similar to the SpellingLoc we had before) and calling getFileLoc only once also seems preferable.

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

655

Could you also check:

1. Multi-level macro arguments TYPE(TYPE(FOO))
2. Concatenated tokens as identifiers, e.g. coming from #define CAT(X, Y) X##Y

[merge_guards_bot]

Build result: pass - 60164 tests passed, 0 failed and 731 were skipped.
Log files: console-log.txt, CMakeCache.txt

[hokein]

add more tests.

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

280

We're using getSpellingLoc here and getFileLoc later. Why not use getFileLoc everywhere?

There are two things in SymbolCollector:

• symbols & ranking signals, we use spelling location for them, the code is part of this, ReferencedDecls is used to calculate the ranking
• references

this patch only targets the reference part (changing the loc here would break many assumptions I think, and there was a failure test).

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

655

Added tests.

[merge_guards_bot]

Build result: pass - 60164 tests passed, 0 failed and 731 were skipped.
Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

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

280

• What are the assumptions that it will break?
• What is rationale for using spelling locations for ranking and file location for references?

It would be nice to have this spelled out somewhere in the code, too. Currently this looks like an accidental inconsistency. Especially given that getFileLoc and getSpellingLoc are often the same.

[ilya-biryukov]

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

659

Previously we would not report any location at all in that case, right?
Not sure how common this is, hope this won't blow up our index size too much. No need to change anything now, but we should be ready to revert if needed.

Worth putting a comment here that AST-based XRefs behave in the same way. (And maybe adding a test there, if there isn't one already)

[hokein]

address comments

• add related test for ast-based xrefs
• add comments to clarify using different location for symbols/references.

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

280

Added comments to clarify the difference between references and other fields.

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

659

Yes, I measure the memory usage before vs after, it increased ~5% memory usage.

[merge_guards_bot]

Build result: pass - 60296 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

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

280

The comment still does not explain *why* we do this.
Why not use the same type of locations for everything?

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

659

On LLVM? This doesn't look too bad, but probably worth mentioning it in the patch description

[ilya-biryukov]

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

280

Update after offline discussion:
there does not seem to be any good reason to use spelling locations and not file locations here. Some reference counts will be affected, but only those that end up being spelled in the macros inside the main file and used outside the main file. Which is very rare and shouldn't affect completion ranking much.

We should definitely try switching to file locations everywhere, but that means doing more changes. Therefore, it's more appropriate to do it in the follow-up change.

[ilya-biryukov]

LGTM

[hokein]

add a fixme.

[merge_guards_bot]

Build result: pass - 60625 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

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

if (D->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None)
     D = CanonicalDecls.try_emplace(D, ASTNode.OrigD).first->second;
   const NamedDecl *ND = dyn_cast<NamedDecl>(D);
   if (!ND)
     return true;
 
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto &SM = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
       (Roles & static_cast<unsigned>(index::SymbolRole::Reference)) &&
-      SM.getFileID(SpellingLoc) == SM.getMainFileID())
+      SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
     ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
   if (!ID)
     return true;
 
   // Note: we need to process relations for all decl occurrences, including
   // refs, because the indexing code only populates relations for specific
bool IsMainFileOnly =
       !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
                     ASTCtx->getLangOpts());
   // In C, printf is a redecl of an implicit builtin! So check OrigD instead.
   if (ASTNode.OrigD->isImplicit() ||
       !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
     return true;
   // 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);
+      (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:
   // - ObjCPropertyDecl may have an OrigD of ObjCPropertyImplDecl, which is
   // not a NamedDecl.
   auto *OriginalDecl = dyn_cast<NamedDecl>(ASTNode.OrigD);

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

TEST_F(SymbolCollectorTest, NameReferences) {
   CollectorOpts.RefFilter = RefKind::All;
   runSymbolCollector(Header.code(), "");
   // When we find references for class Foo, we expect to see all
   // constructor/destructor references.
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
                                   HaveRanges(Header.ranges()))));
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+    #define TYPE(X) X
+    #define FOO Foo
+    class [[Foo]] {};
+    void test() {
+      TYPE([[Foo]]) foo;
+      [[FOO]] foo2;
+    }
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+                                  HaveRanges(Header.ranges()))));
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
   class $Foo[[Foo]] {};
 
   void $Func[[Func]]() {
     $Foo[[Foo]] fo;
   }