llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) <details> <summary>Changes</summary> This reapplies https://github.com/llvm/llvm-project/pull/195995 The last approach caused a regression because sometimes we transformed the constraints of lambda expression too aggressively when we should have preserved it. The basic idea of this patch is to transform constraints of lambda expressions when we instantiate the parameter mapping, thus the evaluation can be emancipated from preserving outer template arguments. Fixes https://github.com/llvm/llvm-project/issues/193944 Fixes https://github.com/llvm/llvm-project/issues/202957 --- Patch is 28.47 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/207966.diff 10 Files Affected: - (modified) clang/include/clang/Sema/Sema.h (+1-1) - (modified) clang/lib/Parse/ParseTemplate.cpp (-6) - (modified) clang/lib/Sema/SemaConcept.cpp (+62-49) - (modified) clang/lib/Sema/SemaTemplate.cpp (+1-1) - (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (-14) - (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+52-11) - (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+8-1) - (modified) clang/lib/Sema/TreeTransform.h (+12-2) - (added) clang/test/SemaTemplate/concepts-lambda-GH199209.cpp (+123) - (modified) clang/test/SemaTemplate/concepts-lambda.cpp (+33-6) ``````````diff diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 631a680727fee..2629b7b0a8679 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -13520,7 +13520,7 @@ class Sema final : public SemaBase { bool SubstTemplateArgumentsInParameterMapping( ArrayRef<TemplateArgumentLoc> Args, SourceLocation BaseLoc, const MultiLevelTemplateArgumentList &TemplateArgs, - TemplateArgumentListInfo &Out); + bool TransformLambdaConstraint, TemplateArgumentListInfo &Out); /// Retrieve the template argument list(s) that should be used to /// instantiate the definition of the given declaration. diff --git a/clang/lib/Parse/ParseTemplate.cpp b/clang/lib/Parse/ParseTemplate.cpp index 735a9bd1f9f1c..1529da187614f 100644 --- a/clang/lib/Parse/ParseTemplate.cpp +++ b/clang/lib/Parse/ParseTemplate.cpp @@ -533,12 +533,6 @@ bool Parser::isTypeConstraintAnnotation() { bool Parser::TryAnnotateTypeConstraint() { if (!getLangOpts().CPlusPlus20) return false; - // The type constraint may declare template parameters, notably - // if it contains a generic lambda, so we need to increment - // the template depth as these parameters would not be instantiated - // at the current depth. - TemplateParameterDepthRAII CurTemplateDepthTracker(TemplateParameterDepth); - ++CurTemplateDepthTracker; CXXScopeSpec SS; bool WasScopeAnnotation = Tok.is(tok::annot_cxxscope); if (ParseOptionalCXXScopeSpecifier(SS, /*ObjectType=*/nullptr, diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp index 32a07b6266666..cbbe9b57a38b9 100644 --- a/clang/lib/Sema/SemaConcept.cpp +++ b/clang/lib/Sema/SemaConcept.cpp @@ -486,6 +486,7 @@ class HashParameterMapping : public RecursiveASTVisitor<HashParameterMapping> { class ConstraintSatisfactionChecker { Sema &S; const NamedDecl *Template; + const ConceptReference *TopLevelConceptId; SourceLocation TemplateNameLoc; UnsignedOrNone PackSubstitutionIndex; ConstraintSatisfaction &Satisfaction; @@ -544,11 +545,13 @@ class ConstraintSatisfactionChecker { public: ConstraintSatisfactionChecker(Sema &SemaRef, const NamedDecl *Template, + const ConceptReference *TopLevelConceptId, SourceLocation TemplateNameLoc, UnsignedOrNone PackSubstitutionIndex, ConstraintSatisfaction &Satisfaction, bool BuildExpression) - : S(SemaRef), Template(Template), TemplateNameLoc(TemplateNameLoc), + : S(SemaRef), Template(Template), TopLevelConceptId(TopLevelConceptId), + TemplateNameLoc(TemplateNameLoc), PackSubstitutionIndex(PackSubstitutionIndex), Satisfaction(Satisfaction), BuildExpression(BuildExpression) {} @@ -676,9 +679,11 @@ ConstraintSatisfactionChecker::SubstitutionInTemplateArguments( llvm::SaveAndRestore PushTemplateArgsCache(S.CurrentCachedTemplateArgs, &CachedTemplateArgs); + // We don't want the template argument substitution into parameter + // mappings to preserve the outer depths. if (S.SubstTemplateArgumentsInParameterMapping( Constraint.getParameterMapping(), Constraint.getBeginLoc(), MLTAL, - SubstArgs)) { + /*TransformLambdaConstraint=*/true, SubstArgs)) { Satisfaction.IsSatisfied = false; return std::nullopt; } @@ -741,17 +746,8 @@ ExprResult ConstraintSatisfactionChecker::EvaluateSlow( // i.e they should not have access to the current class object or its // non-public members. std::optional<Sema::ContextRAII> ConceptContext; - if (ParentConcept) { + if (ParentConcept) ConceptContext.emplace(S, ParentConcept->getDeclContext()); - // FIXME: the evaluation context should learn to track template arguments - // separately from a Decl. - EvaluationContext.emplace( - S, Sema::ExpressionEvaluationContext::ConstantEvaluated, - /*LambdaContextDecl=*/ - ImplicitConceptSpecializationDecl::Create( - S.Context, ParentConcept->getDeclContext(), - ParentConcept->getBeginLoc(), SubstitutedOutermost)); - } Sema::ArgPackSubstIndexRAII SubstIndex(S, PackSubstitutionIndex); ExprResult SubstitutedAtomicExpr = EvaluateAtomicConstraint( @@ -910,8 +906,9 @@ ExprResult ConstraintSatisfactionChecker::EvaluateSlow( Satisfaction.IsSatisfied = false; Satisfaction.ContainsErrors = false; ExprResult Expr = - ConstraintSatisfactionChecker(S, Template, TemplateNameLoc, - UnsignedOrNone(I), Satisfaction, + ConstraintSatisfactionChecker(S, Template, TopLevelConceptId, + TemplateNameLoc, UnsignedOrNone(I), + Satisfaction, /*BuildExpression=*/false) .Evaluate(Constraint.getNormalizedPattern(), *SubstitutedArgs); if (BuildExpression) { @@ -987,37 +984,42 @@ ExprResult ConstraintSatisfactionChecker::EvaluateSlow( } Sema::ArgPackSubstIndexRAII SubstIndex(S, getOuterPackIndex(Constraint)); - const ASTTemplateArgumentListInfo *Ori = ConceptId->getTemplateArgsAsWritten(); - TemplateDeductionInfo Info(TemplateNameLoc); - Sema::SFINAETrap Trap(S, Info); - Sema::InstantiatingTemplate _2( - S, TemplateNameLoc, Sema::InstantiatingTemplate::ConstraintSubstitution{}, - const_cast<NamedDecl *>(Template), Constraint.getSourceRange()); - TemplateArgumentListInfo OutArgs(Ori->LAngleLoc, Ori->RAngleLoc); - if (S.SubstTemplateArguments(Ori->arguments(), *SubstitutedArgs, OutArgs) || - Trap.hasErrorOccurred()) { - Satisfaction.IsSatisfied = false; - if (!Trap.hasErrorOccurred()) - return ExprError(); + if (Constraint.hasParameterMapping() || ConceptId != TopLevelConceptId) { + TemplateDeductionInfo Info(TemplateNameLoc); + Sema::SFINAETrap Trap(S, Info); + Sema::InstantiatingTemplate _2( + S, TemplateNameLoc, + Sema::InstantiatingTemplate::ConstraintSubstitution{}, + const_cast<NamedDecl *>(Template), Constraint.getSourceRange()); - PartialDiagnosticAt SubstDiag{SourceLocation(), - PartialDiagnostic::NullDiagnostic()}; - Info.takeSFINAEDiagnostic(SubstDiag); - // FIXME: This is an unfortunate consequence of there - // being no serialization code for PartialDiagnostics and the fact - // that serializing them would likely take a lot more storage than - // just storing them as strings. We would still like, in the - // future, to serialize the proper PartialDiagnostic as serializing - // it as a string defeats the purpose of the diagnostic mechanism. - Satisfaction.Details.insert( - Satisfaction.Details.begin() + Size, - new (S.Context) ConstraintSubstitutionDiagnostic{ - SubstDiag.first, - allocateStringFromConceptDiagnostic(S, SubstDiag.second)}); - return ExprError(); + if (S.SubstTemplateArguments(Ori->arguments(), *SubstitutedArgs, OutArgs) || + Trap.hasErrorOccurred()) { + Satisfaction.IsSatisfied = false; + if (!Trap.hasErrorOccurred()) + return ExprError(); + + PartialDiagnosticAt SubstDiag{SourceLocation(), + PartialDiagnostic::NullDiagnostic()}; + Info.takeSFINAEDiagnostic(SubstDiag); + // FIXME: This is an unfortunate consequence of there + // being no serialization code for PartialDiagnostics and the fact + // that serializing them would likely take a lot more storage than + // just storing them as strings. We would still like, in the + // future, to serialize the proper PartialDiagnostic as serializing + // it as a string defeats the purpose of the diagnostic mechanism. + Satisfaction.Details.insert( + Satisfaction.Details.begin() + Size, + new (S.Context) ConstraintSubstitutionDiagnostic{ + SubstDiag.first, + allocateStringFromConceptDiagnostic(S, SubstDiag.second)}); + return ExprError(); + } + } else { + for (auto &A : Ori->arguments()) + OutArgs.addArgument(A); } CXXScopeSpec SS; @@ -1028,7 +1030,7 @@ ExprResult ConstraintSatisfactionChecker::EvaluateSlow( ConceptId->getFoundDecl(), ConceptId->getNamedConcept(), &OutArgs, /*DoCheckConstraintSatisfaction=*/false); - if (SubstitutedConceptId.isInvalid() || Trap.hasErrorOccurred()) + if (SubstitutedConceptId.isInvalid()) return ExprError(); if (Size != Satisfaction.Details.size()) { @@ -1236,11 +1238,12 @@ static bool CheckConstraintSatisfaction( Template, /*CSE=*/nullptr, S.ArgPackSubstIndex); - ExprResult Res = ConstraintSatisfactionChecker( - S, Template, TemplateIDRange.getBegin(), - S.ArgPackSubstIndex, Satisfaction, - /*BuildExpression=*/ConvertedExpr != nullptr) - .Evaluate(*C, TemplateArgsLists); + ExprResult Res = + ConstraintSatisfactionChecker( + S, Template, TopLevelConceptId, TemplateIDRange.getBegin(), + S.ArgPackSubstIndex, Satisfaction, + /*BuildExpression=*/ConvertedExpr != nullptr) + .Evaluate(*C, TemplateArgsLists); if (Res.isInvalid()) return true; @@ -2135,7 +2138,8 @@ bool SubstituteParameterMappings::substitute( llvm::SaveAndRestore<decltype(SemaRef.CurrentCachedTemplateArgs)> DoNotCacheDependentArgs(SemaRef.CurrentCachedTemplateArgs, nullptr); if (SemaRef.SubstTemplateArgumentsInParameterMapping( - N.getParameterMapping(), N.getBeginLoc(), *MLTAL, SubstArgs)) + N.getParameterMapping(), N.getBeginLoc(), *MLTAL, + /*TransformLambdaConstraint=*/false, SubstArgs)) return true; Sema::CheckTemplateArgumentInfo CTAI; auto *TD = @@ -2207,7 +2211,8 @@ bool SubstituteParameterMappings::substitute(ConceptIdConstraint &CC) { const ASTTemplateArgumentListInfo *ArgsAsWritten = CSE->getTemplateArgsAsWritten(); if (SemaRef.SubstTemplateArgumentsInParameterMapping( - ArgsAsWritten->arguments(), CC.getBeginLoc(), *MLTAL, Out)) + ArgsAsWritten->arguments(), CC.getBeginLoc(), *MLTAL, + /*TransformLambdaConstraint=*/false, Out)) return true; Sema::CheckTemplateArgumentInfo CTAI; if (SemaRef.CheckTemplateArgumentList(CSE->getNamedConcept(), @@ -2253,6 +2258,14 @@ bool SubstituteParameterMappings::substitute(NormalizedConstraint &N) { } assert(!ArgsAsWritten); const ConceptSpecializationExpr *CSE = CC.getConceptSpecializationExpr(); + // Make sure that lambdas within template arguments live in a + // dependent context such that they are assured to be transformed during + // constraint evaluation. + EnterExpressionEvaluationContext EECtx( + SemaRef, Sema::ExpressionEvaluationContext::ConstantEvaluated, + /*LambdaContextDecl=*/ + const_cast<ImplicitConceptSpecializationDecl *>( + CSE->getSpecializationDecl())); SmallVector<TemplateArgument> InnerArgs(CSE->getTemplateArguments()); ConceptDecl *Concept = CSE->getNamedConcept(); if (RemovePacksForFoldExpr) { diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp index 1591cea9286ae..f0d7e03c7ee4e 100644 --- a/clang/lib/Sema/SemaTemplate.cpp +++ b/clang/lib/Sema/SemaTemplate.cpp @@ -4926,7 +4926,7 @@ ExprResult Sema::CheckConceptTemplateId( LocalInstantiationScope Scope(*this); EnterExpressionEvaluationContext EECtx{ - *this, ExpressionEvaluationContext::Unevaluated, CSD}; + *this, ExpressionEvaluationContext::Unevaluated}; Error = CheckConstraintSatisfaction( NamedConcept, AssociatedConstraint(Concept->getConstraintExpr()), MLTAL, diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp index f57f60eb527ae..dd208d78063e6 100644 --- a/clang/lib/Sema/SemaTemplateDeduction.cpp +++ b/clang/lib/Sema/SemaTemplateDeduction.cpp @@ -5159,20 +5159,6 @@ static bool CheckDeducedPlaceholderConstraints(Sema &S, const AutoType &Type, return true; MultiLevelTemplateArgumentList MLTAL(Concept, CTAI.SugaredConverted, /*Final=*/true); - // Build up an EvaluationContext with an ImplicitConceptSpecializationDecl so - // that the template arguments of the constraint can be preserved. For - // example: - // - // template <class T> - // concept C = []<D U = void>() { return true; }(); - // - // We need the argument for T while evaluating type constraint D in - // building the CallExpr to the lambda. - EnterExpressionEvaluationContext EECtx( - S, Sema::ExpressionEvaluationContext::Unevaluated, - ImplicitConceptSpecializationDecl::Create( - S.getASTContext(), Concept->getDeclContext(), Concept->getLocation(), - CTAI.SugaredConverted)); if (S.CheckConstraintSatisfaction( Concept, AssociatedConstraint(Concept->getConstraintExpr()), MLTAL, TypeLoc.getLocalSourceRange(), Satisfaction)) diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 31d2a7d763c9e..309835b610a6e 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -1076,6 +1076,7 @@ namespace { DeclarationName Entity; // Whether to evaluate the C++20 constraints or simply substitute into them. bool EvaluateConstraints = true; + bool EvaluateLambdaConstraint = false; // Whether Substitution was Incomplete, that is, we tried to substitute in // any user provided template arguments which were null. bool IsIncomplete = false; @@ -1114,10 +1115,14 @@ namespace { inline static struct ForParameterMappingSubstitution_t { } ForParameterMappingSubstitution; + inline static struct ForConstraintSubstitution_t { + } ForConstraintSubstitution; + TemplateInstantiator(ForParameterMappingSubstitution_t, Sema &SemaRef, - SourceLocation Loc, + SourceLocation Loc, bool TransformLambdaConstraint, const MultiLevelTemplateArgumentList &TemplateArgs) : inherited(SemaRef), TemplateArgs(TemplateArgs), Loc(Loc), + EvaluateLambdaConstraint(TransformLambdaConstraint), BailOutOnIncomplete(false) { if (!SemaRef.CurrentCachedTemplateArgs) return; @@ -1127,6 +1132,13 @@ namespace { Arg.Profile(V, SemaRef.Context); } + TemplateInstantiator(ForConstraintSubstitution_t, Sema &SemaRef, + const MultiLevelTemplateArgumentList &TemplateArgs, + SourceLocation Loc, DeclarationName Entity, + bool BailOutOnIncomplete = false) + : inherited(SemaRef), TemplateArgs(TemplateArgs), Loc(Loc), + EvaluateLambdaConstraint(true), BailOutOnIncomplete(false) {} + /// Determine whether the given type \p T has already been /// transformed. /// @@ -1519,9 +1531,22 @@ namespace { if (TA.isDependent()) return CXXRecordDecl::LambdaDependencyKind::LDK_AlwaysDependent; } + if (auto *CD = dyn_cast_if_present<ImplicitConceptSpecializationDecl>( + LSI->Lambda->getLambdaContextDecl())) { + if (llvm::any_of(CD->getTemplateArguments(), + [](const auto &TA) { return TA.isDependent(); })) + return CXXRecordDecl::LambdaDependencyKind::LDK_AlwaysDependent; + } return inherited::ComputeLambdaDependency(LSI); } + ExprResult TransformLambdaConstraint(Expr *AC) { + if (AC && EvaluateLambdaConstraint) + return TransformExpr(const_cast<Expr *>(AC)); + + return AC; + } + ExprResult TransformLambdaExpr(LambdaExpr *E) { // Do not rebuild lambdas to avoid creating a new type. // Lambdas have already been processed inside their eval contexts. @@ -1632,12 +1657,24 @@ namespace { TemplateParameterList *OrigTPL) { if (!OrigTPL || !OrigTPL->size()) return OrigTPL; + std::optional<MultiLevelTemplateArgumentList> OldMLTAL; + // We need to preserve the lambda depth in parameter mapping. + // Otherwise the template argument deduction would fail, if we reduced the + // depth too early. + if (SemaRef.inParameterMappingSubstitution() && + OrigTPL->getDepth() >= TemplateArgs.getNumSubstitutedLevels()) + OldMLTAL = ForgetSubstitution(); + DeclContext *Owner = OrigTPL->getParam(0)->getDeclContext(); - TemplateDeclInstantiator DeclInstantiator(getSema(), - /* DeclContext *Owner */ Owner, - TemplateArgs); - DeclInstantiator.setEvaluateConstraints(EvaluateConstraints); - return DeclInstantiator.SubstTemplateParams(OrigTPL); + TemplateDeclInstantiator DeclInstantiator(getSema(), Owner, TemplateArgs); + // We don't want the template argument substitution into parameter + // mappings to preserve the outer depths. + DeclInstantiator.setEvaluateConstraints( + SemaRef.inConstraintSubstitution() || EvaluateConstraints); + auto *Transformed = DeclInstantiator.SubstTemplateParams(OrigTPL); + if (OldMLTAL) + RememberSubstitution(std::move(*OldMLTAL)); + return Transformed; } concepts::TypeRequirement * @@ -4184,10 +4221,10 @@ bool Sema::SubstTemplateArguments( bool Sema::SubstTemplateArgumentsInParameterMapping( ArrayRef<TemplateArgumentLoc> Args, SourceLocation BaseLoc, const MultiLevelTemplateArgumentList &TemplateArgs, - TemplateArgumentListInfo &Out) { + bool TransformLambdaConstraint, TemplateArgumentListInfo &Out) { TemplateInstantiator Instantiator( TemplateInstantiator::ForParameterMappingSubstitution, *this, BaseLoc, - TemplateArgs); + TransformLambdaConstraint, TemplateArgs); return Instantiator.TransformTemplateArguments(Args.begin(), Args.end(), Out); } @@ -4216,9 +4253,13 @@ Sema::SubstCXXIdExpr(Expr *E, ExprResult Sema::SubstConstraintExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) { - // FIXME: should call SubstExpr directly if this function is equivalent or - // should it be different? - return SubstExpr(E, TemplateArgs); + if (!E) + return E; + + TemplateInstantiator Instantiator( + TemplateInstantiator::ForConstraintSubstitution, *this, TemplateArgs, + SourceLocation(), DeclarationName()); + return Instantiator.TransformExpr(E); } ExprResult Sema::SubstConstraintExprWithoutSatisfaction( diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp index aad0d7da9420a..0cc7795e5d62e 100644 --- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -3632,7 +3632,7 @@ Decl *TemplateDeclInstantiator::VisitTemplateTypeParmDecl( TemplateTypeParmDecl *Inst = TemplateTypeParmDecl::Create( SemaRef.Context, Owner, D->getBeginLoc(), D->getLocation(), - D->getDepth() - TemplateArgs.getNumSubstitutedLevels(), D->getIndex(), + TemplateArgs.getNewDepth(D->getDepth()), D->getIndex(), D->getIdentifier(), D->wasDeclaredWithTypename(), D->isParameterPack(), D->hasTypeConstraint(), NumExpanded); @@ -4926,6 +4926,13 @@ TemplateDeclInstantiator::SubstTemplateParams(TemplateParameterList *L) { return nullptr; Expr *InstRequiresClause = L->getRequiresClause(); + if (InstRequiresClause && EvaluateConstraints) { + ExprResult E = + SemaRef.SubstConstraintExpr(InstRequiresClause, TemplateArgs); + if (E.isInvalid()) + return nullptr; + InstRequiresClause = E.get(); + } TemplateParameterList *InstL = TemplateParameterList::Create(SemaRef.Context, L->getTemplateLoc(), diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h index 3c8fcbe582b43..07f2dcf496f4b 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTrans... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/207966 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
