Skip to content

[clang] disallow narrowing when matching template template parameters #124313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 28, 2025
Merged

[clang] disallow narrowing when matching template template parameters #124313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,9 @@ C++17 Feature Support
^^^^^^^^^^^^^^^^^^^^^
- The implementation of the relaxed template template argument matching rules is
more complete and reliable, and should provide more accurate diagnostics.
This implements:
- `P3310R5: Solving issues introduced by relaxed template template parameter matching <https://wg21.link/p3310r5>`_.
- `P3579R0: Fix matching of non-type template parameters when matching template template parameters <https://wg21.link/p3579r0>`_.

Resolutions to C++ Defect Reports
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
4 changes: 2 additions & 2 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,11 @@ def err_typecheck_converted_constant_expression_indirect : Error<
"conversion from %0 to %1 in converted constant expression would "
"bind reference to a temporary">;
def err_expr_not_cce : Error<
"%select{case value|enumerator value|non-type template argument|"
"%select{case value|enumerator value|non-type template argument|non-type parameter of template template parameter|"
"array size|explicit specifier argument|noexcept specifier argument|"
"call to 'size()'|call to 'data()'}0 is not a constant expression">;
def ext_cce_narrowing : ExtWarn<
"%select{case value|enumerator value|non-type template argument|"
"%select{case value|enumerator value|non-type template argument|non-type parameter of template template parameter|"
"array size|explicit specifier argument|noexcept specifier argument|"
"call to 'size()'|call to 'data()'}0 %select{cannot be narrowed from "
"type %2 to %3|evaluates to %2, which cannot be narrowed to type %3}1">,
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9999,6 +9999,7 @@ class Sema final : public SemaBase {
CCEK_CaseValue, ///< Expression in a case label.
CCEK_Enumerator, ///< Enumerator value with fixed underlying type.
CCEK_TemplateArg, ///< Value of a non-type template parameter.
CCEK_InjectedTTP, ///< Injected parameter of a template template parameter.
CCEK_ArrayBound, ///< Array bound in array declarator or new-expression.
CCEK_ExplicitBool, ///< Condition in an explicit(bool) specifier.
CCEK_Noexcept, ///< Condition in a noexcept(bool) specifier.
Expand Down Expand Up @@ -11682,6 +11683,7 @@ class Sema final : public SemaBase {
SmallVectorImpl<TemplateArgument> &SugaredConverted,
SmallVectorImpl<TemplateArgument> &CanonicalConverted,
CheckTemplateArgumentKind CTAK, bool PartialOrdering,
bool PartialOrderingTTP,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find myself wishing we could combine this with at least PartialOrdering in an enum of some sort.

Copy link
Contributor Author

@mizvekov mizvekov Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, they are orthogonal and all four combinations are valid. Maybe renaming s/PartialOrderingTTP/MatchingTTP would make that more clear, but there are tons of pre-existing users.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats always the balance unfortunately. I hate having 2 bools in a row like this, particularly when they are both SO similar in name/meaning. I would still love an enum with 4 values, but I can wait to see if others are as concerned as I am, and disagree and commit if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can turn the two bools into enums, and we can group parameters which often go together into an 'Info' like thing. Grouping ortogonal bools into one enum like that will create friction in that we will often want to combine and split them apart anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping, how may we proceed?

I don't think a good solution to this would come in this patch, as we have the same problem in multiple places, and this would add a lot of noise here.

We are due a refactor of these partial ordering functions, and I'd prefer to tackle that on a followup.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a case where I would really like to stop the bleeding, so I'd like to find a better solution than just a pair of bools here. If you have a patch to do the refactor, it would be nice to land that first to see its effect in action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done on top of the patch stack: #124668

bool *MatchedPackOnParmToNonPackOnArg);

/// Check that the given template arguments can be provided to
Expand Down Expand Up @@ -11755,6 +11757,7 @@ class Sema final : public SemaBase {
QualType InstantiatedParamType, Expr *Arg,
TemplateArgument &SugaredConverted,
TemplateArgument &CanonicalConverted,
bool PartialOrderingTTP,
CheckTemplateArgumentKind CTAK);

/// Check a template argument against its corresponding
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3676,7 +3676,7 @@ Sema::LookupLiteralOperator(Scope *S, LookupResult &R,
if (CheckTemplateArgument(
Params->getParam(0), Arg, FD, R.getNameLoc(), R.getNameLoc(),
0, SugaredChecked, CanonicalChecked, CTAK_Specified,
/*PartialOrdering=*/false,
/*PartialOrdering=*/false, /*PartialOrderingTTP=*/false,
/*MatchedPackOnParmToNonPackOnArg=*/nullptr) ||
Trap.hasErrorOccurred())
IsTemplate = false;
Expand Down
23 changes: 16 additions & 7 deletions clang/lib/Sema/SemaOverload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6151,8 +6151,8 @@ static ExprResult BuildConvertedConstantExpression(Sema &S, Expr *From,
Sema::CCEKind CCE,
NamedDecl *Dest,
APValue &PreNarrowingValue) {
assert(S.getLangOpts().CPlusPlus11 &&
"converted constant expression outside C++11");
assert((S.getLangOpts().CPlusPlus11 || CCE == Sema::CCEK_InjectedTTP) &&
"converted constant expression outside C++11 or TTP matching");

if (checkPlaceholderForOverload(S, From))
return ExprError();
Expand Down Expand Up @@ -6221,8 +6221,10 @@ static ExprResult BuildConvertedConstantExpression(Sema &S, Expr *From,
// earlier, but that's not guaranteed to work when initializing an object of
// class type.
ExprResult Result;
bool IsTemplateArgument =
CCE == Sema::CCEK_TemplateArg || CCE == Sema::CCEK_InjectedTTP;
if (T->isRecordType()) {
assert(CCE == Sema::CCEK_TemplateArg &&
assert(IsTemplateArgument &&
"unexpected class type converted constant expr");
Result = S.PerformCopyInitialization(
InitializedEntity::InitializeTemplateParameter(
Expand All @@ -6239,7 +6241,7 @@ static ExprResult BuildConvertedConstantExpression(Sema &S, Expr *From,
// A full-expression is [...] a constant-expression [...]
Result = S.ActOnFinishFullExpr(Result.get(), From->getExprLoc(),
/*DiscardedValue=*/false, /*IsConstexpr=*/true,
CCE == Sema::CCEKind::CCEK_TemplateArg);
IsTemplateArgument);
if (Result.isInvalid())
return Result;

Expand All @@ -6248,9 +6250,6 @@ static ExprResult BuildConvertedConstantExpression(Sema &S, Expr *From,
QualType PreNarrowingType;
switch (SCS->getNarrowingKind(S.Context, Result.get(), PreNarrowingValue,
PreNarrowingType)) {
case NK_Dependent_Narrowing:
// Implicit conversion to a narrower type, but the expression is
// value-dependent so we can't tell whether it's actually narrowing.
case NK_Variable_Narrowing:
// Implicit conversion to a narrower type, and the value is not a constant
// expression. We'll diagnose this in a moment.
Expand All @@ -6271,6 +6270,14 @@ static ExprResult BuildConvertedConstantExpression(Sema &S, Expr *From,
<< PreNarrowingValue.getAsString(S.Context, PreNarrowingType) << T;
break;

case NK_Dependent_Narrowing:
// Implicit conversion to a narrower type, but the expression is
// value-dependent so we can't tell whether it's actually narrowing.
// For matching the parameters of a TTP, the conversion is ill-formed
// if it may narrow.
if (CCE != Sema::CCEK_InjectedTTP)
break;
[[fallthrough]];
case NK_Type_Narrowing:
// FIXME: It would be better to diagnose that the expression is not a
// constant expression.
Expand Down Expand Up @@ -6343,6 +6350,8 @@ Sema::EvaluateConvertedConstantExpression(Expr *E, QualType T, APValue &Value,
Expr::EvalResult Eval;
Eval.Diag = &Notes;

assert(CCE != Sema::CCEK_InjectedTTP && "unnexpected CCE Kind");

ConstantExprKind Kind;
if (CCE == Sema::CCEK_TemplateArg && T->isRecordType())
Kind = ConstantExprKind::ClassTemplateArgument;
Expand Down
44 changes: 26 additions & 18 deletions clang/lib/Sema/SemaTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5205,7 +5205,7 @@ bool Sema::CheckTemplateArgument(
SmallVectorImpl<TemplateArgument> &SugaredConverted,
SmallVectorImpl<TemplateArgument> &CanonicalConverted,
CheckTemplateArgumentKind CTAK, bool PartialOrdering,
bool *MatchedPackOnParmToNonPackOnArg) {
bool PartialOrderingTTP, bool *MatchedPackOnParmToNonPackOnArg) {
// Check template type parameters.
if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(Param))
return CheckTemplateTypeArgument(TTP, Arg, SugaredConverted,
Expand Down Expand Up @@ -5260,8 +5260,9 @@ bool Sema::CheckTemplateArgument(
Expr *E = Arg.getArgument().getAsExpr();
TemplateArgument SugaredResult, CanonicalResult;
unsigned CurSFINAEErrors = NumSFINAEErrors;
ExprResult Res = CheckTemplateArgument(NTTP, NTTPType, E, SugaredResult,
CanonicalResult, CTAK);
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.
Expand Down Expand Up @@ -5326,7 +5327,8 @@ bool Sema::CheckTemplateArgument(

TemplateArgument SugaredResult, CanonicalResult;
E = CheckTemplateArgument(NTTP, NTTPType, E.get(), SugaredResult,
CanonicalResult, CTAK_Specified);
CanonicalResult, /*PartialOrderingTTP=*/false,
CTAK_Specified);
if (E.isInvalid())
return true;

Expand Down Expand Up @@ -5585,11 +5587,11 @@ bool Sema::CheckTemplateArgumentList(
getExpandedPackSize(*Param))
Arg = Arg.getPackExpansionPattern();
TemplateArgumentLoc NewArgLoc(Arg, ArgLoc.getLocInfo());
if (CheckTemplateArgument(*Param, NewArgLoc, Template, TemplateLoc,
RAngleLoc, SugaredArgumentPack.size(),
SugaredConverted, CanonicalConverted,
CTAK_Specified, /*PartialOrdering=*/false,
MatchedPackOnParmToNonPackOnArg))
if (CheckTemplateArgument(
*Param, NewArgLoc, Template, TemplateLoc, RAngleLoc,
SugaredArgumentPack.size(), SugaredConverted,
CanonicalConverted, CTAK_Specified, /*PartialOrdering=*/false,
/*PartialOrderingTTP=*/true, MatchedPackOnParmToNonPackOnArg))
return true;
Arg = NewArgLoc.getArgument();
CanonicalConverted.back().setIsDefaulted(
Expand All @@ -5601,11 +5603,11 @@ bool Sema::CheckTemplateArgumentList(
TemplateArgumentLoc(TemplateArgument::CreatePackCopy(Context, Args),
ArgLoc.getLocInfo());
} else {
if (CheckTemplateArgument(*Param, ArgLoc, Template, TemplateLoc,
RAngleLoc, SugaredArgumentPack.size(),
SugaredConverted, CanonicalConverted,
CTAK_Specified, /*PartialOrdering=*/false,
MatchedPackOnParmToNonPackOnArg))
if (CheckTemplateArgument(
*Param, ArgLoc, Template, TemplateLoc, RAngleLoc,
SugaredArgumentPack.size(), SugaredConverted,
CanonicalConverted, CTAK_Specified, /*PartialOrdering=*/false,
PartialOrderingTTP, MatchedPackOnParmToNonPackOnArg))
return true;
CanonicalConverted.back().setIsDefaulted(
clang::isSubstitutedDefaultArgument(Context, ArgLoc.getArgument(),
Expand Down Expand Up @@ -5753,6 +5755,7 @@ bool Sema::CheckTemplateArgumentList(
if (CheckTemplateArgument(*Param, Arg, Template, TemplateLoc, RAngleLoc, 0,
SugaredConverted, CanonicalConverted,
CTAK_Specified, /*PartialOrdering=*/false,
/*PartialOrderingTTP=*/false,
/*MatchedPackOnParmToNonPackOnArg=*/nullptr))
return true;

Expand Down Expand Up @@ -6740,6 +6743,7 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
QualType ParamType, Expr *Arg,
TemplateArgument &SugaredConverted,
TemplateArgument &CanonicalConverted,
bool PartialOrderingTTP,
CheckTemplateArgumentKind CTAK) {
SourceLocation StartLoc = Arg->getBeginLoc();

Expand Down Expand Up @@ -6930,17 +6934,21 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
IsConvertedConstantExpression = false;
}

if (getLangOpts().CPlusPlus17) {
if (getLangOpts().CPlusPlus17 || PartialOrderingTTP) {
// C++17 [temp.arg.nontype]p1:
// A template-argument for a non-type template parameter shall be
// a converted constant expression of the type of the template-parameter.
APValue Value;
ExprResult ArgResult;
if (IsConvertedConstantExpression) {
ArgResult = BuildConvertedConstantExpression(Arg, ParamType,
CCEK_TemplateArg, Param);
if (ArgResult.isInvalid())
ArgResult = BuildConvertedConstantExpression(
Arg, ParamType,
PartialOrderingTTP ? CCEK_InjectedTTP : CCEK_TemplateArg, Param);
assert(!ArgResult.isUnset());
if (ArgResult.isInvalid()) {
NoteTemplateParameterLocation(*Param);
return ExprError();
}
} else {
ArgResult = Arg;
}
Expand Down
4 changes: 3 additions & 1 deletion clang/lib/Sema/SemaTemplateDeduction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2979,7 +2979,8 @@ static bool ConvertDeducedTemplateArgument(
? (Arg.wasDeducedFromArrayBound() ? Sema::CTAK_DeducedFromArrayBound
: Sema::CTAK_Deduced)
: Sema::CTAK_Specified,
PartialOrdering, &MatchedPackOnParmToNonPackOnArg);
PartialOrdering, /*PartialOrderingTTP=*/false,
&MatchedPackOnParmToNonPackOnArg);
if (MatchedPackOnParmToNonPackOnArg)
Info.setMatchedPackOnParmToNonPackOnArg();
return Res;
Expand Down Expand Up @@ -3179,6 +3180,7 @@ static TemplateDeductionResult ConvertDeducedTemplateArguments(
Param, DefArg, TD, TD->getLocation(), TD->getSourceRange().getEnd(),
/*ArgumentPackIndex=*/0, SugaredBuilder, CanonicalBuilder,
Sema::CTAK_Specified, /*PartialOrdering=*/false,
/*PartialOrderingTTP=*/false,
/*MatchedPackOnParmToNonPackOnArg=*/nullptr)) {
Info.Param = makeTemplateParameter(
const_cast<NamedDecl *>(TemplateParams->getParam(I)));
Expand Down
7 changes: 4 additions & 3 deletions clang/lib/Sema/SemaTemplateInstantiate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2399,9 +2399,10 @@ TemplateInstantiator::TransformSubstNonTypeTemplateParmExpr(
// The call to CheckTemplateArgument here produces the ImpCast.
TemplateArgument SugaredConverted, CanonicalConverted;
if (SemaRef
.CheckTemplateArgument(E->getParameter(), SubstType,
SubstReplacement.get(), SugaredConverted,
CanonicalConverted, Sema::CTAK_Specified)
.CheckTemplateArgument(
E->getParameter(), SubstType, SubstReplacement.get(),
SugaredConverted, CanonicalConverted,
/*PartialOrderingTTP=*/false, Sema::CTAK_Specified)
.isInvalid())
return true;
return transformNonTypeTemplateParmRef(E->getAssociatedDecl(),
Expand Down
6 changes: 3 additions & 3 deletions clang/test/CXX/drs/cwg0xx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,12 +521,12 @@ namespace example1 {
namespace A {
int i;
}

namespace A1 {
using A::i;
using A::i;
}

void f()
{
using A::i;
Expand Down Expand Up @@ -1371,7 +1371,7 @@ namespace cwg92 { // cwg92: 4 c++17
// considered in this context. In C++17, we *do* perform an implicit
// conversion (which performs initialization), and the exception specification
// is part of the type of the parameter, so this is invalid.
template<void() throw()> struct X {};
template<void() throw()> struct X {}; // since-cxx17-note {{template parameter is declared here}}
X<&f> xp;
// since-cxx17-error@-1 {{value of type 'void (*)() throw(int, float)' is not implicitly convertible to 'void (*)() throw()'}}

Expand Down
2 changes: 2 additions & 0 deletions clang/test/CXX/drs/cwg12xx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ namespace cwg1295 { // cwg1295: 4
// cxx98-14-error@-1 {{non-type template argument does not refer to any declaration}}
// cxx98-14-note@#cwg1295-Y {{template parameter is declared here}}
// since-cxx17-error@#cwg1295-y {{reference cannot bind to bit-field in converted constant expression}}
// since-cxx17-note@#cwg1295-Y {{template parameter is declared here}}


#if __cplusplus >= 201103L
const unsigned other = 0;
Expand Down
Loading
Loading