[clangd] Include the underlying decls in go-to-definition.

[hokein]

Summary

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

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[hokein]

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

959–1023

we need to avoid the underlying decl in this case.

[hokein]

no regression on non-definition of renaming alias.

[hokein]

remove an accident change.

[hokein]

friendly ping, this patch should be ready for review.

[nridge]

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

980

Why is it useful to give the using-declaration itself as a result?

It seems like the more useful behaviour from the user's point of view is to jump directly to the definition of class Foo, without having to choose from a popup first.

[sammccall]

Sorry about being late coming back to this.

clang-tools-extra/clangd/XRefs.cpp

295

I believe this is going to do the wrong thing on overload sets.

namespace ns { int x(char); int x(double); }
using ns::x;
int y = ^x('a');

getDeclAtPosition(..., TemplatePattern|Alias) will yield UsingDecl, and then targetDecl(UsingDecl, Underlying) will yield both overloads of x.
However getDeclAtPosition(..., Underlying) would correctly return only the desired overload.
(Please add a test like this!)

This is awkward to work around, it's a failure of the targetDecl API. Maybe for now, call getDeclAtPosition with TemplatePattern|Alias, and if there's exactly one result, replace D with it? And leave a fixme to address more complicated cases somehow.

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

980

+1, that would mean adding a continue after the addresultdecl

[hokein]

address review comment: fix the wrong result for multiple overloads.

[sammccall]

Thanks!

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

1005

this is basically the same as the above case (which is fine)
but what about class [[Foo]]; using Bar = ^Foo;?

(maybe this is tested elsewhere already, but it should be moved here)

[hokein]

address comment.

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)) {
+      // 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;
+    }
+
     // Otherwise the target declaration is the right one.
     AddResultDecl(D);
   }
 
   // Now query the index for all Symbol IDs we found in the AST.
   if (Index && !ResultIndex.empty()) {
     LookupRequest QueryRequest;
     for (auto It : ResultIndex)

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

 using ::testing::IsEmpty;
 using ::testing::Matcher;
 using ::testing::UnorderedElementsAre;
 using ::testing::UnorderedElementsAreArray;
 
 MATCHER_P2(FileRange, File, Range, "") {
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
+MATCHER(DeclRange, "") {
+  const LocatedSymbol &Sym = ::testing::get<0>(arg);
+  const Range &Range = ::testing::get<1>(arg);
+  return Sym.PreferredDeclaration.range == Range;
+}
 
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher<const std::vector<DocumentHighlight> &>
 HighlightsFrom(const Annotations &Test) {
   std::vector<DocumentHighlight> Expected;
   auto Add = [&](const Range &R, DocumentHighlightKind K) {
     Expected.emplace_back();
TEST(LocateSymbol, TextualDependent) {
   // locateSymbolNamedTextuallyAt().
   auto Results = locateSymbolAt(AST, Source.point(), Index.get());
   EXPECT_THAT(Results,
               UnorderedElementsAre(
                   Sym("uniqueMethodName", Header.range("FooLoc"), llvm::None),
                   Sym("uniqueMethodName", Header.range("BarLoc"), llvm::None)));
 }
 
-TEST(LocateSymbol, TemplateTypedefs) {
-  auto T = Annotations(R"cpp(
+TEST(LocateSymbol, Alias) {
+  const char *Tests[] = {
+    R"cpp(
       template <class T> struct function {};
-    template <class T> using callback = function<T()>;
+      template <class T> using [[callback]] = function<T()>;
 
       c^allback<int> foo;
-  )cpp");
+    )cpp",
+
+    // triggered on non-definition of a renaming alias: should not give any
+    // underlying decls.
+    R"cpp(
+      class Foo {};
+      typedef Foo [[Bar]];
+
+      B^ar b;
+    )cpp",
+    R"cpp(
+      class Foo {};
+      using [[Bar]] = Foo; // definition
+      Ba^r b;
+    )cpp",
+
+    // triggered on the underlying decl of a renaming alias.
+    R"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;
+    )cpp",
+
+    R"cpp(
+      namespace ns { int [[x]](char); int [[x]](double); }
+      using ns::^x;
+    )cpp",
+
+    R"cpp(
+      namespace ns { int [[x]](char); int x(double); }
+      using ns::x;
+      int y = ^x('a');
+    )cpp",
+
+    R"cpp(
+      namespace ns { class [[Foo]] {}; }
+      using ns::Foo;
+      F^oo f;
+    )cpp",
+
+    // other cases that don't matter much.
+    R"cpp(
+      class Foo {};
+      typedef Foo [[Ba^r]];
+    )cpp",
+    R"cpp(
+      class Foo {};
+      using [[B^ar]] = Foo;
+    )cpp",
+  };
+
+  for (const auto* Case : Tests) {
+    SCOPED_TRACE(Case);
+    auto T = Annotations(Case);
     auto AST = TestTU::withCode(T.code()).build();
-  EXPECT_THAT(locateSymbolAt(AST, T.point()), ElementsAre(Sym("callback")));
+    EXPECT_THAT(locateSymbolAt(AST, T.point()),
+                ::testing::UnorderedPointwise(DeclRange(), T.ranges()));
+  }
 }
 
 TEST(LocateSymbol, RelPathsInCompileCommand) {
   // The source is in "/clangd-test/src".
   // We build in "/clangd-test/build".
 
   Annotations SourceAnnotations(R"cpp(
 #include "header_in_preamble.h"