-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857
base: main
Are you sure you want to change the base?
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
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 |
---|---|---|
|
@@ -847,7 +847,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 | ||
// CheckInstantiatedFunctionTemplateConstraints after this will check it | ||
// better. | ||
if (FD->isDependentContext() || | ||
FD->getTemplatedKind() == | ||
|
@@ -1109,12 +1109,50 @@ 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. | ||
MultiLevelTemplateArgumentList MLTAL = | ||
SemaRef.getTemplateInstantiationArgs(FD, /*DC=*/nullptr, | ||
/*Final=*/false, | ||
/*Innermost=*/TemplateArgs, | ||
/*RelativeToPrimary=*/true, | ||
/*ForConstraintInstantiation=*/true); | ||
|
||
std::optional<Sema::CXXThisScopeRAII> ThisScope; | ||
if (auto *Method = dyn_cast<CXXMethodDecl>(FD)) | ||
ThisScope.emplace(SemaRef, /*Record=*/Method->getParent(), | ||
/*ThisQuals=*/Method->getMethodQualifiers()); | ||
Comment on lines
+1137
to
+1139
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 |
||
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; | ||
|
@@ -1517,7 +1555,7 @@ substituteParameterMappings(Sema &S, NormalizedConstraint &N, | |
static bool substituteParameterMappings(Sema &S, NormalizedConstraint &N, | ||
const ConceptSpecializationExpr *CSE) { | ||
MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs( | ||
CSE->getNamedConcept(), CSE->getNamedConcept()->getLexicalDeclContext(), | ||
CSE->getNamedConcept(), CSE->getNamedConcept()->getDeclContext(), | ||
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. Do you have tests with friend functions? 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. This is another leftover of my previous changes on |
||
/*Final=*/false, CSE->getTemplateArguments(), | ||
/*RelativeToPrimary=*/true, | ||
/*ForConstraintInstantiation=*/true); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3982,18 +3982,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); | ||||||
|
@@ -4008,6 +3996,38 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction( | |||||
Owner = FunctionTemplate->getLexicalDeclContext(); | ||||||
FunctionDecl *FD = FunctionTemplate->getTemplatedDecl(); | ||||||
|
||||||
// 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
+4014
to
+4015
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? |
||||||
return TemplateDeductionResult::ConstraintsNotSatisfied; | ||||||
} | ||||||
} | ||||||
// C++ [temp.deduct.call]p10: [DR1391] | ||||||
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.
Suggested change
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. Oops, I thought I have fixed it everywhere... |
||||||
// 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); | ||||||
|
@@ -4042,8 +4062,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; | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1365,6 +1365,20 @@ namespace { | |||||
// Whether an incomplete substituion should be treated as an error. | ||||||
bool BailOutOnIncomplete; | ||||||
|
||||||
private: | ||||||
bool isSubstitutingConstraints() const { | ||||||
return llvm::any_of( | ||||||
llvm::reverse(SemaRef.CodeSynthesisContexts), [](auto &Context) { | ||||||
return Context.Kind == | ||||||
Sema::CodeSynthesisContext::ConstraintSubstitution; | ||||||
}); | ||||||
} | ||||||
|
||||||
// CWG2770: Function parameters should be instantiated when they are | ||||||
// needed by a satisfaction check of an atomic constraint or | ||||||
// (recursively) by another function parameter. | ||||||
bool maybeInstantiateFunctionParameterToScope(ParmVarDecl *OldParm); | ||||||
|
||||||
public: | ||||||
typedef TreeTransform<TemplateInstantiator> inherited; | ||||||
|
||||||
|
@@ -1378,9 +1392,7 @@ namespace { | |||||
void setEvaluateConstraints(bool B) { | ||||||
EvaluateConstraints = B; | ||||||
} | ||||||
bool getEvaluateConstraints() { | ||||||
return EvaluateConstraints; | ||||||
} | ||||||
bool getEvaluateConstraints() const { return EvaluateConstraints; } | ||||||
|
||||||
/// Determine whether the given type \p T has already been | ||||||
/// transformed. | ||||||
|
@@ -1421,12 +1433,19 @@ namespace { | |||||
ArrayRef<UnexpandedParameterPack> Unexpanded, | ||||||
bool &ShouldExpand, bool &RetainExpansion, | ||||||
std::optional<unsigned> &NumExpansions) { | ||||||
return getSema().CheckParameterPacksForExpansion(EllipsisLoc, | ||||||
PatternRange, Unexpanded, | ||||||
TemplateArgs, | ||||||
ShouldExpand, | ||||||
RetainExpansion, | ||||||
NumExpansions); | ||||||
if (SemaRef.CurrentInstantiationScope && isSubstitutingConstraints()) { | ||||||
for (UnexpandedParameterPack ParmPack : Unexpanded) { | ||||||
NamedDecl *VD = ParmPack.first.dyn_cast<NamedDecl *>(); | ||||||
if (!isa_and_present<ParmVarDecl>(VD)) | ||||||
continue; | ||||||
if (maybeInstantiateFunctionParameterToScope(cast<ParmVarDecl>(VD))) | ||||||
return true; | ||||||
} | ||||||
} | ||||||
|
||||||
return getSema().CheckParameterPacksForExpansion( | ||||||
EllipsisLoc, PatternRange, Unexpanded, TemplateArgs, ShouldExpand, | ||||||
RetainExpansion, NumExpansions); | ||||||
} | ||||||
|
||||||
void ExpandingFunctionParameterPack(ParmVarDecl *Pack) { | ||||||
|
@@ -1916,9 +1935,33 @@ Decl *TemplateInstantiator::TransformDecl(SourceLocation Loc, Decl *D) { | |||||
// template parameter. | ||||||
} | ||||||
|
||||||
if (SemaRef.CurrentInstantiationScope) { | ||||||
if (isSubstitutingConstraints() && isa<ParmVarDecl>(D) && | ||||||
maybeInstantiateFunctionParameterToScope(cast<ParmVarDecl>(D))) | ||||||
return nullptr; | ||||||
} | ||||||
|
||||||
return SemaRef.FindInstantiatedDecl(Loc, cast<NamedDecl>(D), TemplateArgs); | ||||||
} | ||||||
|
||||||
bool TemplateInstantiator::maybeInstantiateFunctionParameterToScope( | ||||||
ParmVarDecl *OldParm) { | ||||||
if (SemaRef.CurrentInstantiationScope->findInstantiationUnsafe(OldParm)) | ||||||
return false; | ||||||
// The current context might have been changed in the process of transforming | ||||||
// lambda expression. So resume it before we substitute into the parameter. | ||||||
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.
Suggested change
|
||||||
Sema::ContextRAII Context(SemaRef, OldParm->getDeclContext()); | ||||||
|
||||||
SmallVector<QualType> PTypes; | ||||||
Sema::ExtParameterInfoBuilder TInfoBuilder; | ||||||
|
||||||
return inherited::TransformFunctionTypeParams( | ||||||
/*Loc=*/SourceLocation(), /*Params=*/OldParm, /*ParamTypes=*/nullptr, | ||||||
/*ParamInfos=*/nullptr, /*PTypes=*/PTypes, /*PVars=*/nullptr, | ||||||
TInfoBuilder, /*LastParamTransformed=*/nullptr, | ||||||
/*IgnoreParameterIndex=*/true); | ||||||
} | ||||||
|
||||||
Decl *TemplateInstantiator::TransformDefinition(SourceLocation Loc, Decl *D) { | ||||||
Decl *Inst = getSema().SubstDecl(D, getSema().CurContext, TemplateArgs); | ||||||
if (!Inst) | ||||||
|
@@ -4542,9 +4585,8 @@ static const Decl *getCanonicalParmVarDecl(const Decl *D) { | |||||
return D; | ||||||
} | ||||||
|
||||||
|
||||||
llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> * | ||||||
LocalInstantiationScope::findInstantiationOf(const Decl *D) { | ||||||
LocalInstantiationScope::findInstantiationUnsafe(const Decl *D) { | ||||||
D = getCanonicalParmVarDecl(D); | ||||||
for (LocalInstantiationScope *Current = this; Current; | ||||||
Current = Current->Outer) { | ||||||
|
@@ -4569,6 +4611,14 @@ LocalInstantiationScope::findInstantiationOf(const Decl *D) { | |||||
break; | ||||||
} | ||||||
|
||||||
return nullptr; | ||||||
} | ||||||
|
||||||
llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> * | ||||||
LocalInstantiationScope::findInstantiationOf(const Decl *D) { | ||||||
auto *Result = findInstantiationUnsafe(D); | ||||||
if (Result) | ||||||
return Result; | ||||||
// If we're performing a partial substitution during template argument | ||||||
// deduction, we may not have values for template parameters yet. | ||||||
if (isa<NonTypeTemplateParmDecl>(D) || isa<TemplateTypeParmDecl>(D) || | ||||||
|
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.
Do we need to point CurContext to the function here as well?
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.
Yes, I think so - but I couldn't find a case where that affects anything.