From 9e0474fbb9c56725a1dfd1658837f07db73f4d8d Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Thu, 22 Dec 2022 05:05:31 +0100 Subject: [PATCH] Perform access checking to private members in simple requirement. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit > Dependent access checks. Fixes: https://github.com/llvm/llvm-project/issues/53364 We previously ignored dependent access checks to private members. These are visible only to the `RequiresExprBodyExpr` (through `PerformDependentDiagnositcs`) and not to the individual requirements. --- > Non-dependent access checks. Fixes: https://github.com/llvm/llvm-project/issues/53334 Access to members in a non-dependent context would always yield an invalid expression. When it appears in a requires-expression, then this is a hard error as this would always result in a substitution failure. https://eel.is/c++draft/expr.prim.req#general-note-1 > Note 1: If a requires-expression contains invalid types or expressions in its requirements, and it does not appear within the declaration of a templated entity, then the program is ill-formed. — end note] > If the substitution of template arguments into a requirement would always result in a substitution failure, the program is ill-formed; no diagnostic required. The main issue here is the delaying of the diagnostics. Use a `ParsingDeclRAIIObject` creates a separate diagnostic pool for diagnositcs associated to the `RequiresExprBodyDecl`. This is important because dependent diagnostics should not be leaked/delayed to higher scopes (Eg. inside a template function or in a trailing requires). These dependent diagnostics must be attached to the `DeclContext` of the parameters of `RequiresExpr` (which is the `RequiresExprBodyDecl` in this case). Non dependent diagnostics, on the other hand, should not delayed and surfaced as hard errors. Differential Revision: https://reviews.llvm.org/D140547 --- clang/docs/ReleaseNotes.rst | 3 +- clang/include/clang/AST/ExprConcepts.h | 6 + clang/lib/Parse/ParseExprCXX.cpp | 5 + clang/lib/Sema/SemaAccess.cpp | 2 + clang/lib/Sema/SemaConcept.cpp | 1 + clang/lib/Sema/SemaTemplateInstantiate.cpp | 18 ++- .../expr.prim/expr.prim.req/simple-requirement.cpp | 135 +++++++++++++++++++++ .../SemaCXX/invalid-requirement-requires-expr.cpp | 23 ++++ 8 files changed, 191 insertions(+), 2 deletions(-) create mode 100644 clang/test/SemaCXX/invalid-requirement-requires-expr.cpp diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 927c852..b55b2d6 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -735,7 +735,8 @@ C++20 Feature Support - Do not hide templated base members introduced via using-decl in derived class (useful specially for constrained members). Fixes `GH50886 `_. - Implemented CWG2635 as a Defect Report, which prohibits structured bindings from being constrained. - +- Correctly handle access-checks in requires expression. Fixes `GH53364 `_, + `GH53334 `_. C++2b Feature Support ^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/AST/ExprConcepts.h b/clang/include/clang/AST/ExprConcepts.h index a358a2a..f02c140 100644 --- a/clang/include/clang/AST/ExprConcepts.h +++ b/clang/include/clang/AST/ExprConcepts.h @@ -528,6 +528,12 @@ public: return RequiresExprBits.IsSatisfied; } + void setSatisfied(bool IsSatisfied) { + assert(!isValueDependent() && + "setSatisfied called on a dependent RequiresExpr"); + RequiresExprBits.IsSatisfied = IsSatisfied; + } + SourceLocation getRequiresKWLoc() const { return RequiresExprBits.RequiresKWLoc; } diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp index 5017f31..7f09120 100644 --- a/clang/lib/Parse/ParseExprCXX.cpp +++ b/clang/lib/Parse/ParseExprCXX.cpp @@ -3509,6 +3509,10 @@ ExprResult Parser::ParseRequiresExpression() { Actions, Sema::ExpressionEvaluationContext::Unevaluated); ParseScope BodyScope(this, Scope::DeclScope); + // Create a separate diagnostic pool for RequiresExprBodyDecl. + // Dependent diagnostics are attached to this Decl and non-depenedent + // diagnostics are surfaced after this parse. + ParsingDeclRAIIObject ParsingBodyDecl(*this, ParsingDeclRAIIObject::NoParent); RequiresExprBodyDecl *Body = Actions.ActOnStartRequiresExpr( RequiresKWLoc, LocalParameterDecls, getCurScope()); @@ -3746,6 +3750,7 @@ ExprResult Parser::ParseRequiresExpression() { } Braces.consumeClose(); Actions.ActOnFinishRequiresExpr(); + ParsingBodyDecl.complete(Body); return Actions.ActOnRequiresExpr(RequiresKWLoc, Body, LocalParameterDecls, Requirements, Braces.getCloseLocation()); } diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp index 00d3efd..cbda624 100644 --- a/clang/lib/Sema/SemaAccess.cpp +++ b/clang/lib/Sema/SemaAccess.cpp @@ -1493,6 +1493,8 @@ void Sema::HandleDelayedAccessCheck(DelayedDiagnostic &DD, Decl *D) { } else if (TemplateDecl *TD = dyn_cast(D)) { if (isa(TD->getTemplatedDecl())) DC = cast(TD->getTemplatedDecl()); + } else if (auto *RD = dyn_cast(D)) { + DC = RD; } EffectiveContext EC(DC); diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp index cab013d..af92069 100644 --- a/clang/lib/Sema/SemaConcept.cpp +++ b/clang/lib/Sema/SemaConcept.cpp @@ -1002,6 +1002,7 @@ static void diagnoseWellFormedUnsatisfiedConstraintExpr(Sema &S, S.DiagnoseUnsatisfiedConstraint(CSE->getSatisfaction()); return; } else if (auto *RE = dyn_cast(SubstExpr)) { + // FIXME: RequiresExpr should store dependent diagnostics. for (concepts::Requirement *Req : RE->getRequirements()) if (!Req->isDependent() && !Req->isSatisfied()) { if (auto *E = dyn_cast(Req)) diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 9757183..b43a754 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -15,6 +15,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/ASTLambda.h" #include "clang/AST/ASTMutationListener.h" +#include "clang/AST/DeclBase.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprConcepts.h" @@ -1362,7 +1363,22 @@ namespace { ExprResult TransformRequiresExpr(RequiresExpr *E) { LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true); - return inherited::TransformRequiresExpr(E); + auto TransReq = inherited::TransformRequiresExpr(E); + if (TransReq.isInvalid()) + return TransReq; + assert(TransReq.get() != E && + "Do not change value of isSatisfied for the existing expression. " + "Create a new expression instead."); + if (E->getBody()->isDependentContext()) { + Sema::SFINAETrap Trap(SemaRef); + // We recreate the RequiresExpr body, but not by instantiating it. + // Produce pending diagnostics for dependent access check. + SemaRef.PerformDependentDiagnostics(E->getBody(), TemplateArgs); + // FIXME: Store SFINAE diagnostics in RequiresExpr for diagnosis. + if (Trap.hasErrorOccurred()) + TransReq.getAs()->setSatisfied(false); + } + return TransReq; } bool TransformRequiresExprRequirements( diff --git a/clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp b/clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp index e491e03..abfadfa 100644 --- a/clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp +++ b/clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp @@ -104,3 +104,138 @@ class X { virtual ~X(); }; constexpr bool b = requires (X &x) { static_cast(nullptr); }; // expected-error@-1{{constraint variable 'x' cannot be used in an evaluated context}} // expected-note@-2{{'x' declared here}} + +namespace access_checks { +namespace in_requires_expression { +template +struct A { + static constexpr bool foo(); + static constexpr bool bar(); + static constexpr bool baz(); + static constexpr bool faz(); +}; + +class C{}; + +class B { + void p() {} + bool data_member = true; + static const bool static_member = true; + friend struct A<0>; +}; + +template +constexpr bool A::foo() { + return requires(B b) { b.p(); }; +} +static_assert(!A<1>::foo()); +static_assert(A<0>::foo()); + +template +constexpr bool A::bar() { + return requires() { B::static_member; }; +} +static_assert(!A<1>::bar()); +static_assert(A<0>::bar()); + +template +constexpr bool A::baz() { + return requires(B b) { b.data_member; }; +} +static_assert(!A<1>::baz()); +static_assert(A<0>::baz()); + +template +constexpr bool A::faz() { + return requires(B a, B b) { + a.p(); + b.data_member; + B::static_member; + }; +} +static_assert(!A<1>::faz()); +static_assert(A<0>::faz()); +} // namespace in_requires_expression + +namespace in_concepts { +// Dependent access does not cause hard errors. +template class A; + +template <> class A<0> { + static void f() {} +}; +template +concept C1 = requires() { A::f(); }; +static_assert(!C1<0>); + +template <> class A<1> { +public: + static void f() {} +}; +static_assert(C1<1>); + +// Non-dependent access to private member is a hard error. +class B{ + static void f() {} // expected-note 2{{implicitly declared private here}} +}; +template +concept C2 = requires() { B::f(); }; // expected-error {{'f' is a private member}} + +constexpr bool non_template_func() { + return requires() { + B::f(); // expected-error {{'f' is a private member}} + }; +} +template +constexpr bool template_func() { + return requires() { + A::f(); + }; +} +static_assert(!template_func<0>()); +static_assert(template_func<1>()); +} // namespace in_concepts + +namespace in_trailing_requires { +template struct B; +class A { + static void f(); + friend struct B; +}; + +template struct B { + static constexpr int index() requires requires{ A::f(); } { + return 1; + } + static constexpr int index() { + return 2; + } +}; + +static_assert(B::index() == 1); +static_assert(B::index() == 2); + +namespace missing_member_function { +template struct Use; +class X { + int a; + static int B; + friend struct Use; +}; +template struct Use { + constexpr static int foo() requires requires(X x) { x.a; } { + return 1; + } + constexpr static int bar() requires requires { X::B; } { + return 1; + } +}; + +void test() { + // FIXME: Propagate diagnostic. + Use::foo(); //expected-error {{invalid reference to function 'foo': constraints not satisfied}} + static_assert(Use::foo() == 1); +} +} // namespace missing_member_function +} // namespace in_trailing_requires +} // namespace access_check diff --git a/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp b/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp new file mode 100644 index 0000000..c238502 --- /dev/null +++ b/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp @@ -0,0 +1,23 @@ +// RUN: %clang_cc1 %s -I%S -std=c++2a -verify + +// RequiresExpr contains invalid requirement. (Eg. Highly recurisive template). +template +struct A { static constexpr bool far(); }; +class B { + bool data_member; + friend struct A<1>; +}; + +template<> +constexpr bool A<0>::far() { return true; } + +template +constexpr bool A::far() { + return requires(B b) { + b.data_member; + requires A::far(); //expected-note 3{{in instantiation}} // expected-note 6{{while}} expected-note {{contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all}} + // expected-error@-1{{recursive template instantiation exceeded maximum depth}} + }; +} +static_assert(A<1>::far()); +static_assert(!A<10001>::far()); // expected-note {{in instantiation of member function}} -- 2.7.4