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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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)



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:442
+  const auto &ASTContext = AST.getASTContext();
+  const NamedDecl *D = getPreferredDecl(Decls.front());
+
----------------
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


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


================
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]] {};
----------------
qchateau wrote:
> sammccall wrote:
> > 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.
> 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 ?
Keeping them around is fine too, we're not strict about this.

Thanks for adding all the tests for deduced types!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977