[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

684

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

685

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)

688

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 ↗

(On Diff #310872)

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

688

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 ↗

(On Diff #310872)

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

688

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 ↗

(On Diff #310872)

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))
+  auto TokensTouchingCursor =
+      syntax::spelledTokensTouching(*CurLoc, AST.getTokens());
+  for (const auto &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)};
 
       ASTNodeKind NodeKind;
-  auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
-                                      *MainFilePath, Index, &NodeKind);
+      auto ASTResults = locateASTReferent(*CurLoc, &Tok, AST, *MainFilePath,
+                                          Index, &NodeKind);
       if (!ASTResults.empty())
         return ASTResults;
+    } else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+      if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+        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)};
+      }
+    }
+  }
 
   // If the cursor can't be resolved directly, try fallback strategies.
   auto Word =
       SpelledWord::touching(*CurLoc, AST.getTokens(), AST.getLangOpts());
   if (Word) {
     // Is the same word nearby a real identifier that might refer to something?
     if (const syntax::Token *NearbyIdent =
             findNearbyIdentifier(*Word, AST.getTokens())) {
       if (auto Macro = locateMacroReferent(*NearbyIdent, AST, *MainFilePath)) {
         log("Found macro definition heuristically using nearby identifier {0}",
             Word->Text);
         return {*std::move(Macro)};
       }
-      ASTResults =
+      auto ASTResults =
           locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
                             *MainFilePath, Index, /*NodeKind=*/nullptr);
       if (!ASTResults.empty()) {
         log("Found definition heuristically using nearby identifier {0}",
             NearbyIdent->text(SM));
         return ASTResults;
       } else {
         vlog("No definition found using nearby identifier {0} at {1}",
              Word->Text, Word->Location.printToString(SM));
       }
     }
     // No nearby word, or it didn't refer to anything either. Try the index.
+    ASTNodeKind NodeKind;
     auto TextualResults =
         locateSymbolTextually(*Word, AST, Index, *MainFilePath, NodeKind);
     if (!TextualResults.empty())
       return TextualResults;
   }
 
   return {};
 }