[clangd] When finding refs for a renaming alias, do not return refs to underlying decls

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/515

[hokein]

clang-tools-extra/clangd/XRefs.cpp

1142–1143

IIRC, clangd's index doesn't distinguish refs of template specifications/primary templates, all refs are treated as primary templates -- if we query refs for a template specification from the index, we'll get no results, so we will end up with refs from the main file (which are from the AST) only.

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

1574

hmm, the new behavior seems wired (at least in this case), I thought it is a bug at first glance, then I realized that this is xrefs for template specification. I'd expect to see the primary template in refs in xrefs result.

however if there is an explicit template specification, excluding the primary template maybe reasonable.

template <typename T>
class Foo {};
template <> class [[Foo]]<int> {};
void func([[Fo^o]]<int>);

[nridge]

Take a different fix approach, as suggested in the issue discussion

[hokein]

clang-tools-extra/clangd/XRefs.cpp

1150

I think it should not happen in practice (Decls just have 1 element in most cases), but the code feels hacky, we are throwing other decls if one of the Decls is a using decl.

I suppose if we're using the same workaround as locateASTReferent, then we should follow that way by only adjusting the UsingDecl/UnresolvedValueDecl results and keeping others.

In general, I think we probably need to remove this workaround (see the FIXME in locateASTReferent) by refining TargetDecl API. The current DeclRelation::Underlying enum is not enough to support our use case where we only want underlying decls for *non-renaming* alias. One rough idea to fix it is to split the Underlying to two RenameAliasUnderlying and RemainingUnderlying -- this would need some API design work, so no need to do it in this patch.

[nridge]

clang-tools-extra/clangd/XRefs.cpp

1150

I don't think we're actually throwing out the other results: by calling getDeclAtPosition() with Relations | Underlying, where Relations is the original flags, the call should find the other results again. If we only replaced the UsingDecl/UnresolvedValueDecl results, I think the other results would appear in duplicate.

[nridge]

Address review comment

[nridge]

clang-tools-extra/clangd/XRefs.cpp

1150

Anyways, I've revised it as you suggested. I don't think it should make a difference in practice.

[hokein]

thanks.

clang-tools-extra/clangd/XRefs.cpp

1149

nit: name it NonrenamingAliasUnderlyingDecls?

1157

I think the break is not necessarily needed. In practice we should just have one non-renaming alias at the current position. If not, that probably means we likely have a bug, break seems to just hide the bug.

[nridge]

Address final review comments

clang-tools-extra/clangd/XRefs.cpp

1157

Agreed, the break was left over from the previous approach and shouldn't be there any more.

clang-tools-extra/clangd/XRefs.cpp

if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
           Result.uri = URIMainFile;
           Results.References.push_back(std::move(Result));
         }
       }
       Req.IDs.insert(*MacroSID);
     }
   } else {
     // Handle references to Decls.
 
-    // We also show references to the targets of using-decls, so we include
-    // DeclRelation::Underlying.
-    DeclRelationSet Relations = DeclRelation::TemplatePattern |
-                                DeclRelation::Alias | DeclRelation::Underlying;
+    DeclRelationSet Relations =
+        DeclRelation::TemplatePattern | DeclRelation::Alias;
     auto Decls = getDeclAtPosition(AST, *CurLoc, Relations);
+    // If the results include a *non-renaming* alias, get its
+    // underlying decls as well. (See similar logic in locateASTReferent()).
+    for (const NamedDecl *D : Decls) {
+      if (llvm::isa<UsingDecl>(D) || llvm::isa<UnresolvedUsingValueDecl>(D)) {
+        Decls = getDeclAtPosition(AST, *CurLoc,
+                                  Relations | DeclRelation::Underlying);
+        break;
+      }
+    }
 
     // We traverse the AST to find references in the main file.
     auto MainFileRefs = findRefs(Decls, AST);
     // We may get multiple refs with the same location and different Roles, as
     // cross-reference is only interested in locations, we deduplicate them
     // by the location to avoid emitting duplicated locations.
     MainFileRefs.erase(std::unique(MainFileRefs.begin(), MainFileRefs.end(),
                                    [](const ReferenceFinder::Reference &L,
                                       const ReferenceFinder::Reference &R) {
                                      return L.SpelledTok.location() ==
                                             R.SpelledTok.location();

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

const char *Tests[] = {
        template <typename T>
        class [[Fo^o]] {};
        void func([[Foo]]<int>);
       )cpp",
 
       R"cpp(
        template <typename T>
        class [[Foo]] {};
        void func([[Fo^o]]<int>);
       )cpp",
       R"cpp(// Not touching any identifiers.
         struct Foo {
           [[~]]Foo() {};
         };
         void foo() {
           Foo f;
           f.[[^~]]Foo();
         }
       )cpp",
+      R"cpp(// Renaming alias
+        template <typename> class Vector {};
+        using [[^X]] = Vector<int>;
+        [[X]] x1;
+        Vector<int> x2;
+        Vector<double> y;
+      )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,