[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

685

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

686

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)

689

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

669

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

689

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

669

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

442

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

689

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

669

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/Hover.cpp

 //===--- Hover.cpp - Information about code at the cursor location --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 }
 
 /// Generate a \p Hover object given the type \p T.
 HoverInfo getHoverContents(QualType T, ASTContext &ASTCtx,
                            const SymbolIndex *Index) {
   HoverInfo HI;
 
   if (const auto *D = T->getAsTagDecl()) {
-    HI.Name = printName(ASTCtx, *D);
-    HI.Kind = index::getSymbolInfo(D).Kind;
-
-    const auto *CommentD = getDeclForComment(D);
-    HI.Documentation = getDeclComment(ASTCtx, *CommentD);
-    enhanceFromIndex(HI, *CommentD, Index);
+    return getHoverContents(D, Index);
   } else {
     // Builtin types
     auto Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
     Policy.SuppressTagKeyword = true;
     HI.Name = T.getAsString(Policy);
   }
   return HI;
 }

clang-tools-extra/clangd/XRefs.cpp

 //===--- XRefs.cpp -----------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "XRefs.h"
llvm::StringRef sourcePrefix(SourceLocation Loc, const SourceManager &SM) {
   bool Invalid = false;
   llvm::StringRef Buf = SM.getBufferData(D.first, &Invalid);
   if (Invalid || D.second > Buf.size())
     return "";
   return Buf.substr(0, D.second);
 }
 
 bool isDependentName(ASTNodeKind NodeKind) {
   return NodeKind.isSame(ASTNodeKind::getFromNodeKind<OverloadExpr>()) ||
          NodeKind.isSame(
              ASTNodeKind::getFromNodeKind<CXXDependentScopeMemberExpr>()) ||
          NodeKind.isSame(
              ASTNodeKind::getFromNodeKind<DependentScopeDeclRefExpr>());
 }
 
 } // namespace
 
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) {
+      if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+        Deduced = Deduced->getNonReferenceType();
+        const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+        if (!D)
+          continue;
+
+        D = getPreferredDecl(D);
+        auto Loc = makeLocation(AST.getASTContext(), nameLocation(*D, SM),
+                                *MainFilePath);
+        if (!Loc)
+          continue;
+
+        LocatedSymbol LocSym;
+        LocSym.Name = printName(AST.getASTContext(), *D);
+        LocSym.PreferredDeclaration = *Loc;
+        if (const NamedDecl *Def = getDefinition(D))
+          LocSym.Definition = makeLocation(
+              AST.getASTContext(), nameLocation(*Def, SM), *MainFilePath);
+
+        return {std::move(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/HoverTests.cpp

 //===-- HoverTests.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
class Foo {})cpp";
       {R"cpp(
         void foo() {
           [[au^to]] lamb = []{};
         }
         )cpp",
        [](HoverInfo &HI) {
          HI.Name = "(lambda)";
          HI.Kind = index::SymbolKind::Class;
+         HI.NamespaceScope = "";
+         HI.LocalScope = "foo::";
+         HI.Definition = "class {}";
        }},
       // auto on template instantiation
       {R"cpp(
         template<typename T> class Foo{};
         void foo() {
           [[au^to]] x = Foo<int>();
         }
         )cpp",
        [](HoverInfo &HI) {
          HI.Name = "Foo<int>";
          HI.Kind = index::SymbolKind::Class;
+         HI.NamespaceScope = "";
+         HI.Definition = "template <> class Foo<int> {}";
        }},
       // auto on specialized template
       {R"cpp(
         template<typename T> class Foo{};
         template<> class Foo<int>{};
         void foo() {
           [[au^to]] x = Foo<int>();
         }
         )cpp",
        [](HoverInfo &HI) {
          HI.Name = "Foo<int>";
          HI.Kind = index::SymbolKind::Class;
+         HI.NamespaceScope = "";
+         HI.Definition = "template <> class Foo<int> {}";
        }},
 
       // macro
       {R"cpp(
         // Best MACRO ever.
         #define MACRO(x,y,z) void foo(x, y, z);
         [[MAC^RO]](int, double d, bool z = false);
         )cpp",
class Foo {})cpp";
           class X;
           void foo() {
             [[^auto]] x = Foo<X>();
           }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Foo<X>";
             HI.Kind = index::SymbolKind::Class;
+            HI.NamespaceScope = "";
+            HI.Definition = "template <> class Foo<X> {}";
           }},
       {// Falls back to primary template, when the type is not instantiated.
        R"cpp(
           // comment from primary
           template <typename T> class Foo {};
           // comment from specialization
           template <typename T> class Foo<T*> {};
           void foo() {
const std::function<void(HoverInfo &)> ExpectedBuilder;
             }
             void foo() {
               ^[[auto]] i = {1,2};
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "initializer_list<int>";
             HI.Kind = index::SymbolKind::Class;
+            HI.NamespaceScope = "std::";
+            HI.Definition = "template <> class initializer_list<int> {}";
           }},
       {
           R"cpp(// User defined conversion to auto
             struct Bar {
               operator ^[[auto]]() const { return 10; }
             };
           )cpp",
           [](HoverInfo &HI) { HI.Name = "int"; }},
const std::function<void(HoverInfo &)> ExpectedBuilder;
             struct Bar {};
             ^[[auto]] test() -> decltype(Bar()) {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "auto function return with trailing type";
           }},
       {
           R"cpp(// trailing return type
             struct Bar {};
             auto test() -> ^[[decltype]](Bar()) {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "trailing return type";
           }},
       {
           R"cpp(// auto in function return
             struct Bar {};
             ^[[auto]] test() {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "auto in function return";
           }},
       {
           R"cpp(// auto& in function return
             struct Bar {};
             ^[[auto]]& test() {
               static Bar x;
               return x;
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "auto& in function return";
           }},
       {
           R"cpp(// auto* in function return
             struct Bar {};
             ^[[auto]]* test() {
               Bar* bar;
               return bar;
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "auto* in function return";
           }},
       {
           R"cpp(// const auto& in function return
             struct Bar {};
             const ^[[auto]]& test() {
               static Bar x;
               return x;
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "const auto& in function return";
           }},
       {
           R"cpp(// decltype(auto) in function return
             struct Bar {};
             ^[[decltype]](auto) test() {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation = "decltype(auto) in function return";
           }},
       {
           R"cpp(// decltype(auto) reference in function return
             ^[[decltype]](auto) test() {
               static int a;
               return (a);
             }
const std::function<void(HoverInfo &)> ExpectedBuilder;
             }
             void foo() {
               ^[[decltype]](test()) i = test();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct Bar {}";
             HI.Documentation =
                 "decltype of function with trailing return type.";
           }},
       {
           R"cpp(// decltype of var with decltype.
             void foo() {
               int I = 0;
               decltype(I) J = I;
const std::function<void(HoverInfo &)> ExpectedBuilder;
           R"cpp(// auto on alias
           struct cls {};
           typedef cls cls_type;
           ^[[auto]] y = cls_type();
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "cls";
             HI.Kind = index::SymbolKind::Struct;
+            HI.NamespaceScope = "";
+            HI.Definition = "struct cls {}";
             HI.Documentation = "auto on alias";
           }},
       {
-          R"cpp(// auto on alias
+          R"cpp(// auto on template
           template <class>
           struct templ {};
           ^[[auto]] z = templ<int>();
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "templ<int>";
             HI.Kind = index::SymbolKind::Struct;
-            HI.Documentation = "auto on alias";
+            HI.NamespaceScope = "";
+            HI.Definition = "template <> struct templ<int> {}";
+            HI.Documentation = "auto on template";
           }},
       {
           R"cpp(// should not crash.
           template <class T> struct cls {
             int method();
           };
 
           auto test = cls<int>().[[m^ethod]]();

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

 //===-- XRefsTests.cpp  ---------------------------*- C++ -*--------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "Annotations.h"
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)
+        au^to x = 42;
+      )cpp",
+
+      R"cpp(// auto on lambda
+        auto x = [[[]]]{};
+        au^to y = x;
+      )cpp",
+
+      R"cpp(// auto on struct
+        namespace ns1 {
+        struct [[S1]] {};
+        } // namespace ns1
+
+        au^to x = ns1::S1{};
+      )cpp",
+
+      R"cpp(// decltype on struct
+        namespace ns1 {
+        struct [[S1]] {};
+        } // namespace ns1
+
+        ns1::S1 i;
+        decl^type(i) j;
+      )cpp",
+
+      R"cpp(// decltype(auto) on struct
+        namespace ns1 {
+        struct [[S1]] {};
+        } // namespace ns1
+
+        ns1::S1 i;
+        ns1::S1& j = i;
+        decl^type(auto) k = j;
+      )cpp",
+
+      R"cpp(// auto on template class
+        template<typename T> class [[Foo]] {};
+
+        au^to x = Foo<int>();
+      )cpp",
+
+      R"cpp(// auto on template class with forward declared class
+        template<typename T> class [[Foo]] {};
+        class X;
+
+        au^to x = Foo<X>();
+      )cpp",
+
+      R"cpp(// auto on specialized template class
+        template<typename T> class Foo {};
+        template<> class [[Foo]]<int> {};
+
+        au^to x = Foo<int>();
+      )cpp",
+
+      R"cpp(// auto on initializer list.
+        namespace std
+        {
+          template<class _E>
+          class [[initializer_list]] {};
+        }
+
+        au^to i = {1,2};
+      )cpp",
+
+      R"cpp(// auto function return with trailing type
+        struct [[Bar]] {};
+        au^to test() -> decltype(Bar()) {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// decltype in trailing return type
+        struct [[Bar]] {};
+        auto test() -> decl^type(Bar()) {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// auto in function return
+        struct [[Bar]] {};
+        au^to test() {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// auto& in function return
+        struct [[Bar]] {};
+        au^to& test() {
+          static Bar x;
+          return x;
+        }
+      )cpp",
+
+      R"cpp(// auto* in function return
+        struct [[Bar]] {};
+        au^to* test() {
+          Bar* x;
+          return x;
+        }
+      )cpp",
+
+      R"cpp(// const auto& in function return
+        struct [[Bar]] {};
+        const au^to& test() {
+          static Bar x;
+          return x;
+        }
+      )cpp",
+
+      R"cpp(// decltype(auto) in function return
+        struct [[Bar]] {};
+        decl^type(auto) test() {
+          return Bar();
+        }
+      )cpp",
+
+      R"cpp(// decltype of function with trailing return type.
+        struct [[Bar]] {};
+        auto test() -> decltype(Bar()) {
+          return Bar();
+        }
+        void foo() {
+          decl^type(test()) i = test();
+        }
+      )cpp",
+
+      R"cpp(// auto on alias
+        struct [[cls]] {};
+        typedef cls cls_type;
+        au^to y = cls_type();
+      )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 {} };