[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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)



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:685
+
+    if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+      if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
----------------
This deserves a high-level comment: // go-to-definition on auto should find the definition of the deduced type, if possible


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:686
+    if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+      if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+        Deduced = Deduced->getNonReferenceType();
----------------
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)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+        const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+        if (!D)
+          continue;
----------------
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)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:669
+
+      R"cpp(// auto on template class with forward declared class
+        template<typename T> class [[Foo]] {};
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92977/new/

https://reviews.llvm.org/D92977