[llvm-branch-commits] [clang] [clang] fix template argument conversion (PR #124386)

[Matheus Izvekov via llvm-branch-commits]

================
@@ -5252,63 +5253,89 @@ bool Sema::CheckTemplateArgument(
         return true;
     }
 
-    switch (Arg.getArgument().getKind()) {
-    case TemplateArgument::Null:
-      llvm_unreachable("Should never see a NULL template argument here");
-
-    case TemplateArgument::Expression: {
-      Expr *E = Arg.getArgument().getAsExpr();
+    auto checkExpr = [&](Expr *E) -> Expr * {
       TemplateArgument SugaredResult, CanonicalResult;
       unsigned CurSFINAEErrors = NumSFINAEErrors;
       ExprResult Res =
           CheckTemplateArgument(NTTP, NTTPType, E, SugaredResult,
                                 CanonicalResult, PartialOrderingTTP, CTAK);
-      if (Res.isInvalid())
-        return true;
       // If the current template argument causes an error, give up now.
-      if (CurSFINAEErrors < NumSFINAEErrors)
-        return true;
+      if (Res.isInvalid() || CurSFINAEErrors < NumSFINAEErrors)
+        return nullptr;
+      SugaredConverted.push_back(SugaredResult);
+      CanonicalConverted.push_back(CanonicalResult);
+      return Res.get();
+    };
+
+    switch (Arg.getKind()) {
+    case TemplateArgument::Null:
+      llvm_unreachable("Should never see a NULL template argument here");
 
+    case TemplateArgument::Expression: {
+      Expr *E = Arg.getAsExpr();
+      Expr *R = checkExpr(E);
+      if (!R)
+        return true;
       // If the resulting expression is new, then use it in place of the
       // old expression in the template argument.
-      if (Res.get() != E) {
-        TemplateArgument TA(Res.get());
-        Arg = TemplateArgumentLoc(TA, Res.get());
+      if (R != E) {
+        TemplateArgument TA(R);
+        ArgLoc = TemplateArgumentLoc(TA, R);
       }
-
-      SugaredConverted.push_back(SugaredResult);
-      CanonicalConverted.push_back(CanonicalResult);
       break;
     }
 
-    case TemplateArgument::Declaration:
-    case TemplateArgument::Integral:
+    // As for the converted NTTP kinds, they still might need another
+    // conversion, as the new corresponding parameter might be different.
+    // Ideally, we would always perform substitution starting with sugared types
+    // and never need these, as we would still have expressions. Since these are
+    // needed so rarely, it's probably a better tradeoff to just convert them
+    // back to expressions.
+    case TemplateArgument::Integral: {
+      IntegerLiteral ILE(Context, Arg.getAsIntegral(), Arg.getIntegralType(),
+                         SourceLocation());
+      if (!checkExpr(&ILE))
----------------
mizvekov wrote:

Documenting on CheckTemplateArgument the fact the input expression might be temporary sounds good, will do.

Otherwise I don't think this is a FIXME situation though.

Keeping the expression around is mostly a function of preserving type sugar.

For example, the reason we hit this bug for these partial ordering test cases is because when checking for a non-deduced mismatch, we use canonical types instead of the sugared type as input to instantiation.
And the reason we do that is because there we can't deal with incomplete substitution on type sugar only, it would take some effort to make that work, but it is certainly fixable.

But since this is a best effort thing and it's very hard to prove we would never drop sugar which would take us here,
we might need to keep handling these cases.

One possible course of action would be to fix all these places which take us here, and make the test suite pass.
Once that is done, just mark all these cases unreachable.
Then we deploy that and let users report any missing cases, and keep fixing until there is nothing left.
If along the way we hit a case that is too complicated to be worth fixing, we give up and re-add these conversions.

https://github.com/llvm/llvm-project/pull/124386