[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

1150

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

1157

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

1157

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

1157

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

1156

nit: name it NonrenamingAliasUnderlyingDecls?

1164

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

1164

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

clang-tools-extra/clangd/XRefs.cpp

public:
 
   bool
   handleDeclOccurrence(const Decl *D, index::SymbolRoleSet Roles,
                        llvm::ArrayRef<index::SymbolRelation> Relations,
                        SourceLocation Loc,
                        index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
     assert(D->isCanonicalDecl() && "expect D to be a canonical declaration");
     const SourceManager &SM = AST.getSourceManager();
-    if (!CanonicalTargets.count(D) || !isInsideMainFile(Loc, SM))
+    // For references to template specializations, `D` will contain the
+    // template and `ASTNode.OrigD` the specialization. We want to find
+    // references to specializations, to check `ASTNode.OrigD` as well.
+    bool ReferencesCanonicalTarget =
+        CanonicalTargets.count(D) || CanonicalTargets.count(ASTNode.OrigD);
+    if (!ReferencesCanonicalTarget || !isInsideMainFile(Loc, SM))
       return true;
     const auto &TB = AST.getTokens();
     Loc = SM.getFileLoc(Loc);
     if (const auto *Tok = TB.spelledTokenAt(Loc))
       References.push_back({*Tok, Roles});
     return true;
   }
 
if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
       }
       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 |
+    DeclRelationSet Relations = DeclRelation::TemplateInstantiation |
                                 DeclRelation::Alias | DeclRelation::Underlying;
     auto Decls = getDeclAtPosition(AST, *CurLoc, Relations);
 
     // 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();
                                    }),
                        MainFileRefs.end());
     for (const auto &Ref : MainFileRefs) {
       Location Result;
       Result.range = Ref.range(SM);
       Result.uri = URIMainFile;
       Results.References.push_back(std::move(Result));
     }
     if (Index && Results.References.size() <= Limit) {

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

const char *Tests[] = {
       R"cpp(
        template <typename T>
        class [[Fo^o]] {};
        void func([[Foo]]<int>);
       )cpp",
 
       R"cpp(
        template <typename T>
-       class [[Foo]] {};
+       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(// Temlate specialization
+        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,