[clang] fix template argument conversion

Converted template arguments need to be converted again, if
the corresponding template parameter changed, as different
conversions might apply in that case.

Author: mizvekov

clang/docs/ReleaseNotes.rst

@@ -993,6 +993,9 @@ Bug Fixes to C++ Support
 - Fix immediate escalation not propagating through inherited constructors.  (#GH112677)
 - Fixed assertions or false compiler diagnostics in the case of C++ modules for
   lambda functions or inline friend functions defined inside templates (#GH122493).
+- Fix template argument checking so that converted template arguments are
+  converted again. This fixes some issues with partial ordering involving
+  template template parameters with non-type template parameters.
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^

clang/include/clang/AST/Expr.h

@@ -2262,6 +2262,16 @@ class UnaryOperator final
   }
 
 public:
+  enum OnStack_t { OnStack };
+  UnaryOperator(OnStack_t _, const ASTContext &Ctx, Expr *input, Opcode opc,
+                QualType type, ExprValueKind VK, ExprObjectKind OK,
+                SourceLocation l, bool CanOverflow,
+                FPOptionsOverride FPFeatures)
+      : UnaryOperator(Ctx, input, opc, type, VK, OK, l, CanOverflow,
+                      FPFeatures) {
+    assert(!FPFeatures.requiresTrailingStorage());
+  }
+
   static UnaryOperator *CreateEmpty(const ASTContext &C, bool hasFPFeatures);
 
   static UnaryOperator *Create(const ASTContext &C, Expr *input, Opcode opc,

clang/include/clang/Sema/Sema.h

@@ -11753,6 +11753,8 @@ class Sema final : public SemaBase {
   /// If an error occurred, it returns ExprError(); otherwise, it
   /// returns the converted template argument. \p ParamType is the
   /// type of the non-type template parameter after it has been instantiated.
+  /// \p Arg is the input template argument expression to be converted.
+  /// this expression may not necessarily outlive the converted result.
   ExprResult CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
                                    QualType InstantiatedParamType, Expr *Arg,
                                    TemplateArgument &SugaredConverted,

clang/lib/AST/TemplateBase.cpp

@@ -515,19 +515,17 @@ void TemplateArgument::print(const PrintingPolicy &Policy, raw_ostream &Out,
   }
 
   case Declaration: {
-    NamedDecl *ND = getAsDecl();
+    ValueDecl *VD = getAsDecl();
     if (getParamTypeForDecl()->isRecordType()) {
-      if (auto *TPO = dyn_cast<TemplateParamObjectDecl>(ND)) {
+      if (auto *TPO = dyn_cast<TemplateParamObjectDecl>(VD)) {
         TPO->getType().getUnqualifiedType().print(Out, Policy);
         TPO->printAsInit(Out, Policy);
         break;
       }
     }
-    if (auto *VD = dyn_cast<ValueDecl>(ND)) {
-      if (needsAmpersandOnTemplateArg(getParamTypeForDecl(), VD->getType()))
-        Out << "&";
-    }
-    ND->printQualifiedName(Out);
+    if (needsAmpersandOnTemplateArg(getParamTypeForDecl(), VD->getType()))
+      Out << "&";
+    VD->printQualifiedName(Out);
     break;
   }
 

clang/lib/Sema/SemaTemplate.cpp

@@ -5199,7 +5199,7 @@ convertTypeTemplateArgumentToTemplate(ASTContext &Context, TypeLoc TLoc) {
 }
 
 bool Sema::CheckTemplateArgument(
-    NamedDecl *Param, TemplateArgumentLoc &Arg, NamedDecl *Template,
+    NamedDecl *Param, TemplateArgumentLoc &ArgLoc, NamedDecl *Template,
     SourceLocation TemplateLoc, SourceLocation RAngleLoc,
     unsigned ArgumentPackIndex,
     SmallVectorImpl<TemplateArgument> &SugaredConverted,
@@ -5208,9 +5208,10 @@ bool Sema::CheckTemplateArgument(
     bool PartialOrderingTTP, bool *MatchedPackOnParmToNonPackOnArg) {
   // Check template type parameters.
   if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(Param))
-    return CheckTemplateTypeArgument(TTP, Arg, SugaredConverted,
+    return CheckTemplateTypeArgument(TTP, ArgLoc, SugaredConverted,
                                      CanonicalConverted);
 
+  const TemplateArgument &Arg = ArgLoc.getArgument();
   // Check non-type template parameters.
   if (NonTypeTemplateParmDecl *NTTP =dyn_cast<NonTypeTemplateParmDecl>(Param)) {
     // Do substitution on the type of the non-type template parameter
@@ -5252,63 +5253,114 @@ 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))
+        return true;
+      break;
+    }
+    case TemplateArgument::Declaration: {
+      bool needsAddrOf = false;
+      ValueDecl *VD = Arg.getAsDecl();
+      QualType ParamType = Arg.getParamTypeForDecl(), DeclType = VD->getType();
+      QualType DeclRefType;
+      auto handlePtrType = [&](auto *P) {
+        QualType PointeeType = P->getPointeeType();
+        if (!Context.hasSameType(P->getPointeeType(), DeclType))
+          return;
+        needsAddrOf = true;
+        DeclRefType = PointeeType;
+      };
+      if (auto *P = ParamType->getAs<PointerType>())
+        handlePtrType(P);
+      else if (auto *P = ParamType->getAs<MemberPointerType>())
+        handlePtrType(P);
+      else
+        DeclRefType = ParamType->castAs<ReferenceType>()->getPointeeType();
+
+      DeclRefExpr DRE(Context, Arg.getAsDecl(),
+                      /*RefersToEnclosingVariableOrCapture=*/false, DeclRefType,
+                      VK_LValue, SourceLocation());
+      if (needsAddrOf) {
+        UnaryOperator UOE(UnaryOperator::OnStack, Context, &DRE, UO_AddrOf,
+                          ParamType, VK_PRValue, OK_Ordinary, SourceLocation(),
+                          /*CanOverflow=*/false, /*FPFeatures=*/{});
+        if (!checkExpr(&UOE))
+          return true;
+      } else {
+        if (!checkExpr(&DRE))
+          return true;
+      }
+      break;
+    }
+    case TemplateArgument::NullPtr: {
+      CXXNullPtrLiteralExpr NPLE(Arg.getNullPtrType(), SourceLocation());
+      if (!checkExpr(&NPLE))
+        return true;
+      break;
+    }
     case TemplateArgument::StructuralValue:
-    case TemplateArgument::NullPtr:
-      // We've already checked this template argument, so just copy
-      // it to the list of converted arguments.
-      SugaredConverted.push_back(Arg.getArgument());
-      CanonicalConverted.push_back(
-          Context.getCanonicalTemplateArgument(Arg.getArgument()));
+      // FIXME: Is it even possible to reach here?
+      // If this is actually used, this needs to convert the argument again.
+      SugaredConverted.push_back(Arg);
+      CanonicalConverted.push_back(Context.getCanonicalTemplateArgument(Arg));
       break;
 
     case TemplateArgument::Template:
     case TemplateArgument::TemplateExpansion:
       // We were given a template template argument. It may not be ill-formed;
       // see below.
-      if (DependentTemplateName *DTN
-            = Arg.getArgument().getAsTemplateOrTemplatePattern()
-                                              .getAsDependentTemplateName()) {
+      if (DependentTemplateName *DTN = Arg.getAsTemplateOrTemplatePattern()
+                                           .getAsDependentTemplateName()) {
         // We have a template argument such as \c T::template X, which we
         // parsed as a template template argument. However, since we now
         // know that we need a non-type template argument, convert this
         // template name into an expression.
 
         DeclarationNameInfo NameInfo(DTN->getIdentifier(),
-                                     Arg.getTemplateNameLoc());
+                                     ArgLoc.getTemplateNameLoc());
 
         CXXScopeSpec SS;
-        SS.Adopt(Arg.getTemplateQualifierLoc());
+        SS.Adopt(ArgLoc.getTemplateQualifierLoc());
         // FIXME: the template-template arg was a DependentTemplateName,
         // so it was provided with a template keyword. However, its source
         // location is not stored in the template argument structure.
@@ -5319,8 +5371,8 @@ bool Sema::CheckTemplateArgument(
 
         // If we parsed the template argument as a pack expansion, create a
         // pack expansion expression.
-        if (Arg.getArgument().getKind() == TemplateArgument::TemplateExpansion){
-          E = ActOnPackExpansion(E.get(), Arg.getTemplateEllipsisLoc());
+        if (Arg.getKind() == TemplateArgument::TemplateExpansion) {
+          E = ActOnPackExpansion(E.get(), ArgLoc.getTemplateEllipsisLoc());
           if (E.isInvalid())
             return true;
         }
@@ -5340,8 +5392,8 @@ bool Sema::CheckTemplateArgument(
       // We have a template argument that actually does refer to a class
       // template, alias template, or template template parameter, and
       // therefore cannot be a non-type template argument.
-      Diag(Arg.getLocation(), diag::err_template_arg_must_be_expr)
-        << Arg.getSourceRange();
+      Diag(ArgLoc.getLocation(), diag::err_template_arg_must_be_expr)
+          << ArgLoc.getSourceRange();
       NoteTemplateParameterLocation(*Param);
 
       return true;
@@ -5357,8 +5409,8 @@ bool Sema::CheckTemplateArgument(
       //
       // We warn specifically about this case, since it can be rather
       // confusing for users.
-      QualType T = Arg.getArgument().getAsType();
-      SourceRange SR = Arg.getSourceRange();
+      QualType T = Arg.getAsType();
+      SourceRange SR = ArgLoc.getSourceRange();
       if (T->isFunctionType())
         Diag(SR.getBegin(), diag::err_template_arg_nontype_ambig) << SR << T;
       else
@@ -5409,34 +5461,33 @@ bool Sema::CheckTemplateArgument(
   //   When [the injected-class-name] is used [...] as a template-argument for
   //   a template template-parameter [...] it refers to the class template
   //   itself.
-  if (Arg.getArgument().getKind() == TemplateArgument::Type) {
+  if (Arg.getKind() == TemplateArgument::Type) {
     TemplateArgumentLoc ConvertedArg = convertTypeTemplateArgumentToTemplate(
-        Context, Arg.getTypeSourceInfo()->getTypeLoc());
+        Context, ArgLoc.getTypeSourceInfo()->getTypeLoc());
     if (!ConvertedArg.getArgument().isNull())
-      Arg = ConvertedArg;
+      ArgLoc = ConvertedArg;
   }
 
-  switch (Arg.getArgument().getKind()) {
+  switch (Arg.getKind()) {
   case TemplateArgument::Null:
     llvm_unreachable("Should never see a NULL template argument here");
 
   case TemplateArgument::Template:
   case TemplateArgument::TemplateExpansion:
-    if (CheckTemplateTemplateArgument(TempParm, Params, Arg, PartialOrdering,
+    if (CheckTemplateTemplateArgument(TempParm, Params, ArgLoc, PartialOrdering,
                                       MatchedPackOnParmToNonPackOnArg))
       return true;
 
-    SugaredConverted.push_back(Arg.getArgument());
-    CanonicalConverted.push_back(
-        Context.getCanonicalTemplateArgument(Arg.getArgument()));
+    SugaredConverted.push_back(Arg);
+    CanonicalConverted.push_back(Context.getCanonicalTemplateArgument(Arg));
     break;
 
   case TemplateArgument::Expression:
   case TemplateArgument::Type:
     // We have a template template parameter but the template
     // argument does not refer to a template.
-    Diag(Arg.getLocation(), diag::err_template_arg_must_be_template)
-      << getLangOpts().CPlusPlus11;
+    Diag(ArgLoc.getLocation(), diag::err_template_arg_must_be_template)
+        << getLangOpts().CPlusPlus11;
     return true;
 
   case TemplateArgument::Declaration:

clang/lib/Sema/TreeTransform.h

@@ -7335,8 +7335,10 @@ QualType TreeTransform<Derived>::TransformTemplateSpecializationType(
                                               NewTemplateArgs))
     return QualType();
 
-  // FIXME: maybe don't rebuild if all the template arguments are the same.
-
+  // This needs to be rebuilt if either the arguments changed, or if the
+  // original template changed. If the template changed, and even if the
+  // arguments didn't change, these arguments might not correspond to their
+  // respective parameters, therefore needing conversions.
   QualType Result =
     getDerived().RebuildTemplateSpecializationType(Template,
                                                    TL.getTemplateNameLoc(),

clang/test/SemaTemplate/cwg2398.cpp

@@ -663,58 +663,38 @@ namespace nttp_auto {
 
 namespace nttp_partial_order {
   namespace t1 {
-    // FIXME: This should pick the second overload.
     template<template<short> class TT1> void f(TT1<0>);
-    // new-note@-1 {{here}}
     template<template<int>   class TT2> void f(TT2<0>) {}
-    // new-note@-1 {{here}}
     template<int> struct B {};
     template void f<B>(B<0>);
-    // new-error@-1 {{ambiguous}}
   } // namespace t1
   namespace t2 {
-    // FIXME: This should pick the second overload.
     struct A {} a;
     template<template<A&>       class TT1> void f(TT1<a>);
-    // new-note@-1 {{here}}
     template<template<const A&> class TT2> void f(TT2<a>) {}
-    // new-note@-1 {{here}}
     template<const A&> struct B {};
     template void f<B>(B<a>);
-    // new-error@-1 {{ambiguous}}
   } // namespace t2
   namespace t3 {
-    // FIXME: This should pick the second overload.
     enum A {};
     template<template<A>   class TT1> void f(TT1<{}>);
-    // new-note@-1 {{here}}
     template<template<int> class TT2> void f(TT2<{}>) {}
-    // new-note@-1 {{here}}
     template<int> struct B {};
     template void f<B>(B<{}>);
-    // new-error@-1 {{ambiguous}}
   } // namespace t3
   namespace t4 {
-    // FIXME: This should pick the second overload.
     struct A {} a;
     template<template<A*>       class TT1> void f(TT1<&a>);
-    // new-note@-1 {{here}}
     template<template<const A*> class TT2> void f(TT2<&a>) {}
-    // new-note@-1 {{here}}
     template<const A*> struct B {};
     template void f<B>(B<&a>);
-    // new-error@-1 {{ambiguous}}
   } // namespace t4
   namespace t5 {
-    // FIXME: This should pick the second overload.
     struct A { int m; };
     template<template<int A::*>       class TT1> void f(TT1<&A::m>);
-    // new-note@-1 {{here}}
     template<template<const int A::*> class TT2> void f(TT2<&A::m>) {}
-    // new-note@-1 {{here}}
     template<const int A::*> struct B {};
     template void f<B>(B<&A::m>);
-    // new-error@-1 {{ambiguous}}
   } // namespace t5
   namespace t6 {
     // FIXME: This should pick the second overload.