[clang][C++20] Non-dependent access checks in requires expression.

[usaxena95]

Summary

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.

Fixes: https://github.com/llvm/llvm-project/issues/53334

[usaxena95]

More tests.

[ilya-biryukov]

Should access checks should happen in the context where concept is written or where it's used? Is there a standard wording around it?
If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted:

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 program is ill-formed and we show the diagnostic even though it's not required.

I poked around and found an interesting case where GCC seems to be doing the wrong thing:

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); // GCC picks 1, MSVC picks 1.
static_assert(B<int>::index() == 2);   // GCC (incorrectly?) picks 1, MSVC picks 2!

Is this related to this change? Could we add a test that validates Clang is doing the right thing?

clang/lib/Parse/ParseExprCXX.cpp

3510

Could you explain how this changes behaviour and how it leads to fixing the issue?

I'm sure there is quite a bit of thought and debugging behind this one-line change, but it's not evident by just looking at it how it solves the issue.

[royjacobson]

In D140876#4023286, @ilya-biryukov wrote:

Should access checks should happen in the context where concept is written or where it's used? Is there a standard wording around it?

Unfortunately, only a standard wording issue :(
https://cplusplus.github.io/CWG/issues/2589.html

We're also allowed to cache atomic constraints regardless of the their (unspecified) scope, which makes this a bit more complicated.

There's some discussion about this in https://github.com/llvm/llvm-project/issues/58672

[usaxena95]

In D140876#4023286, @ilya-biryukov wrote:

Should access checks should happen in the context where concept is written or where it's used?
If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted:

As Roy indicated, this is a wording issue. For the time being, concepts are not evaluated in the scope of its use (as the result of atomic constraints should be same at all points of program). An independent requires clause on the other hand considers the scope where it is written.

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 program is ill-formed and we show the diagnostic even though it's not required.

Are you suggesting to drop the diagnostic in this case ? I think

I poked around and found an interesting case where GCC seems to be doing the wrong thing:

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); // GCC picks 1, MSVC picks 1.
static_assert(B<int>::index() == 2);   // GCC (incorrectly?) picks 1, MSVC picks 2!

Is this related to this change? Could we add a test that validates Clang is doing the right thing?

This is an interesting test case. Clang does the right thing here and it is related to both this change and the base change https://reviews.llvm.org/D140547.
The test case mentioned on that patch also needs both the changes and also something else.

I will squash both these patches into one for a better review/commit.

clang/lib/Parse/ParseExprCXX.cpp

3510

Sorry for not providing more context here. I agree this makes things work quite unexpectedly.

The main issue here is the delaying of the diagnostics. Eg in Parser::ParseTemplateDeclarationOrSpecialization through ParsingDeclRAIIObject ParsingTemplateParams. Diagnostics in ParseConceptDefinition are attached to Diagnostics pool of ParsingTemplateParams which are never flushed. Creating a new context does not delay the diagnostics.

[usaxena95]

Moving the changes to https://reviews.llvm.org/D140547

clang/lib/Parse/ParseExprCXX.cpp

ExprResult Parser::ParseRequiresExpression() {
 
   // Start of requirement list
   llvm::SmallVector<concepts::Requirement *, 2> Requirements;
 
   // C++2a [expr.prim.req]p2
   //   Expressions appearing within a requirement-body are unevaluated operands.
   EnterExpressionEvaluationContext Ctx(
       Actions, Sema::ExpressionEvaluationContext::Unevaluated);
+  Sema::ContextRAII SaveContext(Actions, Actions.CurContext);
 
   ParseScope BodyScope(this, Scope::DeclScope);
   RequiresExprBodyDecl *Body = Actions.ActOnStartRequiresExpr(
       RequiresKWLoc, LocalParameterDecls, getCurScope());
 
   if (Tok.is(tok::r_brace)) {
     // Grammar does not allow an empty body.
     // requirement-body:

clang/test/CXX/expr/expr.prim/expr.prim.req/simple-requirement.cpp

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 non_dependent_access_check_hard_errors {
+// 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>());
+}