[clangd] Don't set the Underlying bit on targets of UsingDecls.

[hokein]

Summary

With this patch, we don't treat using ns::X as a first-class declaration like using Z = ns::Y, reference to X that goes through this using-decl is considered a direct reference (without the Underlying bit).

Fix the workaround in https://reviews.llvm.org/D87225 and https://reviews.llvm.org/D74054.

[hokein]

clang-tools-extra/clangd/FindTarget.h

112

The previous Alias vs Underlying were pretty nice, but they were not enough to support our non-renaming-alias-underlying case. Looking for the feedback on the API change here.

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

1121–1122

this seems a small regression, I think it is fine.

1132

I think it is an improvement.

[sammccall]

We manage to get rid of a little code here, but we add complexity to an important API, and make... one test better and a few tests worse.
I'd like to get rid of the wart in the code, but the tradeoff doesn't seem completely compelling... going to think about the API here a bit.

clang-tools-extra/clangd/FindTarget.h

112

Inevitably this adds some noise, and the names are less clear (we certainly need to avoid using "alias" to mean two different things, and we should try to avoid negatives that confuse the boolean logic).
We could simplify this at least at query locations by defining Underlying = NonAliasUnderlying | AliasUnderlying, but unfortunately we can't put it in DeclRelation.

The other thing is this isn't complete either, because it doesn't distinguish between renaming and non-renaming Aliases. This is why we have the regression in the testcases.

That this is so verbose is a property of the original design: the idea was "mark this edge as alias->underlying, and apply a bit when that edge is traversed" but of course we decided to signal "the edge is not traversed" too. So if we introduce a new type of edge, we need *two* new bits - one at each end.

I'm trying to think of ideas for improvements without totally ditching the API...

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

1121–1122

I can't follow why this is different from the second case of

TEST_F(TargetDeclTest, UsingDecl), where the usingdecl is not reported at all.

[hokein]

refine the patch based on the offline discussion:

• don't set the Underlying bits for using-declaration's underlying decl.

[hokein]

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

1121–1122

I think you over-look it, the second case of TEST_F(TargetDeclTest, UsingDecl) does report the using-decl.

[sammccall]

LG, thanks for all the iterations here.

I do want to solve the regression but we can land without that.

clang-tools-extra/clangd/FindTarget.cpp

346

maybe a comment here "// no Underlying as this is a non-renaming alias"

358

and here

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

1121–1122

I'd like to make a plan to solve this regression (we can do it here or in a followup patch, happy to hack on that).

What do you think about the following heuristic:

• if go-to-definition yields multiple results
• and a particular result touches the cursor*
• (optional) and that result is marked with Alias
• then drop it from the set

It means we still keep some special-case code, but it's not complex and fairly intuitive I think.

[hokein]

address comments.

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

1121–1122

sounds good. I'd leave it as a follow-up. Added a FIXME.

clang-tools-extra/clangd/FindTarget.h

enum class DeclRelation : unsigned {
   /// This is the template instantiation that was referred to.
   /// e.g. template<> class vector<int> (the implicit specialization)
   TemplateInstantiation,
   /// This is the pattern the template specialization was instantiated from.
   /// e.g. class vector<T> (the pattern within the primary template)
   TemplatePattern,
 
   // Alias options apply when the declaration is an alias.
-  // e.g. namespace clang { [[StringRef]] S; }
+  // e.g. namespace client { [[X]] x; }
 
   /// This declaration is an alias that was referred to.
-  /// e.g. using llvm::StringRef (the UsingDecl directly referenced).
+  /// e.g. using ns::X (the UsingDecl directly referenced),
+  ///      using Z = ns::Y (the TypeAliasDecl directly referenced)
   Alias,
-  /// This is the underlying declaration for an alias, decltype etc.
-  /// e.g. class llvm::StringRef (the underlying declaration referenced).
+  /// This is the underlying declaration for a renaming-alias, decltype etc.
+  /// e.g. class ns::Y (the underlying declaration referenced).
+  ///
+  /// Note that we don't treat `using ns::X` as a first-class declaration like
+  /// `using Z = ns::Y`. Therefore reference to X that goes through this
+  /// using-decl is considered a direct reference (without the Underlying bit).
+  /// Nevertheless, we report `using ns::X` as an Alias, so that some features
+  /// like go-to-definition can still target it.
   Underlying,
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, DeclRelation);
 
 /// Information about a reference written in the source code, independent of the
 /// actual AST node that this reference lives in.
 /// Useful for tools that are source-aware, e.g. refactorings.
 struct ReferenceLoc {

clang-tools-extra/clangd/FindTarget.cpp

void add(const Decl *Dcl, RelSet Flags) {
     debug(*D, Flags);
     if (const UsingDirectiveDecl *UDD = llvm::dyn_cast<UsingDirectiveDecl>(D))
       D = UDD->getNominatedNamespaceAsWritten();
 
     if (const TypedefNameDecl *TND = dyn_cast<TypedefNameDecl>(D)) {
       add(TND->getUnderlyingType(), Flags | Rel::Underlying);
       Flags |= Rel::Alias; // continue with the alias.
     } else if (const UsingDecl *UD = dyn_cast<UsingDecl>(D)) {
+      // no Underlying as this is a non-renaming alias.
       for (const UsingShadowDecl *S : UD->shadows())
-        add(S->getUnderlyingDecl(), Flags | Rel::Underlying);
+        add(S->getUnderlyingDecl(), Flags);
       Flags |= Rel::Alias; // continue with the alias.
     } else if (const auto *NAD = dyn_cast<NamespaceAliasDecl>(D)) {
       add(NAD->getUnderlyingDecl(), Flags | Rel::Underlying);
       Flags |= Rel::Alias; // continue with the alias
     } else if (const UnresolvedUsingValueDecl *UUVD =
                    dyn_cast<UnresolvedUsingValueDecl>(D)) {
       for (const NamedDecl *Target : getMembersReferencedViaDependentName(
                UUVD->getQualifier()->getAsType(),
                [UUVD](ASTContext &) { return UUVD->getNameInfo().getName(); },
                ValueFilter)) {
-        add(Target, Flags | Rel::Underlying);
+        add(Target, Flags); // no Underlying as this is a non-renaming alias
       }
       Flags |= Rel::Alias; // continue with the alias
     } else if (const UsingShadowDecl *USD = dyn_cast<UsingShadowDecl>(D)) {
       // Include the using decl, but don't traverse it. This may end up
       // including *all* shadows, which we don't want.
       report(USD->getUsingDecl(), Flags | Rel::Alias);
       // Shadow decls are synthetic and not themselves interesting.
       // Record the underlying decl instead, if allowed.
       D = USD->getTargetDecl();
-      Flags |= Rel::Underlying; // continue with the underlying decl.
     } else if (const auto *DG = dyn_cast<CXXDeductionGuideDecl>(D)) {
       D = DG->getDeducedTemplate();
     } else if (const ObjCImplementationDecl *IID =
                    dyn_cast<ObjCImplementationDecl>(D)) {
       // Treat ObjC{Interface,Implementation}Decl as if they were a decl/def
       // pair as long as the interface isn't implicit.
       if (const auto *CID = IID->getClassInterface())
         if (const auto *DD = CID->getDefinition())

clang-tools-extra/clangd/XRefs.cpp

for (const NamedDecl *D :
     if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
       if (TouchedIdentifier &&
           D->getLocation() == TouchedIdentifier->location()) {
         AddResultDecl(CTSD->getSpecializedTemplate());
         continue;
       }
     }
 
-    // Give the underlying decl if navigation is triggered on a non-renaming
-    // alias.
-    if (llvm::isa<UsingDecl>(D) || llvm::isa<UnresolvedUsingValueDecl>(D)) {
-      // FIXME: address more complicated cases. TargetDecl(... Underlying) gives
-      // all overload candidates, we only want the targeted one if the cursor is
-      // on an using-alias usage, workround it with getDeclAtPosition.
-      llvm::for_each(
-          getDeclAtPosition(AST, CurLoc, DeclRelation::Underlying, NodeKind),
-          [&](const NamedDecl *UD) { AddResultDecl(UD); });
-      continue;
-    }
-
     // Special case: if the class name is selected, also map Objective-C
     // categories and category implementations back to their class interface.
     //
     // Since `TouchedIdentifier` might refer to the `ObjCCategoryImplDecl`
     // instead of the `ObjCCategoryDecl` we intentionally check the contents
     // of the locs when checking for class name equivalence.
     if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D))
       if (const auto *ID = CD->getClassInterface())
if (Macro) {
     }
   } else {
     // Handle references to Decls.
 
     DeclRelationSet Relations =
         DeclRelation::TemplatePattern | DeclRelation::Alias;
     std::vector<const NamedDecl *> Decls =
         getDeclAtPosition(AST, *CurLoc, Relations);
-    std::vector<const NamedDecl *> NonrenamingAliasUnderlyingDecls;
-    // 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)) {
-        for (const NamedDecl *AD :
-             getDeclAtPosition(AST, *CurLoc, DeclRelation::Underlying))
-          NonrenamingAliasUnderlyingDecls.push_back(AD);
-      }
-    }
-    llvm::copy(NonrenamingAliasUnderlyingDecls, std::back_inserter(Decls));
 
     // 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,

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

Code = R"cpp(
     namespace foo {
       int f(int);
       int f(char);
     }
     using foo::f;
     int x = [[f]](42);
   )cpp";
   // f(char) is not referenced!
-  EXPECT_DECLS("DeclRefExpr", {"using foo::f", Rel::Alias},
-               {"int f(int)", Rel::Underlying});
+  EXPECT_DECLS("DeclRefExpr", {"using foo::f", Rel::Alias}, {"int f(int)"});
 
   Code = R"cpp(
     namespace foo {
       int f(int);
       int f(char);
     }
     [[using foo::f]];
   )cpp";
   // All overloads are referenced.
-  EXPECT_DECLS("UsingDecl", {"using foo::f", Rel::Alias},
-               {"int f(int)", Rel::Underlying},
-               {"int f(char)", Rel::Underlying});
+  EXPECT_DECLS("UsingDecl", {"using foo::f", Rel::Alias}, {"int f(int)"},
+               {"int f(char)"});
 
   Code = R"cpp(
     struct X {
       int foo();
     };
     struct Y : X {
       using X::foo;
     };
     int x = Y().[[foo]]();
   )cpp";
-  EXPECT_DECLS("MemberExpr", {"using X::foo", Rel::Alias},
-               {"int foo()", Rel::Underlying});
+  EXPECT_DECLS("MemberExpr", {"using X::foo", Rel::Alias}, {"int foo()"});
 
   Code = R"cpp(
       template <typename T>
       struct Base {
         void waldo() {}
       };
       template <typename T>
       struct Derived : Base<T> {
         using Base<T>::[[waldo]];
       };
     )cpp";
   EXPECT_DECLS("UnresolvedUsingValueDecl", {"using Base<T>::waldo", Rel::Alias},
-               {"void waldo()", Rel::Underlying});
+               {"void waldo()"});
 }
 
 TEST_F(TargetDeclTest, ConstructorInitList) {
   Code = R"cpp(
     struct X {
       int a;
       X() : [[a]](42) {}
     };

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

)cpp",
       class [[Foo]];
       using Bar = Fo^o;
     )cpp",
 
       // triggered on definition of a non-renaming alias: should give underlying
       // decls.
       R"cpp(
       namespace ns { class [[Foo]] {}; }
-      using ns::F^oo;
+      // FIXME: don't return the using decl if it touches the cursor position.
+      using ns::[[F^oo]];
     )cpp",
 
       R"cpp(
       namespace ns { int [[x]](char); int [[x]](double); }
-      using ns::^x;
+      using ns::[[^x]];
     )cpp",
 
       R"cpp(
       namespace ns { int [[x]](char); int x(double); }
-      using ns::x;
+      using ns::[[x]];
       int y = ^x('a');
     )cpp",
 
       R"cpp(
       namespace ns { class [[Foo]] {}; }
       using ns::Foo;
       F^oo f;
     )cpp",
)cpp",
       // Member of dependent base
       R"cpp(
       template <typename T>
       struct Base {
         void [[waldo]]() {}
       };
       template <typename T>
       struct Derived : Base<T> {
-        using Base<T>::w^aldo;
+        using Base<T>::[[w^aldo]];
       };
     )cpp",
   };
 
   for (const auto* Case : Tests) {
     SCOPED_TRACE(Case);
     auto T = Annotations(Case);
     auto AST = TestTU::withCode(T.code()).build();