[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

281

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

729

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

281

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

729

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

281

• 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

733

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

281

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

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

733

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

281

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

733

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

281

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 =
       SM.isWrittenInMainFile(SM.getExpansionLoc(ND->getBeginLoc())) &&
       !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.
+  // 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) &&
-      (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) {
+  // Refs collected from SymbolCollector behave in the same way as
+  // AST-based xrefs.
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+    #define TYPE(X) X
+    #define FOO Foo
+    #define CAT(X, Y) X##Y
+    class [[Foo]] {};
+    void test() {
+      TYPE([[Foo]]) foo;
+      [[FOO]] foo2;
+      TYPE(TYPE([[Foo]])) foo3;
+      [[CAT]](Fo, o) foo4;
+    }
+  )");
+  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;
   }

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

const char *Tests[] = {
       )cpp",
 
       R"cpp(// Namespace
         namespace [[ns]] {
         struct Foo {};
         } // namespace ns
         int main() { [[^ns]]::Foo foo; }
       )cpp",
+
+      R"cpp(// Macros
+        #define TYPE(X) X
+        #define FOO Foo
+        #define CAT(X, Y) X##Y
+        class [[Fo^o]] {};
+        void test() {
+          TYPE([[Foo]]) foo;
+          [[FOO]] foo2;
+          TYPE(TYPE([[Foo]])) foo3;
+          [[CAT]](Fo, o) foo4;
+        }
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
     std::vector<Matcher<Location>> ExpectedLocations;
     for (const auto &R : T.ranges())
       ExpectedLocations.push_back(RangeIs(R));
     EXPECT_THAT(findReferences(AST, T.point(), 0).References,