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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.
Herald added a subscriber: carlosgalvezp.


================
Comment at: clang/include/clang/AST/Type.h:4968
   QualType getDeducedType() const {
-    return !isCanonicalUnqualified() ? getCanonicalTypeInternal() : QualType();
+    return DeducedAsType.isNull() ? QualType() : DeducedAsType;
   }
----------------



================
Comment at: clang/lib/AST/ASTImporter.cpp:3275
+            !DeducedT.isNull()
+                ? dyn_cast<RecordType>(DeducedT->getCanonicalTypeInternal())
+                : nullptr) {
----------------



================
Comment at: clang/lib/AST/ASTImporter.cpp:3285
   }
-  if (const TypedefType *TypedefT =
-          dyn_cast<TypedefType>(FromFPT->getReturnType())) {
-    TypedefNameDecl *TD = TypedefT->getDecl();
+  if (const auto *TypedefT = dyn_cast<TypedefType>(FromFPT->getReturnType())) {
+    const TypedefNameDecl *TD = TypedefT->getDecl();
----------------
(Preparing for D112374)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1154
   if (!ParamFunction || !ArgFunction)
     return Param == Arg;
 
----------------
The `==` comparisons here and below should presumably be `hasSameType` comparisons now that we're preserving sugar.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1234-1250
   llvm::SmallPtrSet<const RecordType *, 8> Visited;
   SmallVector<const RecordType *, 8> ToVisit;
   // We iterate over this later, so we have to use MapVector to ensure
   // determinism.
   llvm::MapVector<const RecordType *, SmallVector<DeducedTemplateArgument, 8>>
       Matches;
 
----------------
I think we should:

* track `ToVisit` as a vector of `QualType`, so we preserve the type sugar as written in base specifier lists
* track `Visited` as a set of (canonical) `const CXXRecordDecl*`s rather than relying on `RecordType`s always being canonical (they currently are, but I don't think that's intended to be guaranteed)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1385-1388
+    P = P.getUnqualifiedType();
     //     - If A is a cv-qualified type, A is replaced by the cv-unqualified
     //       version of A.
+    A = A.getUnqualifiedType();
----------------
Do we need to use `Context.getUnqualifiedArrayType` here, for cases like:
```
typedef const int CInt;
// Partial ordering should deduce `T = int` from both parameters here,
// even though `T = const int` might make more sense for the first one.
template<int N> void f(CInt (&)[N], int*);
template<int N, typename T> void f(T (&)[N], T*);
CInt arr[5];
void g() { f(arr, arr); }
```
? I think `getUnqualifiedType()` on `CInt[N]` will not remove the indirect `const` qualifier.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1437
     // top level, so they can be matched with the qualifiers on the parameter.
-    if (isa<ArrayType>(Arg)) {
+    if (isa<ArrayType>(A)) {
       Qualifiers Quals;
----------------
`A` could be sugar for an array type here.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1546-1568
+  QualType CanP = P.getCanonicalType();
+  // If the parameter type is not dependent, there is nothing to deduce.
+  if (!P->isDependentType()) {
+    if (TDF & TDF_SkipNonDependent)
       return Sema::TDK_Success;
+
+    QualType CanA = A.getCanonicalType();
----------------
I think we can excise `CanA` and `CanP` entirely here so people aren't tempted to use them and lose sugar.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1643
 
-      return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+      return ParDesug == ArgDesug ? Sema::TDK_Success
+                                  : Sema::TDK_NonDeducedMismatch;
----------------
mizvekov wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > This looks wrong to me: we should be comparing the types, not how they're written. `Context.hasSameType(Param, Arg)` (or `Context.hasSameUnqualifiedType(Param, Arg)` in the `TDF_IgnoreQualifiers` case) would be appropriate here.
> > You are right, but the reason we don't get into any troubles here is because this is dead code anyway, the non-dependent case will always be handled above :)
> > 
> > Although perhaps, I wonder if we should dig down into non-dependent types anyway, in case the types are too complex and it's not immediately obvious what does not match, we could perhaps improve the diagnostic?
> > 
> > I will experiment a little bit with this idea.
> Here are the results of this experiment:
> ```
> error: 'note' diagnostics expected but not seen:
>   File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: could not match 'void (I) noexcept(false)' (aka 'void (int) noexcept(false)') against 'void (int) noexcept'
> error: 'note' diagnostics seen but not expected:
>   File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: candidate template ignored: failed template argument deduction
> 
> error: 'note' diagnostics expected but not seen:
>   File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template ignored: could not match 'auto ()' against 'int ()'
>   File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template ignored: could not match 'auto ()' against 'void ()'
> error: 'note' diagnostics seen but not expected:
>   File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template ignored: could not match 'auto' against 'int'
>   File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template ignored: could not match 'auto' against 'void'
> 
> error: 'note' diagnostics expected but not seen:
>   File SemaCXX\pass-object-size.cpp Line 62 (directive at SemaCXX\pass-object-size.cpp:69): candidate address cannot be taken because parameter 1 has pass_object_size attribute
>   File SemaCXX\pass-object-size.cpp Line 56 (directive at SemaCXX\pass-object-size.cpp:74): candidate address cannot be taken because parameter 1 has pass_object_size attribute
>   File SemaCXX\pass-object-size.cpp Line 62 (directive at SemaCXX\pass-object-size.cpp:75): candidate address cannot be taken because parameter 1 has pass_object_size attribute
> error: 'note' diagnostics seen but not expected:
>   File SemaCXX\pass-object-size.cpp Line 56: candidate template ignored: failed template argument deduction
>   File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored: failed template argument deduction
>   File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored: failed template argument deduction
> 
> error: 'note' diagnostics expected but not seen:
>   File SemaTemplate\deduction.cpp Line 316: deduced non-type template argument does not have the same type as the corresponding template parameter ('std::nullptr_t' vs 'int *')
>   File SemaTemplate\deduction.cpp Line 323: values of conflicting types
> error: 'note' diagnostics seen but not expected:
>   File SemaTemplate\deduction.cpp Line 275: candidate template ignored: could not match 'const int' against 'int'
>   File SemaTemplate\deduction.cpp Line 316: candidate template ignored: could not match 'int *' against 'std::nullptr_t'
>   File SemaTemplate\deduction.cpp Line 323: candidate template ignored: could not match 'int *' against 'std::nullptr_t'
> 
> error: 'note' diagnostics expected but not seen:
>   File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template ignored: could not match 'void ()' against 'void (float *)'
>   File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template ignored: could not match 'void (int *)' against 'void (float *)'
> error: 'note' diagnostics seen but not expected:
>   File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template ignored: could not match 'int' against 'float'
>   File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template ignored: failed template argument deduction
> ```
> 
> It's interesting to note that it reveals several cases we give too generic 'failed template argument deduction' errors, like the different noexcept specifications, function prototypes with different amount of parameters, and the 'pass_object_size attribute' case.
Hm, this doesn't seem to be an improvement; I think it's better to present the larger non-matching types and use `%diff` in the diagnostic to highlight the part that differs. Especially because we don't provide a source location for (eg) where in the type we found a `float`, providing the more complete context of (eg) `void (float*)` seems more useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216