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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3032626 <https://reviews.llvm.org/D110216#3032626>, @v.g.vassilev wrote:

> Thanks for working on this!How hard would it be to support:
>
>   using size_t = __SIZE_TYPE__;
>   template<typename T> struct Id { typedef T type; };
>   int main() {
>     struct S {} s;
>     Id<size_t>::type f = s; // just 'unsigned long', 'size_t' sugar has been lost
>   }

Actually supporting that is in my radar :)

Getting the basic idea up is not too complicated.
We need to support 'alias' template specializations which can carry non-canonical arguments,
and they would just point to the canonical template specializations.
We would then have to implement our desugaring of 'SubstTemplateTypeParmType' so it's able to access some context where it can get the correctly sugared ReplacementType for the given replaced 'TemplateTypeParmType'.

But there are some little details that need to be implemented so that it is reasonably usable.
For example, in the general case we cannot currently, for any given declaration, figure out the template arguments used to instantiate it. We can do it for class and function templates, but not for alias and variable templates.
We need to make anything that carries template arguments also be a declaration context, so we can walk up from any declaration and see those.

There is also the trouble that some elements are not properly parented. For example in a requires clause:
`template <class T> requires WHATEVER struct X {}`, WHATEVER is not currently parented to the class template, so we would not be able to recover the instantiation arguments there unless we fixed it.

And who knows what other non-obvious problems might be lurking :)

A lot of what I said also overlaps with what we need to properly support lazy substitution, such as required by C++20 concepts, which is also something I am working on.

Getting the 'template argument deduction' machinery to preserve the sugar, which we do in this patch, certainly benefits when we get there.

In D110216#3031531 <https://reviews.llvm.org/D110216#3031531>, @rsmith wrote:

> Thanks, this is very exciting, and the diagnostic changes in the test suite look great!

Thank you, much appreciated, and also thanks for the review!



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp:276
+
+auto NotOkay5 = []() { U u; return u.getBadLambda(); }();
+// CHECK-EXCEPTIONS: :[[@LINE-1]]:6: warning: initialization of 'NotOkay5' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
----------------
rsmith wrote:
> What happened here? It looks like nothing this expression does can actually throw. (This lambda's `operator()` isn't `noexcept`, but the functions it calls all are.) Was this only avoiding a false positive by accident before?
I haven't debugged it, I was hoping @aaron.ballman  would know something about it and it would save me the trouble :)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:250
         Context.hasSameType(X.getAsType(), Y.getAsType()))
       return X;
 
----------------
rsmith wrote:
> I wonder if we can come up with a good heuristic for determining which of `X` and `Y` we want to return here. One thing that immediately springs to mind is: if either type is canonical, we should return the other one.
I was thinking about the opposite actually, something along the lines of returning the common sugar between them. Starting from the cannonical type and going 'backwards' (adding back the sugar), we return the type just before adding sugar back would make them different.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1051-1052
+              DeduceTemplateArgumentsByTypeMatch(
+                  S, TemplateParams, Params[ParamIdx].getUnqualifiedType(),
+                  Args[ArgIdx].getUnqualifiedType(), Info, Deduced, TDF,
+                  PartialOrdering,
----------------
rsmith wrote:
> Hm, do we need the `getUnqualifiedType()` calls here? I'd expect that we'd have the `TDF_IgnoreQualifiers` flag set in this case, so we should be ignoring qualifiers anyway. Perhaps `DeduceTemplateArgumentsByTypeMatch` should be removing qualifiers itself if that flag is set?
The case I am worried about is checking the parameters in the function prototype case, and `TDF_IgnoreQualifiers` is neither set there, nor it would help if it was set anyway.
I will take a look again, but this was the simplest solution I could come up at the time.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1811-1812
+              S, TemplateParams,
+              FunctionProtoParam->getReturnType().getUnqualifiedType(),
+              FunctionProtoArg->getReturnType().getUnqualifiedType(), Info,
+              Deduced, 0,
----------------
rsmith wrote:
> Ignoring qualifiers here doesn't seem right to me. For example:
> ```
> template<typename T> void f(T(T));
> const int g(int);
> void h() { f(g); }
> ```
> ... should result in a deduction failure, because we should get `T = const int` from the return type but `T = int` from the parameter type.
Right, I must have simply assumed we had the same situation here with regards to function prototype parameters.

This feels like a standard defect to me, we should not be looking at top level qualifiers on return types :)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2306
       return Sema::TDK_Success;
-  }
+    }
 
----------------
rsmith wrote:
> Presumably this indentation change is unintended?
arc lint decided to add it automatically, and I decided not to fight it :-)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2378-2389
+    auto *E = Param.getAsExpr();
+    if (!E->isValueDependent()) {
+      if (const auto Int = E->getIntegerConstantExpr(S.Context)) {
+        if (Arg.getKind() == TemplateArgument::Integral) {
+          if (hasSameExtendedValue(*Int, Arg.getAsIntegral()))
+            return Sema::TDK_Success;
+        }
----------------
rsmith wrote:
> This suggests that we're passing in unresolved template arguments to this function. That seems problematic to me: those unresolved template arguments (as-written) will not be properly converted to the template's parameter type, will miss default arguments, and might not have had arguments properly combined into packs.
> 
> For *type* template arguments, I think it's fine to pass the argument as written if we're sure we know what it is, because basically no conversions apply for type template parameters and there may be meaningful sugar we want to preserve, but otherwise I think we should be passing in the resolved template argument.
Sure, I will revert that, presumably because this would be a separate can of worms we should not deal in this patch, but ideally we would get back to this so we can actually see some sugar here?


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4831
   QualType FuncParam =
-      SubstituteDeducedTypeTransform(*this, TemplArg, /*UseTypeSugar*/false)
+      SubstituteDeducedTypeTransform(*this, TemplArg, /*UseTypeSugar*/ true)
           .Apply(Type);
----------------
rsmith wrote:
> Hm, I see, adding the type sugar here presumably means that we get better diagnostic quality: we can complain that we can't deduce (eg) `auto*` against `int` instead of complaining that we can't deduce `type-parameter-0-0*` against `int`, right?
> 
> Can we remove the `UseTypeSugar` flag entirely now, or is it used for something else too?
The primary reason for it was moving in the direction of removing this flag.
We don't need it anymore in this case, although there are other users of it that need to be cleaned up. This one was just flipping the flag here, so I said why the heck not.
But other users are a bit more complicated, and I was afraid the patch was getting too big.

This could help improving the diagnostics in the future I suppose, but right now we are not using the result of the template argument deduction here very much, just for inconsistent deduction from initializer lists, the rest we defer to a generic `variable 'foo' with type 'bar' has incompatible initializer of type 'baz'`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216