[clang] fix template argument conversion #124386
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mizvekov
requested review from
cor3ntin,
dmpolukhin,
erichkeane,
sdkrystian,
zygoloid and
zyn0217
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesConverted template arguments need to be converted again, if the corresponding template parameter changed, as different conversions might apply in that case. Full diff: https://github.com/llvm/llvm-project/pull/124386.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b89d055304f4a6..27574924a14a92 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/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
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 210df2836eeb07..62c45f15dec54e 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/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,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))
+ return true;
+ break;
+ }
+ case TemplateArgument::Declaration: {
+ DeclRefExpr DRE(Context, Arg.getAsDecl(),
+ /*RefersToEnclosingVariableOrCapture=*/false,
+ Arg.getParamTypeForDecl().getNonReferenceType(),
+ VK_LValue, SourceLocation());
+ 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 +5346,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 +5367,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 +5384,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 +5436,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:
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 12680843a434a0..81e351af575a96 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/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(),
diff --git a/clang/test/SemaTemplate/cwg2398.cpp b/clang/test/SemaTemplate/cwg2398.cpp
index 888ae59f687ef5..84296f55182818 100644
--- a/clang/test/SemaTemplate/cwg2398.cpp
+++ b/clang/test/SemaTemplate/cwg2398.cpp
@@ -655,25 +655,17 @@ 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.
|
zyn0217
approved these changes
Jan 25, 2025
Contributor
zyn0217
left a comment
zyn0217
left a comment
There was a problem hiding this comment.
Generally looks good, but please give others a chance to take a look.
cor3ntin
reviewed
Jan 25, 2025
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-nttp-narrowing
branch
from
1d96050 to
f2df46d
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-fix-reconvert-converted-template-arguments
branch
2 times, most recently
from
0f821a2 to
b46ef4d
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-nttp-narrowing
branch
from
f2df46d to
8210ac8
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-fix-reconvert-converted-template-arguments
branch
from
b46ef4d to
4f4e658
Compare
Contributor
Author
|
I just pushed changes to fix pointer / member pointer NTTP test cases. |
mizvekov
force-pushed
the
users/mizvekov/clang-fix-reconvert-converted-template-arguments
branch
from
4f4e658 to
7b4befe
Compare
cor3ntin
approved these changes
Jan 27, 2025
Contributor
|
This might be good enough for now.
|
erichkeane
reviewed
Jan 27, 2025
Collaborator
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
LGTM pending @cor3ntin 's leak concerns.
Base automatically changed from
users/mizvekov/clang-cwg2398-nttp-narrowing
to
main
January 28, 2025 18:51
Converted template arguments need to be converted again, if the corresponding template parameter changed, as different conversions might apply in that case.
mizvekov
force-pushed
the
users/mizvekov/clang-fix-reconvert-converted-template-arguments
branch
from
7b4befe to
8cf1056
Compare
mizvekov
deleted the
users/mizvekov/clang-fix-reconvert-converted-template-arguments
branch
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/12609 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Converted template arguments need to be converted again, if the corresponding template parameter changed, as different conversions might apply in that case.