[PATCH] D111283: [clang] template / auto deduction deduces common sugar

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

rsmith added a comment.

Do you have examples showing what this does in practice for errors in real-world code? I'm concerned that stripping back to the common type sugar may produce an unintuitive result in some cases, although it certainly does seem at least theoretically nice to use a commutative and associative function to combine deductions like this. The kind of thing I'd be worried about would be (eg) a container where the `iterator` and `const_iterator` are the same, but where the common sugar of `T::iterator` and `T::const_iterator` are some internal type that the user has never heard of. In general it seems like this sugar stripping could result in types that are distant from the user's code. As an extreme example, in the case where one of the types is canonical and the other is not, it seems like stripping all the way back to the canonical type would be worse than ignoring the canonical version of the type and keeping the sugared version.

Perhaps we could treat canonical types as a special case (effectively assuming they came from taking a type and canonicalizing it at some point, rather than from a type the user wrote) and when combining two types, use the non-canonical type if one of them is canonical, and otherwise use the common sugar. That should still be commutative and associative and generally well-behaved, but won't degrade badly when given a canonical type as input. I think that still leaves the possibility of a surprising outcome when combining types that both desugar to some internal type, though I'm not sure what we can do about that other than make an arbitrary choice of which type sugar we like more.

I've not yet done any detailed review of the common type sugar computation mechanism.



================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:2008
+        DeduceAutoType(AllocTypeInfo->getTypeLoc(), Deduce, DeducedType, Info);
+    if (Result != TDK_Success && Result != TDK_MiscellaneousDeductionFailure)
       return ExprError(Diag(StartLoc, diag::err_auto_new_deduction_failure)
----------------
Previously we distinguished between "auto deduction failed and already issued a diagnostic" and "auto deduction failed but no diagnostic has been issued". This allowed us to be sure we always issue exactly one diagnostic for each deduction failure. We seem to have lost that distinction -- `TDK_MiscellaneousDeductionFailure` does not indicate that a diagnostic has already been issued, and if an auto deduction fails for this reason, we now might produce no diagnostics at all.


================
Comment at: clang/test/SemaCXX/sugared-auto.cpp:146
+    return a;
+  return N(); // expected-error {{but deduced as 'SARS (*)() throw(Man, Vibrio)' (aka 'void (*)() throw(Man, Vibrio)')}}
+}
----------------
Why don't we get `Virus` as the deduced return type from line 143 here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283