[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 marked 4 inline comments as done.
usaxena95 added a comment.

I have deleted the test in 
https://github.com/llvm/llvm-project/commit/a3b632ab8772237ae23638f702bdceda028b2016.
It is safe to delete as this is a brittle test trying to generate an invalid 
requirement using very deep template instantiation. I would find a better way 
to test this in another forward patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: aaron.ballman.
erichkeane added a comment.

In D140547#4050936 , @ilya-biryukov 
wrote:

> In D140547#4050752 , @uabelho wrote:
>
>> Anyone else see this?
>
> I have not checked, but I would not be surprised if we hit the stack size 
> limits with asan enabled
> @usaxena95, maybe reduce the number of instantiations from `10001` to `1001` 
> or `101`? It should not change the intention of the test and fix this failure.

This is a regression that was noticed by @aaron.ballman even without ASAN, so 
this is something that needs doing ASAP.  The branch is happening around the 
24th, so this needs to be fixed by then.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1366
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
-  return inherited::TransformRequiresExpr(E);
+  auto TransReq = inherited::TransformRequiresExpr(E);
+  if (TransReq.isInvalid())

'auto' isn't allowed here by coding standard, this needs to be ExprResult.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D140547#4050752 , @uabelho wrote:

> Anyone else see this?

I have not checked, but I would not be surprised if we hit the stack size 
limits with asan enabled
@usaxena95, maybe reduce the number of instantiations from `10001` to `1001` or 
`101`? It should not change the intention of the test and fix this failure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

Hi,

When I run the new testcase
 clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
with ASAN binaries the test fails like

  FAIL: Clang :: SemaCXX/invalid-requirement-requires-expr.cpp (1 of 1)
   TEST 'Clang :: 
SemaCXX/invalid-requirement-requires-expr.cpp' FAILED 
  Script:
  --
  : 'RUN: at line 1';   
/repo/uabelho/main-github/llvm/build-all-bbisdk-asan/bin/clang -cc1 
-internal-isystem 
/repo/uabelho/main-github/llvm/build-all-bbisdk-asan/lib/clang/16/include 
-nostdsysteminc 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 -I/repo/uabelho/main-github/clang/test/SemaCXX -std=c++2a -verify
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  error: 'warning' diagnostics seen but not expected: 
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: stack nearly exhausted; compilation time may suffer, and crashes due 
to stack overflow are likely
  error: 'note' diagnostics seen but not expected: 
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: while checking the satisfaction of nested requirement requested here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: in instantiation of member function 'A<9660>::far' requested here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: while substituting template arguments into constraint expression here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: while checking the satisfaction of nested requirement requested here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: in instantiation of requirement here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: (skipping 1697 contexts in backtrace; use 
-ftemplate-backtrace-limit=0 to see all)
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: while substituting template arguments into constraint expression here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: while checking the satisfaction of nested requirement requested here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: in instantiation of requirement here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 18: while checking the satisfaction of nested requirement requested here
File 
/repo/uabelho/main-github/clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
 Line 23: in instantiation of member function 'A<10001>::far' requested here
  12 errors generated.
  
  --
  
  
  
  Failed Tests (1):
Clang :: SemaCXX/invalid-requirement-requires-expr.cpp
  
  
  Testing Time: 0.16s
Failed: 1

Anyone else see this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:739
+- Correctly handle access-checks in requires expression. Fixes `GH53364 
`_,
+  `GH53334 `_.
 C++2b Feature Support

erichkeane wrote:
> RKSimon wrote:
> > @usaxena95 This has broken the sphinx buildbot - please can you take a 
> > look? https://lab.llvm.org/buildbot/#/builders/92/builds/38512
> Looks like he already fixed it in 75c0d43e857dd
Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:739
+- Correctly handle access-checks in requires expression. Fixes `GH53364 
`_,
+  `GH53334 `_.
 C++2b Feature Support

RKSimon wrote:
> @usaxena95 This has broken the sphinx buildbot - please can you take a look? 
> https://lab.llvm.org/buildbot/#/builders/92/builds/38512
Looks like he already fixed it in 75c0d43e857dd


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:739
+- Correctly handle access-checks in requires expression. Fixes `GH53364 
`_,
+  `GH53334 `_.
 C++2b Feature Support

@usaxena95 This has broken the sphinx buildbot - please can you take a look? 
https://lab.llvm.org/buildbot/#/builders/92/builds/38512


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e0474fbb9c5: Perform access checking to private members in 
simple requirement. (authored by usaxena95).

Changed prior to commit:
  https://reviews.llvm.org/D140547?vs=487866=488136#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  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
  clang/test/SemaCXX/invalid-requirement-requires-expr.cpp

Index: clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
===
--- /dev/null
+++ 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}}
Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,138 @@
 constexpr bool b = requires (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.
+  

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 487866.
usaxena95 added a comment.

Remove new lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  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
  clang/test/SemaCXX/invalid-requirement-requires-expr.cpp

Index: clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
===
--- /dev/null
+++ 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}}
Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,138 @@
 constexpr bool b = requires (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
Index: 

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1375
+if (Trap.hasErrorOccurred())
+  TransReq.getAs()->setSatisfied(false);
+  }

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > `TransReq` may be `ExprError` and this will cause a crash. Worth adding a 
> > test too.
> Could you please add `assert(!TransReq || *TransReq != E)`.
> The common optimization in TreeTransform is to avoid rebuilding the AST nodes 
> if nothing changes. There is no optimization like this for `RequireExpr` 
> right now, but it would not be unexpected if this gets implemented in the 
> future.
> 
> In those situations, the current code can potentially change value of 
> `isSatisfied` for an existing expression rather than for a newly created, 
> which seems like asking for trouble. It would be nice to give an early 
> warning to implementors of this optimization that they should think how to 
> handle this case.
Thanks for spotting. Adding a test with invalid requirement which caused a 
crash.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 487865.
usaxena95 marked 6 inline comments as done.
usaxena95 added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  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
  clang/test/SemaCXX/invalid-requirement-requires-expr.cpp

Index: clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-requirement-requires-expr.cpp
@@ -0,0 +1,24 @@
+
+// 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}}
Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,138 @@
 constexpr bool b = requires (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 

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I think the only major problem is not checking for error case when accessing 
`TransReq`, the rest are NITs.
Thanks for the change! Will be happy to LGTM it as soon as the access to 
`TransReq` is fixed.




Comment at: clang/lib/Parse/ParseExprCXX.cpp:3512
   ParseScope BodyScope(this, Scope::DeclScope);
+  ParsingDeclRAIIObject ParsingBodyDecl(*this, 
ParsingDeclRAIIObject::NoParent);
   RequiresExprBodyDecl *Body = Actions.ActOnStartRequiresExpr(

NIT: could you add a comment explaining that we need this helper in order to 
capture dependent diagnostics properly?



Comment at: clang/lib/Sema/SemaConcept.cpp:1005
   } else if (auto *RE = dyn_cast(SubstExpr)) {
+// TODO(usx): Store and diagnose dependent diagnositcs here.
 for (concepts::Requirement *Req : RE->getRequirements())

NIT: `s/Store/RequiresExpr should store dependent diagnostics`. I was confused 
at first and thought we need to store something in this function rather than 
the other place.
NIT2: Use of `FIXME` is more common in LLVM.
NIT3:  Google LDAP `usx` might be trickier to find in LLVM communication 
channels, I suggest removing it completely or using a full name instead.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1373
+SemaRef.PerformDependentDiagnostics(E->getBody(), TemplateArgs);
+// TODO(usx): Store SFINAE diagnostics in RequiresExpr for diagnosis.
+if (Trap.hasErrorOccurred())

NIT: LLVM uses FIXME more often. 



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1375
+if (Trap.hasErrorOccurred())
+  TransReq.getAs()->setSatisfied(false);
+  }

`TransReq` may be `ExprError` and this will cause a crash. Worth adding a test 
too.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1375
+if (Trap.hasErrorOccurred())
+  TransReq.getAs()->setSatisfied(false);
+  }

ilya-biryukov wrote:
> `TransReq` may be `ExprError` and this will cause a crash. Worth adding a 
> test too.
Could you please add `assert(!TransReq || *TransReq != E)`.
The common optimization in TreeTransform is to avoid rebuilding the AST nodes 
if nothing changes. There is no optimization like this for `RequireExpr` right 
now, but it would not be unexpected if this gets implemented in the future.

In those situations, the current code can potentially change value of 
`isSatisfied` for an existing expression rather than for a newly created, which 
seems like asking for trouble. It would be nice to give an early warning to 
implementors of this optimization that they should think how to handle this 
case.



Comment at: 
clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp:235
+void test() {
+  // TODO: Propagate diagnostic.
+  Use::foo(); //expected-error {{invalid reference to function 'foo': 
constraints not satisfied}}

NIT: FIXME


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 486604.
usaxena95 added a comment.

Use ParsingDeclRAIIObject instead of ContextRAII.

This creates a separate diagnostic pool for diagnositcs associated to the 
RequiresExprBodyDecl.
This is important because dependent diagnostics should not leak to higher 
scopes (Eg. inside a template function or in a trailing requires). These 
dependent diagnstics must be attached to the DeclContext of the parameters of 
RequiresExpr (which is the BodyDecl in this case).
Non dependent diagnostics should not delayed and surfaced as hard errors.

This addresses the previously failing LibCXX failure as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  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

Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,138 @@
 constexpr bool b = requires (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() {
+  // TODO: 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
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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"
@@ -1363,7 +1364,17 @@
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
   

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 486357.
usaxena95 added a comment.

Return a valid RequriesExpr instead of a ExprError in case of depenedent 
diagnositcs. 
We would also need to store the diagnositcs in the RequiresExpr for better 
diagnosis.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  clang/include/clang/AST/ExprConcepts.h
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp

Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,138 @@
 constexpr bool b = requires (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() {
+  // TODO: 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
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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"
@@ -1363,7 +1364,17 @@
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
-  return inherited::TransformRequiresExpr(E);
+  auto TransReq = inherited::TransformRequiresExpr(E);
+  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.

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 486313.
usaxena95 marked an inline comment as done.
usaxena95 added a comment.

Added more tests. Still investigating libcxx failure and tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp

Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,138 @@
 constexpr bool b = requires (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: This should be an error.
+  Use::foo();
+  static_assert(Use::foo() == 1);
+}
+} // namespace missing_member_function
+} // namespace in_trailing_requires
+} // namespace access_check
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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"
@@ -1363,7 +1364,16 @@
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
-  return inherited::TransformRequiresExpr(E);
+  auto TransReq = inherited::TransformRequiresExpr(E);
+  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);
+if (Trap.hasErrorOccurred())
+  return ExprError();
+  }
+  return TransReq;
 }
 
 bool 

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 marked an inline comment as done.
usaxena95 added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1367
+  if (E->getBody()->isDependentContext()) {
+Sema::SFINAETrap Trap(SemaRef);
+// We recreate the RequiresExpr body, but not by instantiating it.

ilya-biryukov wrote:
> Other uses of `PerformDependentDiagnostics` do not add an explicit 
> `SFINAETrap` AFAICS.
> Why is `RequiresExpr` special? Because it should "eat" the errors and only 
> return a value?
Yes. Precisely. 



Comment at: 
clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp:157
+static_assert(A<0>::faz());
+}

ilya-biryukov wrote:
> Could you add a check that in the following case we mention access check in 
> the note to the `no matching function to call` error?
> 
> ```
> template  struct Use;
> 
> class X { int a; friend struct Use; };
> 
> template  struct Use {
>   static void foo() requires (requires (X x) { x.a; }) {
>   }
> };
> 
> void test() {
> Use::foo();
> }
> ```
Added. This does not produce any diagnostics. Investigating!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 486255.
usaxena95 marked 3 inline comments as done.
usaxena95 added a comment.

Adding access check related changes from https://reviews.llvm.org/D140876


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp

Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,129 @@
 constexpr bool b = requires (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 {
+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 access_check_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 access_check_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; friend struct Use; };
+template  struct Use {
+  constexpr static int foo() requires requires(X x) { x.a; } {
+return 1;
+  }
+};
+
+void test() {
+  // FIXME: This should be an error.
+  Use::foo();
+  static_assert(Use::foo() == 1);
+}
+}
+}
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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"
@@ -1363,7 +1364,16 @@
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
-  return inherited::TransformRequiresExpr(E);
+  auto TransReq = inherited::TransformRequiresExpr(E);
+  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);
+if (Trap.hasErrorOccurred())
+  return ExprError();
+  }
+  return TransReq;
 }
 
 bool TransformRequiresExprRequirements(
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -3507,6 +3507,7 @@
   //   Expressions appearing within 

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1367
+  if (E->getBody()->isDependentContext()) {
+Sema::SFINAETrap Trap(SemaRef);
+// We recreate the RequiresExpr body, but not by instantiating it.

Other uses of `PerformDependentDiagnostics` do not add an explicit `SFINAETrap` 
AFAICS.
Why is `RequiresExpr` special? Because it should "eat" the errors and only 
return a value?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1374
+  }
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
   return inherited::TransformRequiresExpr(E);

Other uses of `PerformDependentDiagnostics` happen inside 
`LocalInstantiationScope` and after substitution of inner parts.

I suggest following this pattern and moving the added code after the call to 
`inherited::TransformRequiresExpr`.
I do not have any concrete examples where the current approach fails, but it's 
better to have a single mode of operation across all opeartions.



Comment at: 
clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp:157
+static_assert(A<0>::faz());
+}

Could you add a check that in the following case we mention access check in the 
note to the `no matching function to call` error?

```
template  struct Use;

class X { int a; friend struct Use; };

template  struct Use {
  static void foo() requires (requires (X x) { x.a; }) {
  }
};

void test() {
Use::foo();
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 485726.
usaxena95 added a comment.

Improved comment and added comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,54 @@
 constexpr bool b = requires (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 {
+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());
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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,6 +1363,14 @@
 }
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
+  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);
+if (Trap.hasErrorOccurred())
+  return ExprError();
+  }
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
   return inherited::TransformRequiresExpr(E);
 }


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,54 @@
 constexpr bool b = requires (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 {
+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());
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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,6 +1363,14 @@
 }
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
+  if 

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 485725.
usaxena95 added a comment.

Perform access-dependent checks after transforming requires clause.
This is more generic and less invasive than the previous version which changed 
`RebuildMemberExpr` and also produced duplicate diagnostics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,54 @@
 constexpr bool b = requires (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 {
+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());
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ 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"
@@ -1363,7 +1364,19 @@
 
 ExprResult TransformRequiresExpr(RequiresExpr *E) {
   LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
-  return inherited::TransformRequiresExpr(E);
+  ExprResult TransReq = inherited::TransformRequiresExpr(E);
+  {
+if (TransReq.isInvalid() || !E->getBody()->isDependentContext())
+  return TransReq;
+Sema::SFINAETrap Trap(SemaRef);
+// We recreated parameters, but not by instantiating them. There may be
+// pending access-dependent diagnostics to produce.
+// RequiresExpr Body serves as the DeclContext for the parameters.
+SemaRef.PerformDependentDiagnostics(E->getBody(), TemplateArgs);
+if (Trap.hasErrorOccurred())
+  return ExprError();
+  }
+  return TransReq;
 }
 
 bool TransformRequiresExprRequirements(


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp
@@ -104,3 +104,54 @@
 constexpr bool b = requires (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 {
+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());
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you for the fix, can you elaborate on the motivation for this change, it 
looks like setting `setNamingClass` allows for improved diagnostics but I would 
like it explained better in the PR description.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2244
 TemplateInstantiator::TransformExprRequirement(concepts::ExprRequirement *Req) 
{
-  if (!Req->isDependent() && !AlwaysRebuild())
-return Req;

Why this change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140547/new/

https://reviews.llvm.org/D140547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140547: Perform access checking to private members in simple requirement.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 created this revision.
Herald added a project: All.
usaxena95 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140547

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/class.access/p4.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
  clang/test/SemaCXX/access.cpp

Index: clang/test/SemaCXX/access.cpp
===
--- clang/test/SemaCXX/access.cpp
+++ clang/test/SemaCXX/access.cpp
@@ -115,13 +115,13 @@
 namespace N {
 class Y {
   template friend struct X;
-  int t; // expected-note {{here}}
+  int t; // expected-note 2 {{here}}
 };
 }
 template struct X {
-  X() { (void)N::Y().t; } // expected-error {{private}}
+  X() { (void)N::Y().t; } // expected-error 2 {{private}}
 };
-X x;
+X x; // expected-note {{in instantiation of member function}}
   }
   namespace comment2 {
 struct X;
Index: clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
+++ clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
@@ -97,17 +97,17 @@
 friend class User;
 friend bool transform<>(Bool, bool);
 
-bool value; // expected-note 2 {{declared private here}}
+bool value; // expected-note 4 {{declared private here}}
   };
 
   template  class User {
 static T compute(Bool b) {
-  return b.value; // expected-error {{'value' is a private member of 'test3::Bool'}}
+  return b.value; // expected-error 2 {{'value' is a private member of 'test3::Bool'}}
 }
   };
 
   template  T transform(Bool b, T value) {
-if (b.value) // expected-error {{'value' is a private member of 'test3::Bool'}}
+if (b.value) // expected-error 2 {{'value' is a private member of 'test3::Bool'}}
   return value;
 return value + 1;
   }
@@ -222,7 +222,7 @@
   template  A bar(const T*, const A&);
   template  class A {
   private:
-void foo(); // expected-note {{declared private here}}
+void foo(); // expected-note 2 {{declared private here}}
 friend A bar<>(const T*, const A&);
   };
 
@@ -231,7 +231,7 @@
 l1.foo();
 
 A l2;
-l2.foo(); // expected-error {{'foo' is a private member of 'test10::A'}}
+l2.foo(); // expected-error 2 {{'foo' is a private member of 'test10::A'}}
 
 return l1;
   }
Index: clang/test/CXX/drs/dr6xx.cpp
===
--- clang/test/CXX/drs/dr6xx.cpp
+++ clang/test/CXX/drs/dr6xx.cpp
@@ -882,11 +882,17 @@
 friend int dr674::g(int);
 friend int dr674::h<>(int);
 int n; // expected-note 2{{private}}
+#if __cplusplus  >= 201103L
+  // expected-note@-2 {{private}}
+#endif
   };
 
   template int f(T) { return X().n; }
   int g(int) { return X().n; }
   template int g(T) { return X().n; } // expected-error {{private}}
+  #if __cplusplus  >= 201103L
+// expected-error@-2 {{private}}
+  #endif
   int h(int) { return X().n; } // expected-error {{private}}
   template int h(T) { return X().n; }
 
@@ -910,11 +916,17 @@
 friend int Y::g(int);
 friend int Y::h<>(int);
 int n; // expected-note 2{{private}}
+#if __cplusplus >= 201103L
+  // expected-note@-2 {{private}}
+#endif
   };
 
   template int Y::f(T) { return Z().n; }
   int Y::g(int) { return Z().n; }
   template int Y::g(T) { return Z().n; } // expected-error {{private}}
+  #if __cplusplus  >= 201103L
+// expected-error@-2 {{private}}
+  #endif
   int Y::h(int) { return Z().n; } // expected-error {{private}}
   template int Y::h(T) { return Z().n; }
 
Index: clang/test/CXX/class.access/p4.cpp
===
--- clang/test/CXX/class.access/p4.cpp
+++ clang/test/CXX/class.access/p4.cpp
@@ -503,26 +503,26 @@
 namespace test15 {
   template  class A {
   private:
-int private_foo; // expected-note {{declared private here}}
-static int private_sfoo; // expected-note {{declared private here}}
+int private_foo; // expected-note 2 {{declared private here}}
+static int private_sfoo; // expected-note 2 {{declared private here}}
   protected:
-int protected_foo; // expected-note 3 {{declared protected here}} // expected-note {{can only access this member on an object of type 'test15::B'}}
-static int protected_sfoo; // expected-note 3 {{declared protected here}}
+int protected_foo; // expected-note 6 {{declared protected here}} // expected-note 2 {{can only access this member on an object of type 'test15::B'}}
+static int protected_sfoo; // expected-note 6 {{declared protected here}}
 
 int test1(A ) {
-  return a.private_foo; // expected-error {{private member}}
+  return a.private_foo;