[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D72498#1813989 <https://reviews.llvm.org/D72498#1813989>, @ilya-biryukov wrote:

> In D72498#1813962 <https://reviews.llvm.org/D72498#1813962>, @sammccall wrote:
>
> > It's particularly unclear to me why typeprinter descends into auto but prints decltype, but Kadir says that seems to be intentional.
>
>
> Also don't see why they choose to have this inconsistency and I haven't seen any indication it's not a coincidence.
>  @kadircet, why do you think it's intentional? Should we put some comments there?


I was saying that because `TypePrinter` deliberately prints `decltype(` and then the expression.
I suppose we could make this a printing policy option to print either the underlying expr or type.

> I tend to disagree here. decltype is normally the last resort, so whatever it produces is probably super-obscure, would even expect it to be not representable in C++ in many cases.

I was rather talking about the obscurity of the expression inside decltype vs the typedef alias. I believe it is a lot harder to make any assumptions on `decltype(callback)` compared to `IntMap` without seeing the underlying type.

> Would definitely be helpful. If you feel we have some room in hover, I would love to have that. But there's a balance to be made, see Sam's comments about canonical types being obscure. I agree on 50% of the cases :-)

I think this should be OK to spend some space, as it will only show up when needed. I believe `better` printing of canonical types is a different problem we should definitely solve.

@sammccall wrote:

> It's a contrived example that makes clangd look silly (why decltype(a) instead of int?) but also the user look silly (why hover the variable rather than the decltype?).


Right, the user can definitely hover over the decltype, but this goes against our objectives. I believe with hover we are trying to reduce number of jumps a user needs to take for acquiring some info,
and requiring them to jump to the definition of symbol for figuring out the type doesn't seem right.

> But note that this patch doesn't handle the important use case of decltype as return type, really!
>  imagine cout << sum('c', 4), and you want to know what type the function call has.
>  The natural thing is to hover over the function call, you'll see "function sum<char, int> -> decltype(t1 + t2)" which is indeed pretty useless.
>  But this patch doesn't fix that behaviour, it only handles types of declarations.

Yes, but this was an oversight rather than an explicit decision. I believe we should definitely do the same for return type and even parameter types.

> @ilya-biryukov @kadircet what do you think about unwrapping decltype only when it's a return value (optional: of a function whose leading return type is auto) to narrowly catch this idiom?

I wouldn't special case that behavior as it is equally useful for ValueDecls, since we are trying to get rid of the extra jump as mentioned above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498