[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

A few comments on the behavior & clangd changes. I can't say I understand enough about how deduction is implemented to give a proper review of the guts :-(

In general eliminating type-parameter-0-0 is great and retaining sugar gives us more options for display.
Thanks for updating clangd, I have some quibbles but feel free to land this in ~any form where the tests pass, there are no critical regressions and I'm happy to polish the behavior afterwards.

If I understand, a deduced decltype(auto) now desugars as `decltype(auto) -> decltype(foo) -> Foo`.
This seems principled, but at least clangd we usually call getDeducedType() to display the underlying type somehow, and `decltype(foo)` isn't terribly enlightening. (The asymmetry between `auto` and `decltype(auto)` is a bit unfortunate).
For clangd, unwrapping it in the `clangd::getDeducedType()` helper seems fine, so I only really mention this because I get the same feeling about some of the clang diagnostic messages.
It might be that the DeducedAsType for `decltype(auto)` should skip the DeclType (when possible, i.e. unless dependent), WDYT?



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:495
+                                               const QualType &Type,
+                                               bool Underlying = false) {
   const auto &SM = AST.getSourceManager();
----------------
(i'm confused by the optional parameter - there's only one caller)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:506
+      targetDecl(DynTypedNode::create(Type.getNonReferenceType()),
+                 DeclRelation::TemplatePattern | DeclRelation::Alias |
+                     (Underlying ? DeclRelation::Underlying : DeclRelation()),
----------------
`Alias | Underlying` means that go-to-definition will return both A and B in this case:

```
using A = int;
using B = A;
au^to x = B();
```

I don't think we particularly want that.

Really this change is just trying to make sure we can resolve `decltype(auto) -> decltype(foo) -> Foo` right? I think we should rather make clangd::getDeducedType(...) unwrap the decltype(foo) in the first place.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:449
   )cpp",
-                  ExpectedHint{": int &", "z"});
+                  ExpectedHint{": decltype(y)", "z"});
 }
----------------
this is a regression, again retaining sugar is good in general but we need to unwrap the deduced decltype.

(No need to fix this, I'm happy to do it as a followup)


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-cxx14.cpp:69
 auto multi1c = multi1a, multi1d = multi1b;
-decltype(auto) multi1e = multi1a, multi1f = multi1b; // expected-error {{'decltype(auto)' deduced as 'int' in declaration of 'multi1e' and deduced as 'int &' in declaration of 'multi1f'}}
+decltype(auto) multi1e = multi1a, multi1f = multi1b; // expected-error {{'decltype(auto)' deduced as 'decltype(multi1a)' (aka 'int') in declaration of 'multi1e' and deduced as 'decltype(multi1b)' (aka 'int &') in declaration of 'multi1f'}}
 
----------------
here's an example where I think retaining the `decltype(expr)` sugar probably adds more noise than insight


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216