-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857
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.
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
Changes from all commits
1119f0a
ca00718
2dd772d
96bf64c
6b7072d
bb31d36
631be75
7d484e2
c515407
a9da5a3
cebe706
e0b21a5
30dc1d1
9d3981f
afb8a90
6a5085a
974a0ad
d19e700
5be9921
8ace714
da61fab
18da86a
1b8f0f6
ef5acad
6fc2f7d
d457f02
f07c4a9
a2818ba
8f2dc77
d16f2cf
7e7ed8e
7bc252a
f0b1e7c
2e9061b
9f935ab
e623bf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -846,7 +846,7 @@ bool Sema::CheckFunctionConstraints(const FunctionDecl *FD, | |
bool ForOverloadResolution) { | ||
// Don't check constraints if the function is dependent. Also don't check if | ||
// this is a function template specialization, as the call to | ||
// CheckinstantiatedFunctionTemplateConstraints after this will check it | ||
// CheckFunctionTemplateConstraints after this will check it | ||
// better. | ||
if (FD->isDependentContext() || | ||
FD->getTemplatedKind() == | ||
|
@@ -1111,12 +1111,55 @@ bool Sema::EnsureTemplateArgumentListConstraints( | |
return false; | ||
} | ||
|
||
bool Sema::CheckInstantiatedFunctionTemplateConstraints( | ||
static bool CheckFunctionConstraintsWithoutInstantiation( | ||
Sema &SemaRef, SourceLocation PointOfInstantiation, | ||
FunctionTemplateDecl *Template, ArrayRef<TemplateArgument> TemplateArgs, | ||
ConstraintSatisfaction &Satisfaction) { | ||
SmallVector<const Expr *, 3> TemplateAC; | ||
Template->getAssociatedConstraints(TemplateAC); | ||
if (TemplateAC.empty()) { | ||
Satisfaction.IsSatisfied = true; | ||
return false; | ||
} | ||
|
||
LocalInstantiationScope Scope(SemaRef); | ||
|
||
FunctionDecl *FD = Template->getTemplatedDecl(); | ||
// Collect the list of template arguments relative to the 'primary' | ||
// template. We need the entire list, since the constraint is completely | ||
// uninstantiated at this point. | ||
|
||
// FIXME: Add TemplateArgs through the 'Innermost' parameter once | ||
// the refactoring of getTemplateInstantiationArgs() relands. | ||
MultiLevelTemplateArgumentList MLTAL; | ||
MLTAL.addOuterTemplateArguments(Template, std::nullopt, /*Final=*/false); | ||
SemaRef.getTemplateInstantiationArgs( | ||
MLTAL, /*D=*/FD, FD, | ||
/*Final=*/false, /*Innermost=*/std::nullopt, /*RelativeToPrimary=*/true, | ||
/*Pattern=*/nullptr, /*ForConstraintInstantiation=*/true); | ||
MLTAL.replaceInnermostTemplateArguments(Template, TemplateArgs); | ||
zyn0217 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to enter an Unevaluated |
||
Sema::ContextRAII SavedContext(SemaRef, FD); | ||
std::optional<Sema::CXXThisScopeRAII> ThisScope; | ||
zyn0217 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (auto *Method = dyn_cast<CXXMethodDecl>(FD)) | ||
ThisScope.emplace(SemaRef, /*Record=*/Method->getParent(), | ||
/*ThisQuals=*/Method->getMethodQualifiers()); | ||
Comment on lines
+1144
to
+1146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is that useful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I don't know. :( I copied that from the previous implementation, and I was expecting it to make a difference in situations where member function calls are involved in a constraint evaluation. But I ran the regression tests with that removed and everything seemed fine. I will have another try on a build without this patch. It could also be the case we're missing some tests, but I don't know... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, I removed these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe try to play with things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to contrive a case like template <class T>
struct S {
void foo() requires(__is_same_as(S&, decltype(*this))) {}
};
void g() {
S<int>().foo();
} But there would still be no difference if we did the Maybe I'm running out of ideas right now, so let's just preserve these "strange but plausible" things? |
||
return SemaRef.CheckConstraintSatisfaction( | ||
Template, TemplateAC, MLTAL, PointOfInstantiation, Satisfaction); | ||
} | ||
|
||
bool Sema::CheckFunctionTemplateConstraints( | ||
SourceLocation PointOfInstantiation, FunctionDecl *Decl, | ||
ArrayRef<TemplateArgument> TemplateArgs, | ||
ConstraintSatisfaction &Satisfaction) { | ||
// In most cases we're not going to have constraints, so check for that first. | ||
FunctionTemplateDecl *Template = Decl->getPrimaryTemplate(); | ||
|
||
if (!Template) | ||
return ::CheckFunctionConstraintsWithoutInstantiation( | ||
*this, PointOfInstantiation, Decl->getDescribedFunctionTemplate(), | ||
TemplateArgs, Satisfaction); | ||
|
||
// Note - code synthesis context for the constraints check is created | ||
// inside CheckConstraintsSatisfaction. | ||
SmallVector<const Expr *, 3> TemplateAC; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3933,18 +3933,6 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction( | |
Result != TemplateDeductionResult::Success) | ||
return Result; | ||
|
||
// C++ [temp.deduct.call]p10: [DR1391] | ||
// If deduction succeeds for all parameters that contain | ||
// template-parameters that participate in template argument deduction, | ||
// and all template arguments are explicitly specified, deduced, or | ||
// obtained from default template arguments, remaining parameters are then | ||
// compared with the corresponding arguments. For each remaining parameter | ||
// P with a type that was non-dependent before substitution of any | ||
// explicitly-specified template arguments, if the corresponding argument | ||
// A cannot be implicitly converted to P, deduction fails. | ||
if (CheckNonDependent()) | ||
return TemplateDeductionResult::NonDependentConversionFailure; | ||
|
||
// Form the template argument list from the deduced template arguments. | ||
TemplateArgumentList *SugaredDeducedArgumentList = | ||
TemplateArgumentList::CreateCopy(Context, SugaredBuilder); | ||
|
@@ -3974,6 +3962,39 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction( | |
FD = const_cast<FunctionDecl *>(FDFriend); | ||
Owner = FD->getLexicalDeclContext(); | ||
} | ||
// C++20 [temp.deduct.general]p5: [CWG2369] | ||
// If the function template has associated constraints, those constraints | ||
// are checked for satisfaction. If the constraints are not satisfied, type | ||
// deduction fails. | ||
// | ||
// FIXME: We haven't implemented CWG2369 for lambdas yet, because we need | ||
// to figure out how to instantiate lambda captures to the scope without | ||
// first instantiating the lambda. | ||
bool IsLambda = isLambdaCallOperator(FD) || isLambdaConversionOperator(FD); | ||
if (!IsLambda && !IsIncomplete) { | ||
if (CheckFunctionTemplateConstraints( | ||
Info.getLocation(), | ||
FunctionTemplate->getCanonicalDecl()->getTemplatedDecl(), | ||
CanonicalBuilder, Info.AssociatedConstraintsSatisfaction)) | ||
return TemplateDeductionResult::MiscellaneousDeductionFailure; | ||
if (!Info.AssociatedConstraintsSatisfaction.IsSatisfied) { | ||
Info.reset(Info.takeSugared(), | ||
TemplateArgumentList::CreateCopy(Context, CanonicalBuilder)); | ||
Comment on lines
+3981
to
+3982
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is that necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Frankly these lines are also copied from the previous failure handling) I guess the instantiated concept-related nodes that were created during the checking would use the TemplateArgumentLists created on the ASTContext, or rather they would take ownership of the TemplateArgumentList. So, in order for the pilfered argument lists to work for other clients of TemplateDeductionInfo, we have to make a copy of the list. (I saw the patch that tried to preserve sugars in concepts turn this line into "copy sugared arguments, but do nothing for canonical arguments") But I'm not entirely sure of such deduction things, so probably @mizvekov would correct my understanding in some way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ping @mizvekov There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, what @zyn0217 said is correct, the canonical argument list will have references persist in AST nodes, so it needs to be copied here. |
||
return TemplateDeductionResult::ConstraintsNotSatisfied; | ||
} | ||
} | ||
// C++ [temp.deduct.call]p10: [CWG1391] | ||
// If deduction succeeds for all parameters that contain | ||
// template-parameters that participate in template argument deduction, | ||
// and all template arguments are explicitly specified, deduced, or | ||
// obtained from default template arguments, remaining parameters are then | ||
// compared with the corresponding arguments. For each remaining parameter | ||
// P with a type that was non-dependent before substitution of any | ||
// explicitly-specified template arguments, if the corresponding argument | ||
// A cannot be implicitly converted to P, deduction fails. | ||
if (CheckNonDependent()) | ||
return TemplateDeductionResult::NonDependentConversionFailure; | ||
|
||
MultiLevelTemplateArgumentList SubstArgs( | ||
FunctionTemplate, CanonicalDeducedArgumentList->asArray(), | ||
/*Final=*/false); | ||
|
@@ -4008,8 +4029,8 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction( | |
// ([temp.constr.decl]), those constraints are checked for satisfaction | ||
// ([temp.constr.constr]). If the constraints are not satisfied, type | ||
// deduction fails. | ||
if (!IsIncomplete) { | ||
if (CheckInstantiatedFunctionTemplateConstraints( | ||
if (IsLambda && !IsIncomplete) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a fixme comment here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's one on line 4003, maybe that just suffices? |
||
if (CheckFunctionTemplateConstraints( | ||
Info.getLocation(), Specialization, CanonicalBuilder, | ||
Info.AssociatedConstraintsSatisfaction)) | ||
return TemplateDeductionResult::MiscellaneousDeductionFailure; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was taken from #110387, which has also been reverted due to its dependency on the refactoring work.
The extra parameters can certainly be removed alongside the relanding of that huge patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove that now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No? Otherwise the MLTAL would be created internally and we wouldn't have a chance to put an empty args outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(For clarity, this function doesn't insert an empty list for FunctionDecl that is not a specialization. So we need to do that right before the call)