Perform access checking to private members in simple requirement.

[usaxena95]

Summary

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.

[shafik]

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.

clang/lib/Sema/SemaTemplateInstantiate.cpp

2243

Why this change?

[usaxena95]

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.

[usaxena95]

Improved comment and added comment.

[ilya-biryukov]

clang/lib/Sema/SemaTemplateInstantiate.cpp

1365

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.

1366

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?

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

157

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 <class T> struct Use;

class X { int a; friend struct Use<short>; };

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

void test() {
    Use<int>::foo();
}

[usaxena95]

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

[usaxena95]

clang/lib/Sema/SemaTemplateInstantiate.cpp

1366

Yes. Precisely.

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

157

Added. This does not produce any diagnostics. Investigating!

[usaxena95]

Added more tests. Still investigating libcxx failure and tests.

[usaxena95]

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.

[usaxena95]

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.

[ilya-biryukov]

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.

clang/lib/Parse/ParseExprCXX.cpp

3512

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

clang/lib/Sema/SemaConcept.cpp

1005

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.

clang/lib/Sema/SemaTemplateInstantiate.cpp

1372

NIT: LLVM uses FIXME more often.

1374

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

1374

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.

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

235

NIT: FIXME

[usaxena95]

Addressed comments.

clang/lib/Sema/SemaTemplateInstantiate.cpp

1374

Thanks for spotting. Adding a test with invalid requirement which caused a crash.

[usaxena95]

Remove new lines.

[ilya-biryukov]

LGTM, thanks for fixing this!

[RKSimon]

clang/docs/ReleaseNotes.rst

739

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

[erichkeane]

clang/docs/ReleaseNotes.rst

739

Looks like he already fixed it in 75c0d43e857dd

[RKSimon]

clang/docs/ReleaseNotes.rst

739

Thanks!

[uabelho]

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?

[ilya-biryukov]

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.

[erichkeane]

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.

clang/lib/Sema/SemaTemplateInstantiate.cpp

1366

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

[usaxena95]

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.

clang/docs/ReleaseNotes.rst

 - Implemented `P0857R0 <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0857r0.html>`_,
   which specifies constrained lambdas and constrained template *template-parameter*\s.
 - Required parameter pack to be provided at the end of the concept parameter list. This
   fixes `Issue 48182 <https://github.com/llvm/llvm-project/issues/48182>`_.
 
 - 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
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Support label at end of compound statement (`P2324 <https://wg21.link/p2324r2>`_).
 - Implemented `P1169R4: static operator() <https://wg21.link/P1169R4>`_ and `P2589R1: static operator[] <https://wg21.link/P2589R1>`_.
 - Implemented "char8_t Compatibility and Portability Fix" (`P2513R3 <https://wg21.link/P2513R3>`_).
   This change was applied to C++20 as a Defect Report.
 - Implemented "Permitting static constexpr variables in constexpr functions" (`P2647R1 <https://wg21.link/P2647R1>_`).

clang/include/clang/AST/ExprConcepts.h

public:
   /// \brief Whether or not the requires clause is satisfied.
   /// The expression must not be dependent.
   bool isSatisfied() const {
     assert(!isValueDependent()
            && "isSatisfied called on a dependent RequiresExpr");
     return RequiresExprBits.IsSatisfied;
   }
 
+  void setSatisfied(bool IsSatisfied) {
+    assert(!isValueDependent() &&
+           "setSatisfied called on a dependent RequiresExpr");
+    RequiresExprBits.IsSatisfied = IsSatisfied;
+  }
+
   SourceLocation getRequiresKWLoc() const {
     return RequiresExprBits.RequiresKWLoc;
   }
 
   SourceLocation getRBraceLoc() const { return RBraceLoc; }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == RequiresExprClass;

clang/lib/Parse/ParseExprCXX.cpp

ExprResult Parser::ParseRequiresExpression() {
   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);
 
   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());
 
   if (Tok.is(tok::r_brace)) {
     // Grammar does not allow an empty body.
     // requirement-body:
     //   { requirement-seq }
     // requirement-seq:
if (Requirements.empty()) {
       // wrote.
       Braces.consumeClose();
       Actions.ActOnFinishRequiresExpr();
       return ExprError();
     }
   }
   Braces.consumeClose();
   Actions.ActOnFinishRequiresExpr();
+  ParsingBodyDecl.complete(Body);
   return Actions.ActOnRequiresExpr(RequiresKWLoc, Body, LocalParameterDecls,
                                    Requirements, Braces.getCloseLocation());
 }
 
 static TypeTrait TypeTraitFromTokKind(tok::TokenKind kind) {
   switch (kind) {
   default: llvm_unreachable("Not a known type trait");
 #define TYPE_TRAIT_1(Spelling, Name, Key) \

clang/lib/Sema/SemaAccess.cpp

void Sema::HandleDelayedAccessCheck(DelayedDiagnostic &DD, Decl *D) {
   DeclContext *DC = D->getDeclContext();
   if (D->isLocalExternDecl()) {
     DC = D->getLexicalDeclContext();
   } else if (FunctionDecl *FN = dyn_cast<FunctionDecl>(D)) {
     DC = FN;
   } 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);
 
   AccessTarget Target(DD.getAccessData());
 
   if (CheckEffectiveAccess(*this, EC, DD.Loc, Target) == ::AR_inaccessible)
     DD.Triggered = true;

clang/lib/Sema/SemaConcept.cpp

if (BinaryOperator *BO = dyn_cast<BinaryOperator>(SubstExpr)) {
     } else {
       S.Diag(SubstExpr->getSourceRange().getBegin(),
              diag::note_concept_specialization_constraint_evaluated_to_false)
           << (int)First << CSE;
     }
     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))
           diagnoseUnsatisfiedRequirement(S, E, First);
         else if (auto *T = dyn_cast<concepts::TypeRequirement>(Req))
           diagnoseUnsatisfiedRequirement(S, T, First);
         else
           diagnoseUnsatisfiedRequirement(

clang/lib/Sema/SemaTemplateInstantiate.cpp

 //===----------------------------------------------------------------------===/
 
 #include "TreeTransform.h"
 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTConsumer.h"
 #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"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/Stack.h"
ExprResult TransformLambdaExpr(LambdaExpr *E) {
             PVD->setDefaultArg(ErrorResult.get());
         }
       }
 
       return Result;
     }
 
     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(
         ArrayRef<concepts::Requirement *> Reqs,
         SmallVectorImpl<concepts::Requirement *> &Transformed) {
       bool SatisfactionDetermined = false;
       for (concepts::Requirement *Req : Reqs) {
         concepts::Requirement *TransReq = nullptr;
return RebuildTypeRequirement(createSubstDiag(SemaRef, Info,
         [&] (llvm::raw_ostream& OS) {
             Req->getType()->getType().print(OS, SemaRef.getPrintingPolicy());
         }));
   return RebuildTypeRequirement(TransType);
 }
 
 concepts::ExprRequirement *
 TemplateInstantiator::TransformExprRequirement(concepts::ExprRequirement *Req) {
   if (!Req->isDependent() && !AlwaysRebuild())
     return Req;
 
   Sema::SFINAETrap Trap(SemaRef);
 
   llvm::PointerUnion<Expr *, concepts::Requirement::SubstitutionDiagnostic *>
       TransExpr;
   if (Req->isExprSubstitutionFailure())
     TransExpr = Req->getExprSubstitutionDiagnostic();

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

 }
 
 // typeid() of an expression becomes potentially evaluated if the expression is
 // of a polymorphic type.
 class X { virtual ~X(); };
 constexpr bool b = requires (X &x) { static_cast<int(*)[(typeid(x), 0)]>(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<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

clang/test/SemaCXX/invalid-requirement-requires-expr.cpp

+// 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@-1{{recursive template instantiation exceeded maximum depth}}
+    };
+}
+static_assert(A<1>::far());
+static_assert(!A<10001>::far()); // expected-note {{in instantiation of member function}}