Make evaluation of nested requirement consistent with requires expr.

[usaxena95]

Summary

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

template<class T>  concept True = true;

template <class T>
concept C1 = requires (T) {
   requires True<typename T::value> || True<T>;
};

template <class T>
constexpr bool foo()
requires True<typename T::value> || True<T> {
    return true;
}
static_assert(C1<double>); // Previously failed due to SFINAE error
static_assert(foo<int>()); // but this works fine.

The issue here is the discrepancy between how a nested requirement is evaluated Vs how a non-nested requirement is evaluated.

This patch makes constraint checking consistent for nested requirement
and trailing requires expressions by using the same evaluator.

[erichkeane]

clang/include/clang/AST/ExprConcepts.h

428

I'm a little concerned that we're changing to a situation where the constraint in a nested-requirement can be null like this. The other cases it can be null we consider it a 'substitution failure' (see the 1st constructor).

It seems to me that we perhaps need a different state here for that.

clang/test/SemaTemplate/instantiate-requires-expr.cpp

27

This is wrong here, we shouldn't be making our diagnostics incorrect like this.

[usaxena95]

Added tests.
Fixed regressions in diagnositcs.

[erichkeane]

clang/include/clang/AST/ExprConcepts.h

428

I still have this concern, we need to make sure that NestedRequirement is written to consider this type of failure, it likely needs a separate constructor that contains information about the failure. We probably want at least some sort of stored diagnostic here, sot hat we dont lose the output we have below.

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

23

Losing the extra context here is unfortunate as well... why do we lose this info?

94

Can you re-arrange this test with the #NAME type bookmarks so that the instantiation notes/etc are all arranged with the error they are associated with?

[usaxena95]

Addressed comments.

clang/include/clang/AST/ExprConcepts.h

428

NestedRequirement will no more capture a substitution failure. I am planning to remove all references to SubstitutionDiagnostics for it. I have temporarily added unreachable tags to the references of SubstitutionDiagnostics.

If this looks fine to you then I would move forward with refactoring it to remove all support for SubstitutionDiagnostics in NestedRequirement. WDYT ?

clang/lib/Sema/SemaConcept.cpp

172

For context: This is not necessarily invalid but more importantly the BinaryOp cannot be constant evaluated as it would have dependent RHS.

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

23

This is now restored.

[usaxena95]

Removed extraneous colons from diagnostic.

[usaxena95]

Remove unintended whitespaces and new lines.

[erichkeane]

Happy now, thanks!

clang/include/clang/AST/ExprConcepts.h

428

Ok, that works for me.

[usaxena95]

Removed use of SubstitutionDiagnostic from NestedRequirement.

[rtrieu]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2364

I have found a crash here when it access vector Result without checking the size first, leading to out of bounds memory access. CReduce gave the following testcase:

template <class a, a> struct b;
template <bool c> using d = b<bool, c>;
template <class a, class e> using f = d<__is_same(a, e)>;
template <class a, class e>
concept g = f<a, e>::h;
template <class a, class e>
concept i = g<e, a>;
template <typename> class j {
  template <typename k>
  requires requires { requires i<j, k>; }
  j();
};
template <> j();

clang reduce.ii --std=c++20

assertion failed at llvm/include/llvm/ADT/SmallVector.h:294 in reference llvm::SmallVectorTemplateCommon<clang::Expr *>::operator[](size_type) [T = clang::Expr *]: idx < size()
...
...
(anonymous namespace)::TemplateInstantiator::TransformNestedRequirement(clang::concepts::NestedRequirement*) clang/lib/Sema/SemaTemplateInstantiate.cpp:2339:25
...
...

[usaxena95]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2364

Thanks for noticing and sorry for the trouble. I have fixed this forward in https://github.com/llvm/llvm-project/commit/8c0aa53b07caa604d58a0d83dc571d8fcb004972.

[shafik]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2364

Can you add a test case as well so if we regress we will catch it, thank you!

clang/include/clang/AST/ExprConcepts.h

NestedRequirement(Expr *Constraint) :
                   Constraint->containsUnexpandedParameterPack()),
       Value(Constraint) {
     assert(Constraint->isInstantiationDependent() &&
            "Nested requirement with non-dependent constraint must be "
            "constructed with a ConstraintSatisfaction object");
   }
 
   NestedRequirement(ASTContext &C, Expr *Constraint,
-                    const ConstraintSatisfaction &Satisfaction) :
-      Requirement(RK_Nested, Constraint->isInstantiationDependent(),
-                  Constraint->containsUnexpandedParameterPack(),
+                    const ConstraintSatisfaction &Satisfaction)
+      : Requirement(RK_Nested,
+                    Constraint && Constraint->isInstantiationDependent(),
+                    Constraint && Constraint->containsUnexpandedParameterPack(),
                     Satisfaction.IsSatisfied),
         Value(Constraint),
         Satisfaction(ASTConstraintSatisfaction::Create(C, Satisfaction)) {}
 
   bool isSubstitutionFailure() const {
     return Value.is<SubstitutionDiagnostic *>();
   }
 
   SubstitutionDiagnostic *getSubstitutionDiagnostic() const {
     assert(isSubstitutionFailure() &&
            "getSubstitutionDiagnostic() may not be called when there was no "

clang/lib/Sema/SemaConcept.cpp

calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
   if (LogicalBinOp BO = ConstraintExpr) {
     ExprResult LHSRes = calculateConstraintSatisfaction(
         S, BO.getLHS(), Satisfaction, Evaluator);
 
     if (LHSRes.isInvalid())
       return ExprError();
 
     bool IsLHSSatisfied = Satisfaction.IsSatisfied;
     
     if (BO.isOr() && IsLHSSatisfied)
       // [temp.constr.op] p3
       //    A disjunction is a constraint taking two operands. To determine if
       //    a disjunction is satisfied, the satisfaction of the first operand
       //    is checked. If that is satisfied, the disjunction is satisfied.
       //    Otherwise, the disjunction is satisfied if and only if the second
       //    operand is satisfied.
       return BO.recreateBinOp(S, LHSRes);
 
     if (BO.isAnd() && !IsLHSSatisfied)
       // [temp.constr.op] p2
       //    A conjunction is a constraint taking two operands. To determine if
       //    a conjunction is satisfied, the satisfaction of the first operand
       //    is checked. If that is not satisfied, the conjunction is not
       //    satisfied. Otherwise, the conjunction is satisfied if and only if
       //    the second operand is satisfied.
return calculateConstraintSatisfaction(
           // We do not want error diagnostics escaping here.
           Sema::SFINAETrap Trap(S);
           SubstitutedExpression =
               S.SubstConstraintExpr(const_cast<Expr *>(AtomicExpr), MLTAL);
           // Substitution might have stripped off a contextual conversion to
           // bool if this is the operand of an '&&' or '||'. For example, we
           // might lose an lvalue-to-rvalue conversion here. If so, put it back
           // before we try to evaluate.
-          if (!SubstitutedExpression.isInvalid())
+          if (SubstitutedExpression.isUsable() &&
+              !SubstitutedExpression.isInvalid())
             SubstitutedExpression =
                 S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
           if (SubstitutedExpression.isInvalid() || Trap.hasErrorOccurred()) {
             // C++2a [temp.constr.atomic]p1
             //   ...If substitution results in an invalid type or expression, the
             //   constraint is not satisfied.
             if (!Trap.hasErrorOccurred())
               // A non-SFINAE error has occurred as a result of this
return calculateConstraintSatisfaction(
             DiagString = ": ";
             SubstDiag.second.EmitToString(S.getDiagnostics(), DiagString);
             unsigned MessageSize = DiagString.size();
             char *Mem = new (S.Context) char[MessageSize];
             memcpy(Mem, DiagString.c_str(), MessageSize);
             Satisfaction.Details.emplace_back(
                 AtomicExpr,
                 new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic{
-                        SubstDiag.first, StringRef(Mem, MessageSize)});
             Satisfaction.IsSatisfied = false;
             return ExprEmpty();
           }
         }
 
         if (!S.CheckConstraintExpression(SubstitutedExpression.get()))
           return ExprError();
 

clang/lib/Sema/SemaTemplateInstantiate.cpp

TemplateDeductionInfo Info(Req->getConstraintExpr()->getBeginLoc());
     EnterExpressionEvaluationContext ContextRAII(
         SemaRef, Sema::ExpressionEvaluationContext::ConstantEvaluated);
     Sema::SFINAETrap Trap(SemaRef);
     Sema::InstantiatingTemplate ConstrInst(SemaRef,
         Req->getConstraintExpr()->getBeginLoc(), Req, Info,
         Req->getConstraintExpr()->getSourceRange());
     if (ConstrInst.isInvalid())
       return nullptr;
-    TransConstraint = TransformExpr(Req->getConstraintExpr());
-    if (!TransConstraint.isInvalid()) {
+    llvm::SmallVector<Expr *> Result;
+    if (SemaRef.CheckConstraintSatisfaction(
+            nullptr, {Req->getConstraintExpr()}, Result, TemplateArgs,
+            Req->getConstraintExpr()->getSourceRange(), Satisfaction))
+      TransConstraint = Result[0];
+    if (TransConstraint.isUsable() && !TransConstraint.isInvalid()) {
       bool CheckSucceeded =
           SemaRef.CheckConstraintExpression(TransConstraint.get());
       (void)CheckSucceeded;
       assert((CheckSucceeded || Trap.hasErrorOccurred()) &&
                                    "CheckConstraintExpression failed, but "
                                    "did not produce a SFINAE error");
     }
     // Use version of CheckConstraintSatisfaction that does no substitutions.
-    if (!TransConstraint.isInvalid() &&
+    if (TransConstraint.isUsable() && !TransConstraint.isInvalid() &&
         !TransConstraint.get()->isInstantiationDependent() &&
         !Trap.hasErrorOccurred()) {
       bool CheckFailed = SemaRef.CheckConstraintSatisfaction(
           TransConstraint.get(), Satisfaction);
       (void)CheckFailed;
       assert((!CheckFailed || Trap.hasErrorOccurred()) &&
                                  "CheckConstraintSatisfaction failed, "
                                  "but did not produce a SFINAE error");
     }
     if (TransConstraint.isInvalid() || Trap.hasErrorOccurred())
       return RebuildNestedRequirement(createSubstDiag(SemaRef, Info,
           [&] (llvm::raw_ostream& OS) {
               Req->getConstraintExpr()->printPretty(OS, nullptr,
                                                     SemaRef.getPrintingPolicy());
           }));
   }
-  if (TransConstraint.get()->isInstantiationDependent())
+  if (TransConstraint.isUsable() &&
+      TransConstraint.get()->isInstantiationDependent())
     return new (SemaRef.Context)
         concepts::NestedRequirement(TransConstraint.get());
   return new (SemaRef.Context) concepts::NestedRequirement(
       SemaRef.Context, TransConstraint.get(), Satisfaction);
 }
 
 
 /// Perform substitution on the type T with a given set of template
 /// arguments.

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

 
 template<typename T> requires requires (T t) { requires sizeof(t) == 0; } // expected-note{{because 'sizeof (t) == 0' (4 == 0) evaluated to false}}
 struct r3 {};
 
 using r3i = r3<int>; // expected-error{{constraints not satisfied for class template 'r3' [with T = int]}}
 
 template<typename T>
 struct X {
-    template<typename U> requires requires (U u) { requires sizeof(u) == sizeof(T); } // expected-note{{because 'sizeof (u) == sizeof(T)' would be invalid: invalid application of 'sizeof' to an incomplete type 'void'}}
+    template<typename U> requires requires (U u) { requires sizeof(u) == sizeof(T); } 
+    // expected-note@-1 {{because substituted constraint expression is ill-formed: invalid application of 'sizeof' to an incomplete type 'void'}}
     struct r4 {};
 };
 
 using r4i = X<void>::r4<int>; // expected-error{{constraints not satisfied for class template 'r4' [with U = int]}}
 
 // C++ [expr.prim.req.nested] Examples
 namespace std_example {
   template<typename U> concept C1 = sizeof(U) == 1; // expected-note{{because 'sizeof(decltype(+t)) == 1' (4 == 1) evaluated to false}}
   template<typename T> concept D =
     requires (T t) {
       requires C1<decltype (+t)>; // expected-note{{because 'decltype(+t)' (aka 'int') does not satisfy 'C1'}}
   };
 
   struct T1 { char operator+() { return 'a'; } };
   static_assert(D<T1>);
   template<D T> struct D_check {}; // expected-note{{because 'short' does not satisfy 'D'}}
   using dc1 = D_check<short>; // expected-error{{constraints not satisfied for class template 'D_check' [with T = short]}}
 
   template<typename T>
   concept C2 = requires (T a) {
       requires sizeof(a) == 4; // OK
-      requires a == 0; // expected-note{{because 'a == 0' would be invalid: constraint variable 'a' cannot be used in an evaluated context}}
+      requires a == 0; // expected-note{{because substituted constraint expression is ill-formed: constraint variable 'a' cannot be used in an evaluated context}}
     };
   static_assert(C2<int>); // expected-note{{because 'int' does not satisfy 'C2'}} expected-error{{static assertion failed}}
 }
 
 template<typename T>
 concept K = requires (T::Type X) {
   X.next();
 };
 
+namespace SubstitutionFailureNestedRequires {
+template<class T>  concept True = true;
+
+struct S { double value; };
+
+template <class T>
+concept Pipes = requires (T x) {
+   requires True<decltype(x.value)> || True<T>;
+   requires True<T> || True<decltype(x.value)>;
+};
+
+template <class T>
+concept Amps1 = requires (T x) {
+   requires True<decltype(x.value)> && True<T>;
+   // expected-note@-1{{because substituted constraint expression is ill-formed: member reference base type 'int' is not a structure or union}}
+};
+template <class T>
+concept Amps2 = requires (T x) {
+   requires True<T> && True<decltype(x.value)>;
+};
+
+static_assert(Pipes<S>);
+static_assert(Pipes<double>);
+
+static_assert(Amps1<S>);
+static_assert(!Amps1<double>);
+
+static_assert(Amps2<S>);
+static_assert(!Amps2<double>);
+
+template<class T>
+void foo1() requires requires (T x) { // expected-note {{candidate template ignored: constraints not satisfied [with T = int]}}
+  requires
+  True<decltype(x.value)> // expected-note {{because substituted constraint expression is ill-formed: member reference base type 'int' is not a structure or union}}
+  && True<T>;
+} {}
+template<class T> void fooPipes() requires Pipes<T> {}
+template<class T> void fooAmps1() requires Amps1<T> {}
+// expected-note@-1 {{candidate template ignored: constraints not satisfied [with T = int]}} \
+// expected-note@-1 {{because 'int' does not satisfy 'Amps1'}}
+
+void foo() {
+  foo1<S>();
+  foo1<int>(); // expected-error {{no matching function for call to 'foo1'}}
+  fooPipes<S>();
+  fooPipes<int>();
+  fooAmps1<S>();
+  fooAmps1<int>(); // expected-error {{no matching function for call to 'fooAmps1'}}
+}
+
+template<class T>
+concept HasNoValue = requires (T x) {
+  requires !True<decltype(x.value)> && True<T>;
+};
+// FIXME: 'int' does not satisfy 'HasNoValue' currently since `!True<decltype(x.value)>` is an invalid expression.
+// But, in principle, it should be constant-evaluated to true.
+static_assert(!HasNoValue<int>);
+static_assert(!HasNoValue<S>);
+
+template<class T> constexpr bool NotAConceptTrue = true;
+template <class T>
+concept SFinNestedRequires = requires (T x) {
+    // SF in a non-concept specialisation should also be evaluated to false.
+   requires NotAConceptTrue<decltype(x.value)> || NotAConceptTrue<T>;
+};
+static_assert(SFinNestedRequires<int>);
+static_assert(SFinNestedRequires<S>);
+template <class T>
+void foo() requires SFinNestedRequires<T> {} 
+void bar() { 
+  foo<int>();
+  foo<S>();
+}
+namespace SFINAEWithoutConcept {
+  //Add more TODO(usx).
+template<typename T> struct X { static constexpr bool value = T::value; };
+struct True { static constexpr bool value = true; };
+template<typename T> concept C = true;
+
+template<typename T> requires requires(T) { requires C<T> || X<T>::value; } void foo();
+
+void func() {
+  foo<True>();
+  foo<int>(); 
+}
+}
+}
 No newline at end of file

clang/test/SemaTemplate/instantiate-requires-expr.cpp

 
 template<typename... Ts>
 using void_t = void;
 
 // Check that requires parameters are instantiated correctly.
 
 template<typename T> requires
 false_v<requires (T t) { requires is_same_v<decltype(t), int>; }>
-// expected-note@-1 {{because 'false_v<requires (int t) { requires is_same_v<decltype(t), int>; }>' evaluated to false}}
-// expected-note@-2 {{because 'false_v<requires (char t) { requires is_same_v<decltype(t), int>; }>' evaluated to false}}
+// expected-note@-1 {{because 'false_v<requires (int t) { requires <null expr>; }>' evaluated to false}}
+// expected-note@-2 {{because 'false_v<requires (char t) { requires <null expr>; }>' evaluated to false}}
 struct r1 {};
 
 using r1i1 = r1<int>; // expected-error {{constraints not satisfied for class template 'r1' [with T = int]}}
 using r1i2 = r1<char>; // expected-error {{constraints not satisfied for class template 'r1' [with T = char]}}
 
 // Check that parameter packs work.
 
 template<typename... Ts> requires
 false_v<requires (Ts... ts) {requires ((sizeof(ts) == 2) && ...);}>
-// expected-note@-1 {{because 'false_v<requires (short ts, unsigned short ts) { requires (sizeof (ts) == 2) && (sizeof (ts) == 2); }>'}}
-// expected-note@-2 {{because 'false_v<requires (short ts) { requires (sizeof (ts) == 2); }>' evaluated to false}}
+// expected-note@-1 {{because 'false_v<requires (short ts, unsigned short ts) { requires <null expr>; }>' evaluated to false}}
+// expected-note@-2 {{because 'false_v<requires (short ts) { requires <null expr>; }>' evaluated to false}}
 struct r2 {};
 
 using r2i1 = r2<short, unsigned short>; // expected-error {{constraints not satisfied for class template 'r2' [with Ts = <short, unsigned short>]}}
 using r2i2 = r2<short>; // expected-error {{constraints not satisfied for class template 'r2' [with Ts = <short>]}}
 
 template<typename... Ts> requires
 false_v<(requires (Ts ts) {requires sizeof(ts) != 0;} && ...)>
-// expected-note@-1 {{because 'false_v<requires (short ts) { requires sizeof (ts) != 0; } && requires (unsigned short ts) { requires sizeof (ts) != 0; }>' evaluated to false}}
-// expected-note@-2 {{because 'false_v<requires (short ts) { requires sizeof (ts) != 0; }>' evaluated to false}}
+// expected-note@-1 {{because 'false_v<requires (short ts) { requires <null expr>; } && requires (unsigned short ts) { requires <null expr>; }>' evaluated to false}}
+// expected-note@-2 {{because 'false_v<requires (short ts) { requires <null expr>; }>' evaluated to false}}
 struct r3 {};
 
 using r3i1 = r3<short, unsigned short>; // expected-error {{constraints not satisfied for class template 'r3' [with Ts = <short, unsigned short>]}}
 using r3i2 = r3<short>; // expected-error {{constraints not satisfied for class template 'r3' [with Ts = <short>]}}
 
 template<typename T>
 struct identity { using type = T; };
 
struct r4 {
 
       constexpr void invalid() requires false { }
   };
   static_assert(r4<int>{}.foo() == 0);
 }
 
 namespace nested_requirement {
   // check that constraint expression is instantiated correctly
-  template<typename T> requires false_v<requires { requires sizeof(T) == 2; }> // expected-note{{because 'false_v<requires { requires sizeof(int) == 2; }>' evaluated to false}}
+  template<typename T> requires false_v<requires { requires sizeof(T) == 2; }> // expected-note{{because 'false_v<requires { requires <null expr>; }>' evaluated to false}}
   struct r1 {};
 
   using r1i = r1<int>; // expected-error{{constraints not satisfied for class template 'r1' [with T = int]}}
 
   // substitution error occurs in expr, then expr is instantiated again.
   template<typename T>
   struct a {
       template<typename U> requires
-      (requires { requires sizeof(T::a) == 0; }, false) // expected-note{{because 'requires { requires <<error-expression>>; } , false' evaluated to false}}
+      (requires { requires sizeof(T::a) == 0; }, false) // expected-note{{because 'requires { requires <null expr>; } , false' evaluated to false}}
       struct r {};
   };
 
   using ari = a<int>::r<short>; // expected-error{{constraints not satisfied for class template 'r' [with U = short]}}
 
   // Parameter pack inside expr
   template<typename... Ts> requires
   false_v<(requires { requires sizeof(Ts) == 0; } && ...)>
-  // expected-note@-1 {{because 'false_v<requires { requires sizeof(int) == 0; } && requires { requires sizeof(short) == 0; }>' evaluated to false}}
+  // expected-note@-1 {{because 'false_v<requires { requires <null expr>; } && requires { requires <null expr>; }>' evaluated to false}}
   struct r2 {};
 
   using r2i = r2<int, short>; // expected-error{{constraints not satisfied for class template 'r2' [with Ts = <int, short>]}}
 }
 
 // Parameter pack inside multiple requirements
 template<typename... Ts> requires
 false_v<(requires { requires sizeof(Ts) == 0; sizeof(Ts); } && ...)>
-// expected-note@-1 {{because 'false_v<requires { requires sizeof(int) == 0; sizeof(Ts); } && requires { requires sizeof(short) == 0; sizeof(Ts); }>' evaluated to false}}
+// expected-note@-1 {{because 'false_v<requires { requires <null expr>; sizeof(Ts); } && requires { requires <null expr>; sizeof(Ts); }>' evaluated to false}}
 struct r4 {};
 
 using r4i = r4<int, short>; // expected-error{{constraints not satisfied for class template 'r4' [with Ts = <int, short>]}}
 
 template<typename... Ts> requires
 false_v<(requires(Ts t) { requires sizeof(t) == 0; t++; } && ...)>
-// expected-note@-1 {{because 'false_v<requires (int t) { requires sizeof (t) == 0; t++; } && requires (short t) { requires sizeof (t) == 0; t++; }>' evaluated to false}}
+// expected-note@-1 {{because 'false_v<requires (int t) { requires <null expr>; t++; } && requires (short t) { requires <null expr>; t++; }>' evaluated to false}}
 struct r5 {};
 
 using r5i = r5<int, short>; // expected-error{{constraints not satisfied for class template 'r5' [with Ts = <int, short>]}}
 
 template<typename T> requires
 false_v<(requires(T t) { T{t}; })> // T{t} creates an "UnevaluatedList" context.
 // expected-note@-1 {{because 'false_v<(requires (int t) { int{t}; })>' evaluated to false}}
 struct r6 {};