[PATCH] D128826: Go-to-type on smart_ptr<Foo> now also shows Foo

[Sam McCall via Phabricator via cfe-commits]

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

LG, just nits you know of outstanding and I think no extra tests needed.

In D128826#3619956 <https://reviews.llvm.org/D128826#3619956>, @tom-anders wrote:

>>> So here's what I thought: Say you have a variable that's a smart pointer and trigger textDocument/typeDefinition on it and there's only a single result that takes you straight to the type. In this case, you might not even know that the type is actually wrapped inside a smart pointer and might make wrong assumptions about the code. Of course you could've done a Hover beforehand, but in the case where there are 2 results, you don't need that extra step.
>>
>> Yeah. There's no guarantee the type in question is well-represented by a decl, but this is a bit confusing.
>> I think it might still be worth it for specific types like `unique_ptr<Foo>`, but for `vector<Foo>` it's too surprising even if you want Foo 99% of the time.
>
> Right, maybe it would make sense to go straight to `T` for declarations, but include `smart_ptr` when it's used in an expression? But that's something for a follow-up patch.

Hmm, is the distinction that the type is written in the declaration (not `auto`)? I'm not sure it's worth specializing behavior for that scenario too much, when it's not too hard to go-to-definition on the specific token you want.
The chance of guessing right has to be weighed against keeping the behavior predictable and comprehensible...

> Agreed. The heuristic there would be "template class with at least 1 type parameter, where all other parameters except the first have a default value", right? 
> But maybe it even makes sense for stuff like `std::map`, so it's more like "template class, with `n>0` type template parameters, followed by `m>=0` defaulted template parameters"

Right, if we have type template parameters we should offer them in addition to the template.

Defaulted is tricky, actually. (And important for stuff like container allocators and string traits).

- the type sometimes has "as-written" sugar which will tell you the spelled args, but the spelled form isn't that interesting when you can't see the spelling (and sometimes the sugar is missing entirely)
- TypePrinter has some logic to determine whether the template arg is equivalent to the default (isSubstitutedDefaultArgument in TypePrinter.cpp), but would need some refactoring to expose it
- the simplistic option is just to suppress a template arg if the parameter has a default (without checking whether it is default or not). In the common case this is correct, in the uncommon case it is conservative.

> Do we already have an issue for this? If not, I'd open a new one.

I don't think so, the bug you found is the closest we have.



================
Comment at: clang-tools-extra/clangd/HeuristicResolver.h:75
+  // could look up the name appearing on the RHS.
+  const Type *getPointeeType(const Type *T) const;
+
----------------
tom-anders wrote:
> sammccall wrote:
> > tom-anders wrote:
> > > Not sure if it's the right call to make this public? The documentation needs to be adapted at least I think
> > It's a bit unfortunate that it doesn't fit the usual pattern of "resolve the target of this AST node", but it's fairly closely related, it needs to use the Ctx in the same way, and it's already implemented here... I think it's OK (at least I can't see a better alternative).
> > 
> > We should probably have a unit test for it though.
> Not really sure where to put the unit test, probably FindTargetTests.cpp? It looks like it tests `HeuristicResolver` implicitly. Couldn't find anything that tests `HeuristicResolver` directly.
Oh you're right - we don't have direct tests :-\
Well, this function already exists and we already have some indirect tests, let's just leave it as-is.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1913
 // to target.
-static QualType unwrapFindType(QualType T) {
+static llvm::SmallVector<QualType> unwrapFindType(
+    QualType T, const HeuristicResolver* H) {
----------------
tom-anders wrote:
> tom-anders wrote:
> > sammccall wrote:
> > > Ergonomically I might prefer `void unwrapFindType(QualType, ..., vector<QualType>& Out)` or so.
> > > This tends to compose a bit better IMO by making all the cases look similar whether you're adding one type or several or combining lists.
> > > 
> > > You can still `return Out.push_back(T)` etc on a single line.
> > Ah yes that makes sense. Didn't think about the `return Out.push_back(T)` trick to keep using early returns, thought I'd have to add a ton of `else if`s instead.
> It makes the call side a bit uglier though - Is it okay to add a convenience wrapper `vector<QualType> unwrapFindType(QualType, HeuristicResolver*)` that forwards to the overload that uses the out-parameter?
Just the final call, not the recursive calls, right? They can be `return unwrapFindType(...);` too.

It doesn't look ugly to me (2 extra lines at one callsite) but up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128826