https://github.com/ilya-biryukov updated https://github.com/llvm/llvm-project/pull/74490
>From 51d6e95d67c3cc6070d926ef04b168c2ea429c7d Mon Sep 17 00:00:00 2001 From: Ilya Biryukov <ibiryu...@google.com> Date: Tue, 5 Dec 2023 14:28:01 +0100 Subject: [PATCH 1/2] [Sema] When checking for constraint equivalence, do not calculate satisfaction ... and only look at equivalence of substituted expressions, not results of constraint satisfaction. Fixes #74314. There is already some existing machinery for that in `TemplateInstantiator` and `Sema` exposed separate functions for substituting expressions with intention to do that: - `Sema::SubstExpr` should not evaluate constraints. - `Sema::SubstConstraintExpr` should. However, both functions used to be equivalent. This commit changes the former to actually avoid calculating constraint satisfaction and uses it in the code path that matches definition and declaration. --- clang/include/clang/Sema/Template.h | 1 + clang/lib/Sema/SemaConcept.cpp | 15 ++++++------- clang/lib/Sema/SemaTemplateInstantiate.cpp | 8 +++++++ .../SemaTemplate/concepts-out-of-line-def.cpp | 21 +++++++++++++++++++ 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h index 2a553054a0ce51..ce44aca797b0fb 100644 --- a/clang/include/clang/Sema/Template.h +++ b/clang/include/clang/Sema/Template.h @@ -564,6 +564,7 @@ enum class TemplateSubstitutionKind : char { const MultiLevelTemplateArgumentList &TemplateArgs; Sema::LateInstantiatedAttrVec* LateAttrs = nullptr; LocalInstantiationScope *StartingScope = nullptr; + // Whether to evaluate the C++20 constraints or simply substitute into them. bool EvaluateConstraints = true; /// A list of out-of-line class template partial diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp index 719c6aab74e017..be3541573377bb 100644 --- a/clang/lib/Sema/SemaConcept.cpp +++ b/clang/lib/Sema/SemaConcept.cpp @@ -771,10 +771,9 @@ namespace { }; } // namespace -static const Expr * -SubstituteConstraintExpression(Sema &S, - const Sema::TemplateCompareNewDeclInfo &DeclInfo, - const Expr *ConstrExpr) { +static const Expr *SubstituteConstraintExpressionWithoutSatisfaction( + Sema &S, const Sema::TemplateCompareNewDeclInfo &DeclInfo, + const Expr *ConstrExpr) { MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs( DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(), /*Final=*/false, /*Innermost=*/nullptr, @@ -798,7 +797,7 @@ SubstituteConstraintExpression(Sema &S, if (auto *RD = dyn_cast<CXXRecordDecl>(DeclInfo.getDeclContext())) ThisScope.emplace(S, const_cast<CXXRecordDecl *>(RD), Qualifiers()); ExprResult SubstConstr = - S.SubstConstraintExpr(const_cast<clang::Expr *>(ConstrExpr), MLTAL); + S.SubstExpr(const_cast<clang::Expr *>(ConstrExpr), MLTAL); if (SFINAE.hasErrorOccurred() || !SubstConstr.isUsable()) return nullptr; return SubstConstr.get(); @@ -814,12 +813,14 @@ bool Sema::AreConstraintExpressionsEqual(const NamedDecl *Old, if (Old && !New.isInvalid() && !New.ContainsDecl(Old) && Old->getLexicalDeclContext() != New.getLexicalDeclContext()) { if (const Expr *SubstConstr = - SubstituteConstraintExpression(*this, Old, OldConstr)) + SubstituteConstraintExpressionWithoutSatisfaction(*this, Old, + OldConstr)) OldConstr = SubstConstr; else return false; if (const Expr *SubstConstr = - SubstituteConstraintExpression(*this, New, NewConstr)) + SubstituteConstraintExpressionWithoutSatisfaction(*this, New, + NewConstr)) NewConstr = SubstConstr; else return false; diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 6ad70109c8cee9..971a4c99add0cb 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -1186,6 +1186,7 @@ namespace { const MultiLevelTemplateArgumentList &TemplateArgs; SourceLocation Loc; DeclarationName Entity; + // Whether to evaluate the C++20 constraints or simply substitute into them. bool EvaluateConstraints = true; public: @@ -2489,6 +2490,12 @@ TemplateInstantiator::TransformNestedRequirement( Req->getConstraintExpr()->getBeginLoc(), Req, Sema::InstantiatingTemplate::ConstraintsCheck{}, Req->getConstraintExpr()->getSourceRange()); + if (!getEvaluateConstraints()) { + ExprResult TransConstraint = TransformExpr(Req->getConstraintExpr()); + if (TransConstraint.isInvalid() || !TransConstraint.get()) + return nullptr; + return RebuildNestedRequirement(TransConstraint.get()); + } ExprResult TransConstraint; ConstraintSatisfaction Satisfaction; @@ -4077,6 +4084,7 @@ Sema::SubstExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) { TemplateInstantiator Instantiator(*this, TemplateArgs, SourceLocation(), DeclarationName()); + Instantiator.setEvaluateConstraints(false); return Instantiator.TransformExpr(E); } diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp index f134394615fb24..7e79eeb6d279a0 100644 --- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp +++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp @@ -504,3 +504,24 @@ struct bar { bar<int> x; } // namespace GH61763 + +namespace GH74314 { +template <class T, class U> constexpr bool is_same_v = __is_same(T, U); +template <class T, class U> constexpr bool is_not_same_v = !__is_same(T, U); + +template <class Result> +concept something_interesting = requires { + true; + requires is_same_v<int, Result>; +}; + +template <class T> +struct X { + void foo() requires requires { requires is_not_same_v<T, int>; }; +}; + +template <class T> +void X<T>::foo() requires requires { requires something_interesting<T>; } {} +// expected-error@-1{{definition of 'foo' does not match any declaration}} +// expected-note@*{{}} +} // namespace GH74314 \ No newline at end of file >From 87a9ac815782723c63b0d57935049ba4e67c274f Mon Sep 17 00:00:00 2001 From: Ilya Biryukov <ibiryu...@google.com> Date: Thu, 21 Dec 2023 14:58:24 +0100 Subject: [PATCH 2/2] Provide a more localized fix, add tests with requires expressions inside of decltype, unifty implementation of SubstConstraintExpr and SubstExpr and add corresponding comments --- clang/include/clang/Sema/Sema.h | 6 ++++-- clang/lib/Sema/SemaConcept.cpp | 4 ++-- clang/lib/Sema/SemaTemplateInstantiate.cpp | 18 ++++++++++++++---- .../SemaTemplate/concepts-out-of-line-def.cpp | 11 +++++++++++ 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 38377f01a10086..f8ae488cdf2bda 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -10240,11 +10240,13 @@ class Sema final { ~ConstraintEvalRAII() { TI.setEvaluateConstraints(OldValue); } }; - // Unlike the above, this evaluates constraints, which should only happen at - // 'constraint checking' time. + // Must be used instead of SubstExpr at 'constraint checking' time. ExprResult SubstConstraintExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs); + // Unlike the above, this does not evaluates constraints. + ExprResult SubstConstraintExprWithoutSatisfaction( + Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs); /// Substitute the given template arguments into a list of /// expressions, expanding pack expansions if required. diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp index be3541573377bb..acfc00f4125407 100644 --- a/clang/lib/Sema/SemaConcept.cpp +++ b/clang/lib/Sema/SemaConcept.cpp @@ -796,8 +796,8 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction( std::optional<Sema::CXXThisScopeRAII> ThisScope; if (auto *RD = dyn_cast<CXXRecordDecl>(DeclInfo.getDeclContext())) ThisScope.emplace(S, const_cast<CXXRecordDecl *>(RD), Qualifiers()); - ExprResult SubstConstr = - S.SubstExpr(const_cast<clang::Expr *>(ConstrExpr), MLTAL); + ExprResult SubstConstr = S.SubstConstraintExprWithoutSatisfaction( + const_cast<clang::Expr *>(ConstrExpr), MLTAL); if (SFINAE.hasErrorOccurred() || !SubstConstr.isUsable()) return nullptr; return SubstConstr.get(); diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 971a4c99add0cb..dee7d1196e3227 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -2494,7 +2494,12 @@ TemplateInstantiator::TransformNestedRequirement( ExprResult TransConstraint = TransformExpr(Req->getConstraintExpr()); if (TransConstraint.isInvalid() || !TransConstraint.get()) return nullptr; - return RebuildNestedRequirement(TransConstraint.get()); + if (TransConstraint.get()->isInstantiationDependent()) + return new (SemaRef.Context) + concepts::NestedRequirement(TransConstraint.get()); + ConstraintSatisfaction Satisfaction; + return new (SemaRef.Context) concepts::NestedRequirement( + SemaRef.Context, TransConstraint.get(), Satisfaction); } ExprResult TransConstraint; @@ -4084,20 +4089,25 @@ Sema::SubstExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) { TemplateInstantiator Instantiator(*this, TemplateArgs, SourceLocation(), DeclarationName()); - Instantiator.setEvaluateConstraints(false); return Instantiator.TransformExpr(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); +} + +ExprResult Sema::SubstConstraintExprWithoutSatisfaction( + Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) { if (!E) return E; - // This is where we need to make sure we 'know' constraint checking needs to - // happen. TemplateInstantiator Instantiator(*this, TemplateArgs, SourceLocation(), DeclarationName()); + Instantiator.setEvaluateConstraints(false); return Instantiator.TransformExpr(E); } diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp index 7e79eeb6d279a0..ea21f2720e45af 100644 --- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp +++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp @@ -518,10 +518,21 @@ concept something_interesting = requires { template <class T> struct X { void foo() requires requires { requires is_not_same_v<T, int>; }; + void bar(decltype(requires { requires is_not_same_v<T, int>; })); }; template <class T> void X<T>::foo() requires requires { requires something_interesting<T>; } {} // expected-error@-1{{definition of 'foo' does not match any declaration}} // expected-note@*{{}} + +template <class T> +void X<T>::foo() requires requires { requires is_not_same_v<T, int>; } {} // ok + +template <class T> +void X<T>::bar(decltype(requires { requires something_interesting<T>; })) {} +// expected-error@-1{{definition of 'bar' does not match any declaration}} + +template <class T> +void X<T>::bar(decltype(requires { requires is_not_same_v<T, int>; })) {} } // namespace GH74314 \ No newline at end of file _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits