[clangd] Improve hover and goToDefinition on auto and dectype

[qchateau]

Summary

Make the hover content on auto and decltype
look like the hover content we would get on the
deduced type.

locateSymbolAt (used in goToDeclaration) follows the
deduced type instead of failing to locate the declaration.

---

I took a screenshot of what happens when you hover over an 'auto' keyword in VSCode with clangd-10 and with the one integrating my patch (see below). Concerning the goToDefinition feature, it did not work at all for 'auto', not it goes to the same definition as the one shown on hover.

PS: This is the first time I submit a patch to LLVM, please let me know what I need to change and what I could improve, both concerning the patch itself and my submission to Phabricator.

[qchateau]

I've fixed the failing unittests, added some more
to test the new behavior, and fixed some bugs in
the new code.

[qchateau]

Not sure what's wrong with the unit test "x64 windows > LLVM.CodeGen/XCore::threads.ll", I don't see how it's related to my patch

[sammccall]

Thanks for doing this! Go-to-def-on-auto has been bugging me, it'll be great to have it :-)
Overall the patch looks really solid, with nice tests. The one thing I'd suggest is splitting the two features (hover and go-to-def) into separate patches.
e.g. here, I'm not totally sure about the hover change yet, and want to think and get some input from others. But I don't want to delay the go-to-definition part. So I've left comments on the go-to-definition changes, and if you want to drop hover from this patch and upload it as a separate review, that'd be really helpful.

---

My concern with hover is mostly around consistency and clarity. In isolation, the new hover certainly looks better than the old one for structs (e.g. syntax highligthing!).
However now we have a different style for struct vs non-struct types, and it's not just a surface thing: for structs we show the declaration, but many types have no declaration to show. I'm not sure how you can show a hover for auto = char *const* or so that feels consistent with this one.

We should also consider consistency and clarity with different types of auto. Currently our hover for undeduced auto is just "auto", and our hover for auto params is a mess. It would be nice about this distinction, as when the same keyword has multiple meanings people can conflate them.

One solution would be to make auto the name, and the type the definition. This provides less information (but I suspect that information is often noise):

### deduced-type `auto`

---

struct cv::Ptr<cv::detail::ChannelsCompensator>

(For context, the auto hover was one of the first implemented. Later we went back and added some structure (type/name/definition/documentation) but the old auto hover was mostly shoehorned into this. So if we're going to change auto hover, we shuold consider trying to make it fit this schema, too)

clang-tools-extra/clangd/XRefs.cpp

761

This deserves a high-level comment: // go-to-definition on auto should find the definition of the deduced type, if possible

762

I think we should pull the contents of this if() into a function findSymbolForType or so.

LSP has a textDocument/typeDefinition request that could also make use of this logic.
(You don't need to implement that, and I'm very much in favor of having this textDocument/definition be the do-what-i-mean swiss army knife like this patch does. But I'd just like to factor this code with potential reuse in mind)

765

there are many other types we could resolve to a decl: TypedefType, TemplateTypeParmtype, ...
If we're only going to handle tag types, it deserves a FIXME comment.

But we do have a helper for this. Can you try calling targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | DeclRelation::Alias) instead of Deduced->getTypePtr()->getAsTagDecl()?

That should handle more cases, at a minimum, this case sholud pass:

using [[T]] = int;
T x;
^auto y = x;

(I see you have a test case that aliases backed by tag types are resolved to the underlying tag decl, this change would offer the alias instead, which is more consistent with our current go-to-definition. You could also offer both by passing DeclRelation::TemplatePattern | DeclRelation::Alias | DeclRelation::Underlying... but I think we should value consistency here)

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

685

There's a scalability problem with testing every corner of the C++ language (there are lots!) with every feature. Generally I prefer to cover some important/weird/bugfix cases here, but to cover as much as possible with unittests of the underlying functions.

In this case, that would mean

• moving tests that are about auto in different contexts to ASTTests.cpp (current test coverage there is poor).
• (assuming we can use targetDecl()) moving tests that are about which decl a type should resolve to to FindTargetTests.cpp - but current coverage there is pretty good so we can probably just drop many of them.

[qchateau]

• Remove the patches affecting hover
• Add tests in ASTTests.cpp to test the behavior of getDeducedType for auto and decltype
• Add missing comment
• Create a locateSymbolForType function (which uses targetDecl)
• Remove the unit tests that were testing undesirable behaviors

[qchateau]

clang-tools-extra/clangd/XRefs.cpp

765

locateSymbolForType uses targetDecl. Thanks for the tip, that's exactly what I was looking for.

I also agree with you that go-to-definition should go to the alias instead of the underlying type for consistency, but using targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | DeclRelation::Alias) is not enough to resolve this consistency issue: getDeducedType already returns the underlying type. My current knowledge of the clangd code is nou sufficient to understand why, but the root of the problem seem to lie in the DeducedTypeVisitor class.

I removed the test that tested the undesirable behavior, should I open a bug report for getDeducedType that should not "go through" aliases ? In which case, what's the right place for that ? Github ?

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

685

I added the relevant tests to ASTTests.cpp but I did not remove the tests here for the moment. I've always had the habit to keep tests that are already written, even if redundant.

If you'd like me to remove some of them, I'd say the only tests that are not redundant with the ones in ASTTests.cpp are the ones that test template specializations. Shoud I keep only these ?

[sammccall]

Looks good, thanks again!
If you don't yet have LLVM commit access, I can commit this for you, let me know which email address to associate it with.
(https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access describes how you can get commit access if you're contributing regularly)

clang-tools-extra/clangd/XRefs.cpp

482

nit: we do often just use the first element (there's almost always just one), but here we can actually return multiple LocatedSymbols (client shows some sort of selector widget for them) so I'd suggest we loop and return all

765

Great!
Marginally better than removing a test is keeping it with a comment // FIXME: it'd be nice if this resolved to the alias instead or something like that

Github is the right place to file a bug, though I don't think this is really a big deal, so sometimes we just leave the comment.

(I'm not sure offhand if the AST actually preserves the sugar type or just the underlying one)

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

685

Keeping them around is fine too, we're not strict about this.

Thanks for adding all the tests for deduced types!

[qchateau]

• Reintroduce test with a FIXME comment
• locateSymbolForType returns a vector instead of an optional

[qchateau]

I've updated the patch with your latest comments.

Please commit this patch for me, using "[e-mail]", I'll probably ask for commit access after a few successful reviews

[sammccall]

I sent you D93314 to extend the getNonReferenceType() a bit... curious what you think

clang-tools-extra/clangd/XRefs.cpp

if (Index && !ResultIndex.empty()) {
     });
   }
 
   auto Overrides = findOverrides(VirtualMethods, Index, MainFilePath);
   Result.insert(Result.end(), Overrides.begin(), Overrides.end());
   return Result;
 }
 
+std::vector<LocatedSymbol> locateSymbolForType(const ParsedAST &AST,
+                                               const QualType &Type) {
+  const auto &SM = AST.getSourceManager();
+  auto MainFilePath =
+      getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  if (!MainFilePath) {
+    elog("Failed to get a path for the main file, so no symbol.");
+    return {};
+  }
+
+  auto Decls = targetDecl(DynTypedNode::create(Type.getNonReferenceType()),
+                          DeclRelation::TemplatePattern | DeclRelation::Alias);
+  if (Decls.empty())
+    return {};
+
+  std::vector<LocatedSymbol> Results;
+  const auto &ASTContext = AST.getASTContext();
+
+  for (const NamedDecl *D : Decls) {
+    D = getPreferredDecl(D);
+
+    auto Loc = makeLocation(ASTContext, nameLocation(*D, SM), *MainFilePath);
+    if (!Loc)
+      continue;
+
+    Results.emplace_back();
+    Results.back().Name = printName(ASTContext, *D);
+    Results.back().PreferredDeclaration = *Loc;
+    if (const NamedDecl *Def = getDefinition(D))
+      Results.back().Definition =
+          makeLocation(ASTContext, nameLocation(*Def, SM), *MainFilePath);
+  }
+
+  return Results;
+}
+
 bool tokenSpelledAt(SourceLocation SpellingLoc, const syntax::TokenBuffer &TB) {
   auto ExpandedTokens = TB.expandedTokens(
       TB.sourceManager().getMacroArgExpandedLocation(SpellingLoc));
   return !ExpandedTokens.empty();
 }
 
 llvm::StringRef sourcePrefix(SourceLocation Loc, const SourceManager &SM) {
   auto D = SM.getDecomposedLoc(Loc);
std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
 
   auto CurLoc = sourceLocationInMainFile(SM, Pos);
   if (!CurLoc) {
     elog("locateSymbolAt failed to convert position to source location: {0}",
          CurLoc.takeError());
     return {};
   }
 
-  const syntax::Token *TouchedIdentifier =
-      syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
-  if (TouchedIdentifier)
-    if (auto Macro =
-            locateMacroReferent(*TouchedIdentifier, AST, *MainFilePath))
+  const syntax::Token *TouchedIdentifier = nullptr;
+  auto TokensTouchingCursor =
+      syntax::spelledTokensTouching(*CurLoc, AST.getTokens());
+  for (const syntax::Token &Tok : TokensTouchingCursor) {
+    if (Tok.kind() == tok::identifier) {
+      if (auto Macro = locateMacroReferent(Tok, AST, *MainFilePath))
         // Don't look at the AST or index if we have a macro result.
         // (We'd just return declarations referenced from the macro's
         // expansion.)
         return {*std::move(Macro)};
 
+      TouchedIdentifier = &Tok;
+      break;
+    }
+
+    if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+      // go-to-definition on auto should find the definition of the deduced
+      // type, if possible
+      if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+        auto LocSym = locateSymbolForType(AST, *Deduced);
+        if (!LocSym.empty())
+          return LocSym;
+      }
+    }
+  }
+
   ASTNodeKind NodeKind;
   auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
                                       *MainFilePath, Index, &NodeKind);
   if (!ASTResults.empty())
     return ASTResults;
 
   // If the cursor can't be resolved directly, try fallback strategies.
   auto Word =

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

 #include <cstddef>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-TEST(GetDeducedType, KwAutoExpansion) {
+TEST(GetDeducedType, KwAutoKwDecltypeExpansion) {
   struct Test {
     StringRef AnnotatedCode;
     const char *DeducedType;
   } Tests[] = {
       {"^auto i = 0;", "int"},
       {"^auto f(){ return 1;};", "int"},
+      {
+          R"cpp( // auto on struct in a namespace
+              namespace ns1 { struct S {}; }
+              ^auto v = ns1::S{};
+          )cpp",
+          "struct ns1::S",
+      },
+      {
+          R"cpp( // decltype on struct
+              namespace ns1 { struct S {}; }
+              ns1::S i;
+              ^decltype(i) j;
+          )cpp",
+          "ns1::S",
+      },
+      {
+          R"cpp(// decltype(auto) on struct&
+            namespace ns1 {
+            struct S {};
+            } // namespace ns1
+
+            ns1::S i;
+            ns1::S& j = i;
+            ^decltype(auto) k = j;
+          )cpp",
+          "struct ns1::S &",
+      },
+      {
+          R"cpp( // auto on template class
+              class X;
+              template<typename T> class Foo {};
+              ^auto v = Foo<X>();
+          )cpp",
+          "class Foo<class X>",
+      },
+      {
+          R"cpp( // auto on initializer list.
+              namespace std
+              {
+                template<class _E>
+                class [[initializer_list]] {};
+              }
+
+              ^auto i = {1,2};
+          )cpp",
+          "class std::initializer_list<int>",
+      },
+      {
+          R"cpp( // auto in function return type with trailing return type
+            struct Foo {};
+            ^auto test() -> decltype(Foo()) {
+              return Foo();
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // decltype in trailing return type
+            struct Foo {};
+            auto test() -> ^decltype(Foo()) {
+              return Foo();
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // auto in function return type
+            struct Foo {};
+            ^auto test() {
+              return Foo();
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // auto& in function return type
+            struct Foo {};
+            ^auto& test() {
+              static Foo x;
+              return x;
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // auto* in function return type
+            struct Foo {};
+            ^auto* test() {
+              Foo *x;
+              return x;
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // const auto& in function return type
+            struct Foo {};
+            const ^auto& test() {
+              static Foo x;
+              return x;
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // decltype(auto) in function return (value)
+            struct Foo {};
+            ^decltype(auto) test() {
+              return Foo();
+            }
+          )cpp",
+          "struct Foo",
+      },
+      {
+          R"cpp( // decltype(auto) in function return (ref)
+            struct Foo {};
+            ^decltype(auto) test() {
+              static Foo x;
+              return (x);
+            }
+          )cpp",
+          "struct Foo &",
+      },
+      {
+          R"cpp( // decltype(auto) in function return (const ref)
+            struct Foo {};
+            ^decltype(auto) test() {
+              static const Foo x;
+              return (x);
+            }
+          )cpp",
+          "const struct Foo &",
+      },
+      {
+          R"cpp( // auto on alias
+            struct Foo {};
+            using Bar = Foo;
+            ^auto x = Bar();
+          )cpp",
+          // FIXME: it'd be nice if this resolved to the alias instead
+          "struct Foo",
+      },
   };
   for (Test T : Tests) {
     Annotations File(T.AnnotatedCode);
     auto AST = TestTU::withCode(File.code()).build();
     SourceManagerForFile SM("foo.cpp", File.code());
 
+    SCOPED_TRACE(File.code());
+    EXPECT_FALSE(File.points().empty());
     for (Position Pos : File.points()) {
       auto Location = sourceLocationInMainFile(SM.get(), Pos);
       ASSERT_TRUE(!!Location) << llvm::toString(Location.takeError());
       auto DeducedType = getDeducedType(AST.getASTContext(), *Location);
+      ASSERT_TRUE(DeducedType);
       EXPECT_EQ(DeducedType->getAsString(), T.DeducedType);
     }
   }
 }
 
 TEST(ClangdAST, GetQualification) {
   // Tries to insert the decl `Foo` into position of each decl named `insert`.
   // This is done to get an appropriate DeclContext for the insertion location.

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

const char *Tests[] = {
       R"cpp(// Declaration of partial template specialization
         template <typename T>
         struct $decl[[Foo]] {};
 
         template <typename T>
         struct Fo^o<T*> {};
       )cpp",
 
+      R"cpp(// auto builtin type (not supported)
+        ^auto x = 42;
+      )cpp",
+
+      R"cpp(// auto on lambda
+        auto x = [[[]]]{};
+        ^auto y = x;
+      )cpp",
+
+      R"cpp(// auto on struct
+        namespace ns1 {
+        struct [[S1]] {};
+        } // namespace ns1
+
+        ^auto x = ns1::S1{};
+      )cpp",
+
+      R"cpp(// decltype on struct
+        namespace ns1 {
+        struct [[S1]] {};
+        } // namespace ns1
+
+        ns1::S1 i;
+        ^decltype(i) j;
+      )cpp",
+
+      R"cpp(// decltype(auto) on struct
+        namespace ns1 {
+        struct [[S1]] {};
+        } // namespace ns1
+
+        ns1::S1 i;
+        ns1::S1& j = i;
+        ^decltype(auto) k = j;
+      )cpp",
+
+      R"cpp(// auto on template class
+        template<typename T> class [[Foo]] {};
+
+        ^auto x = Foo<int>();
+      )cpp",
+
+      R"cpp(// auto on template class with forward declared class
+        template<typename T> class [[Foo]] {};
+        class X;
+
+        ^auto x = Foo<X>();
+      )cpp",
+
+      R"cpp(// auto on specialized template class
+        template<typename T> class Foo {};
+        template<> class [[Foo]]<int> {};
+
+        ^auto x = Foo<int>();
+      )cpp",
+
+      R"cpp(// auto on initializer list.
+        namespace std
+        {
+          template<class _E>
+          class [[initializer_list]] {};
+        }
+
+        ^auto i = {1,2};
+      )cpp",
+
+      R"cpp(// auto function return with trailing type
+        struct [[Bar]] {};
+        ^auto test() -> decltype(Bar()) {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// decltype in trailing return type
+        struct [[Bar]] {};
+        auto test() -> ^decltype(Bar()) {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// auto in function return
+        struct [[Bar]] {};
+        ^auto test() {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// auto& in function return
+        struct [[Bar]] {};
+        ^auto& test() {
+          static Bar x;
+          return x;
+        }
+      )cpp",
+
+      R"cpp(// auto* in function return
+        struct [[Bar]] {};
+        ^auto* test() {
+          Bar* x;
+          return x;
+        }
+      )cpp",
+
+      R"cpp(// const auto& in function return
+        struct [[Bar]] {};
+        const ^auto& test() {
+          static Bar x;
+          return x;
+        }
+      )cpp",
+
+      R"cpp(// decltype(auto) in function return
+        struct [[Bar]] {};
+        ^decltype(auto) test() {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// decltype of function with trailing return type.
+        struct [[Bar]] {};
+        auto test() -> decltype(Bar()) {
+          return Bar();
+        }
+        void foo() {
+          ^decltype(test()) i = test();
+        }
+      )cpp",
+
       R"cpp(// Override specifier jumps to overridden method
         class Y { virtual void $decl[[a]]() = 0; };
         class X : Y { void a() ^override {} };
       )cpp",
 
       R"cpp(// Final specifier jumps to overridden method
         class Y { virtual void $decl[[a]]() = 0; };
         class X : Y { void a() ^final {} };