[clang] 9e0474f - Perform access checking to private members in simple requirement.
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 11 03:14:15 PST 2023
Author: Utkarsh Saxena
Date: 2023-01-11T12:13:16+01:00
New Revision: 9e0474fbb9c56725a1dfd1658837f07db73f4d8d
URL: https://github.com/llvm/llvm-project/commit/9e0474fbb9c56725a1dfd1658837f07db73f4d8d
DIFF: https://github.com/llvm/llvm-project/commit/9e0474fbb9c56725a1dfd1658837f07db73f4d8d.diff
LOG: Perform access checking to private members in simple requirement.
> 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
Added:
clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/AST/ExprConcepts.h
clang/lib/Parse/ParseExprCXX.cpp
clang/lib/Sema/SemaAccess.cpp
clang/lib/Sema/SemaConcept.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 927c85249b729..b55b2d6752972 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 <https://github.com/llvm/llvm-project/issues/50886>`_.
- Implemented CWG2635 as a Defect Report, which prohibits structured bindings from being constrained.
-
+- Correctly handle access-checks in requires expression. Fixes `GH53364 <https://github.com/llvm/llvm-project/issues/53364>`_,
+ `GH53334 <https://github.com/llvm/llvm-project/issues/53334>`_.
C++2b Feature Support
^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ExprConcepts.h b/clang/include/clang/AST/ExprConcepts.h
index a358a2a41619e..f02c140c14c19 100644
--- a/clang/include/clang/AST/ExprConcepts.h
+++ b/clang/include/clang/AST/ExprConcepts.h
@@ -528,6 +528,12 @@ class RequiresExpr final : public Expr,
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 5017f31a1ecb2..7f09120574a7a 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 00d3efd19d7a2..cbda62497e6a6 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<TemplateDecl>(D)) {
if (isa<DeclContext>(TD->getTemplatedDecl()))
DC = cast<DeclContext>(TD->getTemplatedDecl());
+ } else if (auto *RD = dyn_cast<RequiresExprBodyDecl>(D)) {
+ DC = RD;
}
EffectiveContext EC(DC);
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index cab013de95c12..af920693859e4 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<RequiresExpr>(SubstExpr)) {
+ // FIXME: RequiresExpr should store dependent diagnostics.
for (concepts::Requirement *Req : RE->getRequirements())
if (!Req->isDependent() && !Req->isSatisfied()) {
if (auto *E = dyn_cast<concepts::ExprRequirement>(Req))
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 97571837e6148..b43a754a58ce7 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<RequiresExpr>()->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 e491e039026b8..abfadfa348841 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<int(*)[(typeid(x), 0)]>(nullptr); };
// expected-error at -1{{constraint variable 'x' cannot be used in an evaluated context}}
// expected-note at -2{{'x' declared here}}
+
+namespace access_checks {
+namespace in_requires_expression {
+template<auto>
+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<auto x>
+constexpr bool A<x>::foo() {
+ return requires(B b) { b.p(); };
+}
+static_assert(!A<1>::foo());
+static_assert(A<0>::foo());
+
+template<auto x>
+constexpr bool A<x>::bar() {
+ return requires() { B::static_member; };
+}
+static_assert(!A<1>::bar());
+static_assert(A<0>::bar());
+
+template<auto x>
+constexpr bool A<x>::baz() {
+ return requires(B b) { b.data_member; };
+}
+static_assert(!A<1>::baz());
+static_assert(A<0>::baz());
+
+template<auto x>
+constexpr bool A<x>::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<int N> class A;
+
+template <> class A<0> {
+ static void f() {}
+};
+template<int N>
+concept C1 = requires() { A<N>::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<class T>
+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<int x>
+constexpr bool template_func() {
+ return requires() {
+ A<x>::f();
+ };
+}
+static_assert(!template_func<0>());
+static_assert(template_func<1>());
+} // namespace in_concepts
+
+namespace in_trailing_requires {
+template <class> struct B;
+class A {
+ static void f();
+ friend struct B<short>;
+};
+
+template <class T> struct B {
+ static constexpr int index() requires requires{ A::f(); } {
+ return 1;
+ }
+ static constexpr int index() {
+ return 2;
+ }
+};
+
+static_assert(B<short>::index() == 1);
+static_assert(B<int>::index() == 2);
+
+namespace missing_member_function {
+template <class T> struct Use;
+class X {
+ int a;
+ static int B;
+ friend struct Use<short>;
+};
+template <class T> 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<int>::foo(); //expected-error {{invalid reference to function 'foo': constraints not satisfied}}
+ static_assert(Use<short>::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 0000000000000..c23850219c8a8
--- /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<int x>
+struct A { static constexpr bool far(); };
+class B {
+ bool data_member;
+ friend struct A<1>;
+};
+
+template<>
+constexpr bool A<0>::far() { return true; }
+
+template<int x>
+constexpr bool A<x>::far() {
+ return requires(B b) {
+ b.data_member;
+ requires A<x-1>::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 at -1{{recursive template instantiation exceeded maximum depth}}
+ };
+}
+static_assert(A<1>::far());
+static_assert(!A<10001>::far()); // expected-note {{in instantiation of member function}}
More information about the cfe-commits
mailing list