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

425

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 ↗

(On Diff #478539)

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

425

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?

93

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

425

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

183–184

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

425

Ok, that works for me.

[usaxena95]

Removed use of SubstitutionDiagnostic from NestedRequirement.

[rtrieu]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2339

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

2339

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

2339

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

clang/include/clang/AST/ASTConcept.h

public:
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &C) {
     Profile(ID, C, ConstraintOwner, TemplateArgs);
   }
 
   static void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &C,
                       const NamedDecl *ConstraintOwner,
                       ArrayRef<TemplateArgument> TemplateArgs);
+
+  bool HasSubstitutionFailure() {
+    for (const auto &Detail : Details)
+      if (Detail.second.dyn_cast<SubstitutionDiagnostic *>())
+        return true;
+    return false;
+  }
 };
 
 /// Pairs of unsatisfied atomic constraint expressions along with the
 /// substituted constraint expr, if the template arguments could be
 /// substituted into them, or a diagnostic if substitution resulted in
 /// an invalid expression.
 using UnsatisfiedConstraintRecord =
     std::pair<const Expr *,
struct ASTConstraintSatisfaction final :
   }
 
   const UnsatisfiedConstraintRecord *end() const {
     return getTrailingObjects<UnsatisfiedConstraintRecord>() + NumRecords;
   }
 
   ASTConstraintSatisfaction(const ASTContext &C,
                             const ConstraintSatisfaction &Satisfaction);
+  ASTConstraintSatisfaction(const ASTContext &C,
+                            const ASTConstraintSatisfaction &Satisfaction);
 
   static ASTConstraintSatisfaction *
   Create(const ASTContext &C, const ConstraintSatisfaction &Satisfaction);
+  static ASTConstraintSatisfaction *
+  Rebuild(const ASTContext &C, const ASTConstraintSatisfaction &Satisfaction);
 };
 
 /// \brief Common data class for constructs that reference concepts with
 /// template arguments.
 class ConceptReference {
 protected:
   // \brief The optional nested name specifier used when naming the concept.
   NestedNameSpecifierLoc NestedNameSpec;

clang/include/clang/AST/ASTNodeTraverser.h

getNodeDelegate().AddChild([=] {
       } else if (auto *ER = dyn_cast<concepts::ExprRequirement>(R)) {
         if (!ER->isExprSubstitutionFailure())
           Visit(ER->getExpr());
         if (!ER->getReturnTypeRequirement().isEmpty())
           Visit(ER->getReturnTypeRequirement()
                     .getTypeConstraint()
                     ->getImmediatelyDeclaredConstraint());
       } else if (auto *NR = dyn_cast<concepts::NestedRequirement>(R)) {
-        if (!NR->isSubstitutionFailure())
+        if (!NR->hasInvalidConstraint())
           Visit(NR->getConstraintExpr());
       }
     });
   }
 
   void Visit(const APValue &Value, QualType Ty) {
     getNodeDelegate().AddChild([=] { getNodeDelegate().Visit(Value, Ty); });
   }

clang/include/clang/AST/ExprConcepts.h

 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TrailingObjects.h"
 #include <utility>
 #include <string>
 
 namespace clang {
 class ASTStmtReader;
 class ASTStmtWriter;
 
public:
   static bool classof(const Requirement *R) {
     return R->getKind() == RK_Compound || R->getKind() == RK_Simple;
   }
 };
 
 /// \brief A requires-expression requirement which is satisfied when a general
 /// constraint expression is satisfied ('nested' requirements).
 class NestedRequirement : public Requirement {
-  llvm::PointerUnion<Expr *, SubstitutionDiagnostic *> Value;
+  Expr *Constraint = nullptr;
   const ASTConstraintSatisfaction *Satisfaction = nullptr;
+  bool HasInvalidConstraint = false;
+  StringRef InvalidConstraintEntity;
 
 public:
   friend ASTStmtReader;
   friend ASTStmtWriter;
 
-  NestedRequirement(SubstitutionDiagnostic *SubstDiag) :
-      Requirement(RK_Nested, /*IsDependent=*/false,
-                  /*ContainsUnexpandedParameterPack*/false,
-                  /*IsSatisfied=*/false), Value(SubstDiag) {}
-
-  NestedRequirement(Expr *Constraint) :
-      Requirement(RK_Nested, /*IsDependent=*/true,
+  NestedRequirement(Expr *Constraint)
+      : Requirement(RK_Nested, /*IsDependent=*/true,
                     Constraint->containsUnexpandedParameterPack()),
-      Value(Constraint) {
+        Constraint(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(),
+                    const ConstraintSatisfaction &Satisfaction)
+      : Requirement(RK_Nested, Constraint->isInstantiationDependent(),
                     Constraint->containsUnexpandedParameterPack(),
                     Satisfaction.IsSatisfied),
-      Value(Constraint),
+        Constraint(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 "
-           "substitution failure.");
-    return Value.get<SubstitutionDiagnostic *>();
+  NestedRequirement(StringRef InvalidConstraintEntity,
+                    const ASTConstraintSatisfaction *Satisfaction)
+      : Requirement(RK_Nested,
+                    /*IsDependent=*/false,
+                    /*ContainsUnexpandedParameterPack*/ false,
+                    Satisfaction->IsSatisfied),
+        Satisfaction(Satisfaction), HasInvalidConstraint(true),
+        InvalidConstraintEntity(InvalidConstraintEntity) {}
+
+  NestedRequirement(ASTContext &C, StringRef InvalidConstraintEntity,
+                    const ConstraintSatisfaction &Satisfaction)
+      : NestedRequirement(InvalidConstraintEntity,
+                          ASTConstraintSatisfaction::Create(C, Satisfaction)) {}
+
+  bool hasInvalidConstraint() const { return HasInvalidConstraint; }
+
+  StringRef getInvalidConstraintEntity() {
+    assert(hasInvalidConstraint());
+    return InvalidConstraintEntity;
   }
 
   Expr *getConstraintExpr() const {
-    assert(!isSubstitutionFailure() && "getConstraintExpr() may not be called "
-                                       "on nested requirements with "
-                                       "substitution failures.");
-    return Value.get<Expr *>();
+    assert(!hasInvalidConstraint() &&
+           "getConstraintExpr() may not be called "
+           "on nested requirements with invalid constraint.");
+    return Constraint;
   }
 
   const ASTConstraintSatisfaction &getConstraintSatisfaction() const {
-    assert(!isSubstitutionFailure() && "getConstraintSatisfaction() may not be "
-                                       "called on nested requirements with "
-                                       "substitution failures.");
     return *Satisfaction;
   }
 
   static bool classof(const Requirement *R) {
     return R->getKind() == RK_Nested;
   }
 };
 

clang/include/clang/AST/RecursiveASTVisitor.h

if (RetReq.isTypeConstraint()) {
     }
   }
   return true;
 }
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseConceptNestedRequirement(
     concepts::NestedRequirement *R) {
-  if (!R->isSubstitutionFailure())
+  if (!R->hasInvalidConstraint())
     return getDerived().TraverseStmt(R->getConstraintExpr());
   return true;
 }
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::PostVisitStmt(Stmt *S) {
   // In pre-order traversal mode, each Traverse##STMT method is responsible for
   // calling WalkUpFrom. Therefore, if the user overrides Traverse##STMT and

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_expr_requirement_constraints_not_satisfied : Note<
   "%select{and|because}0 type constraint '%1' was not satisfied:">;
 def note_expr_requirement_constraints_not_satisfied_simple : Note<
   "%select{and|because}0 %1 does not satisfy %2:">;
 def note_type_requirement_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid: %2">;
 def note_type_requirement_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;
 def note_nested_requirement_substitution_error : Note<
-  "%select{and|because}0 '%1' would be invalid: %2">;
+  "%select{and|because}0 '%1' would be invalid%2">;
 def note_nested_requirement_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;
 def note_ambiguous_atomic_constraints : Note<
   "similar constraint expressions not considered equivalent; constraint "
   "expressions cannot be considered equivalent unless they originate from the "
   "same concept">;
 def note_ambiguous_atomic_constraints_similar_expression : Note<
   "similar constraint expression here">;

clang/include/clang/Sema/Sema.h

BuildExprRequirement(
       bool IsSatisfied, SourceLocation NoexceptLoc,
       concepts::ExprRequirement::ReturnTypeRequirement ReturnTypeRequirement);
   concepts::TypeRequirement *BuildTypeRequirement(TypeSourceInfo *Type);
   concepts::TypeRequirement *
   BuildTypeRequirement(
       concepts::Requirement::SubstitutionDiagnostic *SubstDiag);
   concepts::NestedRequirement *BuildNestedRequirement(Expr *E);
   concepts::NestedRequirement *
-  BuildNestedRequirement(
-      concepts::Requirement::SubstitutionDiagnostic *SubstDiag);
+  BuildNestedRequirement(StringRef InvalidConstraintEntity,
+                         const ASTConstraintSatisfaction &Satisfaction);
   ExprResult ActOnRequiresExpr(SourceLocation RequiresKWLoc,
                                RequiresExprBodyDecl *Body,
                                ArrayRef<ParmVarDecl *> LocalParameters,
                                ArrayRef<concepts::Requirement *> Requirements,
                                SourceLocation ClosingBraceLoc);
 
   //===--------------------------------------------------------------------===//
   // C++ Variadic Templates (C++0x [temp.variadic])

clang/lib/AST/ASTConcept.cpp

 #include "clang/AST/ASTConcept.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/TemplateBase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
 using namespace clang;
 
-ASTConstraintSatisfaction::ASTConstraintSatisfaction(
-    const ASTContext &C, const ConstraintSatisfaction &Satisfaction)
-    : NumRecords{Satisfaction.Details.size()},
-      IsSatisfied{Satisfaction.IsSatisfied}, ContainsErrors{
-                                                 Satisfaction.ContainsErrors} {
-  for (unsigned I = 0; I < NumRecords; ++I) {
-    auto &Detail = Satisfaction.Details[I];
-    if (Detail.second.is<Expr *>())
-      new (getTrailingObjects<UnsatisfiedConstraintRecord>() + I)
-         UnsatisfiedConstraintRecord{Detail.first,
-                                     UnsatisfiedConstraintRecord::second_type(
-                                         Detail.second.get<Expr *>())};
-    else {
-      auto &SubstitutionDiagnostic =
-          *Detail.second.get<std::pair<SourceLocation, StringRef> *>();
-      unsigned MessageSize = SubstitutionDiagnostic.second.size();
-      char *Mem = new (C) char[MessageSize];
-      memcpy(Mem, SubstitutionDiagnostic.second.data(), MessageSize);
-      auto *NewSubstDiag = new (C) std::pair<SourceLocation, StringRef>(
-          SubstitutionDiagnostic.first, StringRef(Mem, MessageSize));
-      new (getTrailingObjects<UnsatisfiedConstraintRecord>() + I)
-         UnsatisfiedConstraintRecord{Detail.first,
-                                     UnsatisfiedConstraintRecord::second_type(
-                                         NewSubstDiag)};
-    }
-  }
+namespace {
+void CreatUnsatisfiedConstraintRecord(
+    const ASTContext &C, const UnsatisfiedConstraintRecord &Detail,
+    UnsatisfiedConstraintRecord *TrailingObject) {
+    new (TrailingObject) UnsatisfiedConstraintRecord{
+        Detail.first,
+        UnsatisfiedConstraintRecord::second_type(Detail.second.get<Expr *>())};
+    new (TrailingObject) UnsatisfiedConstraintRecord{
+        Detail.first, UnsatisfiedConstraintRecord::second_type(NewSubstDiag)};
+} // namespace
+
+ASTConstraintSatisfaction::ASTConstraintSatisfaction(
+    const ASTContext &C, const ConstraintSatisfaction &Satisfaction)
+    : NumRecords{Satisfaction.Details.size()},
+      IsSatisfied{Satisfaction.IsSatisfied}, ContainsErrors{
+                                                 Satisfaction.ContainsErrors} {
+  for (unsigned I = 0; I < NumRecords; ++I)
+    CreatUnsatisfiedConstraintRecord(
+        C, Satisfaction.Details[I],
+        getTrailingObjects<UnsatisfiedConstraintRecord>() + I);
+}
+
+ASTConstraintSatisfaction::ASTConstraintSatisfaction(
+    const ASTContext &C, const ASTConstraintSatisfaction &Satisfaction)
+    : NumRecords{Satisfaction.NumRecords},
+      IsSatisfied{Satisfaction.IsSatisfied},
+      ContainsErrors{Satisfaction.ContainsErrors} {
+  for (unsigned I = 0; I < NumRecords; ++I)
+    CreatUnsatisfiedConstraintRecord(
+        C, *(Satisfaction.begin() + I),
+        getTrailingObjects<UnsatisfiedConstraintRecord>() + I);
 }
 
 ASTConstraintSatisfaction *
 ASTConstraintSatisfaction::Create(const ASTContext &C,
                                   const ConstraintSatisfaction &Satisfaction) {
   std::size_t size =
       totalSizeToAlloc<UnsatisfiedConstraintRecord>(
           Satisfaction.Details.size());
   void *Mem = C.Allocate(size, alignof(ASTConstraintSatisfaction));
   return new (Mem) ASTConstraintSatisfaction(C, Satisfaction);
 }
 
+ASTConstraintSatisfaction *ASTConstraintSatisfaction::Rebuild(
+    const ASTContext &C, const ASTConstraintSatisfaction &Satisfaction) {
+  std::size_t size =
+      totalSizeToAlloc<UnsatisfiedConstraintRecord>(Satisfaction.NumRecords);
+  void *Mem = C.Allocate(size, alignof(ASTConstraintSatisfaction));
+  return new (Mem) ASTConstraintSatisfaction(C, Satisfaction);
+}
+
 void ConstraintSatisfaction::Profile(
     llvm::FoldingSetNodeID &ID, const ASTContext &C,
     const NamedDecl *ConstraintOwner, ArrayRef<TemplateArgument> TemplateArgs) {
   ID.AddPointer(ConstraintOwner);
   ID.AddInteger(TemplateArgs.size());
   for (auto &Arg : TemplateArgs)
     Arg.Profile(ID, C);
 }

clang/lib/AST/StmtPrinter.cpp

if (auto *TypeReq = dyn_cast<concepts::TypeRequirement>(Req)) {
             OS << "<<error-type>>";
           else if (RetReq.isTypeConstraint())
             RetReq.getTypeConstraint()->print(OS, Policy);
         }
       }
     } else {
       auto *NestedReq = cast<concepts::NestedRequirement>(Req);
       OS << "requires ";
-      if (NestedReq->isSubstitutionFailure())
+      if (NestedReq->hasInvalidConstraint())
         OS << "<<error-expression>>";
       else
         PrintExpr(NestedReq->getConstraintExpr());
     }
     OS << "; ";
   }
   OS << "}";
 }

clang/lib/AST/StmtProfile.cpp

if (auto *TypeReq = dyn_cast<concepts::TypeRequirement>(Req)) {
         Visit(RetReq.getTypeConstraint()->getImmediatelyDeclaredConstraint());
       } else {
         assert(RetReq.isSubstitutionFailure());
         ID.AddInteger(2);
       }
     } else {
       ID.AddInteger(concepts::Requirement::RK_Nested);
       auto *NestedReq = cast<concepts::NestedRequirement>(Req);
-      ID.AddBoolean(NestedReq->isSubstitutionFailure());
-      if (!NestedReq->isSubstitutionFailure())
+      ID.AddBoolean(NestedReq->hasInvalidConstraint());
+      if (!NestedReq->hasInvalidConstraint())
         Visit(NestedReq->getConstraintExpr());
     }
   }
 }
 
 static Stmt::StmtClass DecodeOperatorCall(const CXXOperatorCallExpr *S,
                                           UnaryOperatorKind &UnaryOp,
                                           BinaryOperatorKind &BinaryOp) {

clang/lib/Sema/SemaConcept.cpp

if (LogicalBinOp BO = ConstraintExpr) {
 
     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);
+      // LHS is instantiated while RHS is not. Skip creating invalid BinaryOp.
+      return 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 BO.recreateBinOp(S, LHSRes);
+      // LHS is instantiated while RHS is not. Skip creating invalid BinaryOp.
+      return LHSRes;
 
     ExprResult RHSRes = calculateConstraintSatisfaction(
         S, BO.getRHS(), Satisfaction, std::forward<AtomicEvaluator>(Evaluator));
     if (RHSRes.isInvalid())
       return ExprError();
 
     return BO.recreateBinOp(S, LHSRes, RHSRes);
   }
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
else
           << (int)First << SubstDiag->SubstitutedEntity;
     return;
   }
   default:
     llvm_unreachable("Unknown satisfaction status");
     return;
   }
 }
+static void diagnoseWellFormedUnsatisfiedConstraintExpr(Sema &S,
+                                                        Expr *SubstExpr,
+                                                        bool First = true);
 
 static void diagnoseUnsatisfiedRequirement(Sema &S,
                                            concepts::NestedRequirement *Req,
                                            bool First) {
-  if (Req->isSubstitutionFailure()) {
-    concepts::Requirement::SubstitutionDiagnostic *SubstDiag =
-        Req->getSubstitutionDiagnostic();
-    if (!SubstDiag->DiagMessage.empty())
-      S.Diag(SubstDiag->DiagLoc,
-             diag::note_nested_requirement_substitution_error)
-             << (int)First << SubstDiag->SubstitutedEntity
-             << SubstDiag->DiagMessage;
+  using SubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
+  for (auto &Pair : Req->getConstraintSatisfaction()) {
+    if (auto *SubstDiag = Pair.second.dyn_cast<SubstitutionDiagnostic *>())
+      S.Diag(SubstDiag->first, diag::note_nested_requirement_substitution_error)
+          << (int)First << Req->getInvalidConstraintEntity() << SubstDiag->second;
     else
-      S.Diag(SubstDiag->DiagLoc,
-             diag::note_nested_requirement_unknown_substitution_error)
-          << (int)First << SubstDiag->SubstitutedEntity;
-    return;
+      diagnoseWellFormedUnsatisfiedConstraintExpr(
+          S, Pair.second.dyn_cast<Expr *>(), First);
+    First = false;
   }
-  S.DiagnoseUnsatisfiedConstraint(Req->getConstraintSatisfaction(), First);
 }
 
-
 static void diagnoseWellFormedUnsatisfiedConstraintExpr(Sema &S,
                                                         Expr *SubstExpr,
-                                                        bool First = true) {
+                                                        bool First) {
   SubstExpr = SubstExpr->IgnoreParenImpCasts();
   if (BinaryOperator *BO = dyn_cast<BinaryOperator>(SubstExpr)) {
     switch (BO->getOpcode()) {
     // These two cases will in practice only be reached when using fold
     // expressions with || and &&, since otherwise the || and && will have been
     // broken down into atomic constraints during satisfaction checking.
     case BO_LOr:
       // Or evaluated to false - meaning both RHS and LHS evaluated to false.

clang/lib/Sema/SemaExprCXX.cpp

if (!Constraint->isInstantiationDependent() &&
       CheckConstraintSatisfaction(nullptr, {Constraint}, /*TemplateArgs=*/{},
                                   Constraint->getSourceRange(), Satisfaction))
     return nullptr;
   return new (Context) concepts::NestedRequirement(Context, Constraint,
                                                    Satisfaction);
 }
 
 concepts::NestedRequirement *
-Sema::BuildNestedRequirement(
-    concepts::Requirement::SubstitutionDiagnostic *SubstDiag) {
-  return new (Context) concepts::NestedRequirement(SubstDiag);
+Sema::BuildNestedRequirement(StringRef InvalidConstraintEntity,
+                       const ASTConstraintSatisfaction &Satisfaction) {
+  return new (Context) concepts::NestedRequirement(
+      InvalidConstraintEntity,
+      ASTConstraintSatisfaction::Rebuild(Context, Satisfaction));
 }
 
 RequiresExprBodyDecl *
 Sema::ActOnStartRequiresExpr(SourceLocation RequiresKWLoc,
                              ArrayRef<ParmVarDecl *> LocalParameters,
                              Scope *BodyScope) {
   assert(BodyScope);
 

clang/lib/Sema/SemaTemplateInstantiate.cpp

 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.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"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaConcept.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
 #include "clang/Sema/TemplateInstCallback.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TimeProfiler.h"
 
 using namespace clang;
 using namespace sema;
 
 //===----------------------------------------------------------------------===/
 // Template Instantiation Support
 //===----------------------------------------------------------------------===/
return RebuildExprRequirement(
       Req->isSimple(), Req->getNoexceptLoc(), std::move(*TransRetReq));
 }
 
 concepts::NestedRequirement *
 TemplateInstantiator::TransformNestedRequirement(
     concepts::NestedRequirement *Req) {
   if (!Req->isDependent() && !AlwaysRebuild())
     return Req;
-  if (Req->isSubstitutionFailure()) {
+  if (Req->hasInvalidConstraint()) {
     if (AlwaysRebuild())
-      return RebuildNestedRequirement(
-          Req->getSubstitutionDiagnostic());
+      return RebuildNestedRequirement(Req->getInvalidConstraintEntity(),
+                                      Req->getConstraintSatisfaction());
     return Req;
   }
   Sema::InstantiatingTemplate ReqInst(SemaRef,
       Req->getConstraintExpr()->getBeginLoc(), Req,
       Sema::InstantiatingTemplate::ConstraintsCheck{},
       Req->getConstraintExpr()->getSourceRange());
 
   ExprResult TransConstraint;
   ConstraintSatisfaction Satisfaction;
   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()) {
-      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() &&
-        !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());
-          }));
+    llvm::SmallVector<Expr *> Result;
+    if (!SemaRef.CheckConstraintSatisfaction(
+            nullptr, {Req->getConstraintExpr()}, Result, TemplateArgs,
+            Req->getConstraintExpr()->getSourceRange(), Satisfaction))
+      TransConstraint = Result[0];
+    assert(!Trap.hasErrorOccurred() && "Substitution failures must be handled "
+                                       "by CheckConstraintSatisfaction.");
+  if (TransConstraint.isUsable() &&
+      TransConstraint.get()->isInstantiationDependent())
+    return new (SemaRef.Context)
+        concepts::NestedRequirement(TransConstraint.get());
+  if (TransConstraint.isInvalid() || !TransConstraint.get() ||
+      Satisfaction.HasSubstitutionFailure()) {
+    SmallString<128> Entity;
+    llvm::raw_svector_ostream OS(Entity);
+    char *EntityBuf = new (SemaRef.Context) char[Entity.size()];
+    std::copy(Entity.begin(), Entity.end(), EntityBuf);
+    return new (SemaRef.Context) concepts::NestedRequirement(
+        SemaRef.Context, StringRef(EntityBuf, Entity.size()), Satisfaction);
   }
-  if (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/lib/Sema/TreeTransform.h

   concepts::ExprRequirement *
   RebuildExprRequirement(Expr *E, bool IsSimple, SourceLocation NoexceptLoc,
                          concepts::ExprRequirement::ReturnTypeRequirement Ret) {
     return SemaRef.BuildExprRequirement(E, IsSimple, NoexceptLoc,
                                         std::move(Ret));
   }
 
   concepts::NestedRequirement *
-  RebuildNestedRequirement(
-      concepts::Requirement::SubstitutionDiagnostic *SubstDiag) {
-    return SemaRef.BuildNestedRequirement(SubstDiag);
+  RebuildNestedRequirement(StringRef InvalidConstraintEntity,
+                           const ASTConstraintSatisfaction &Satisfaction) {
+    return SemaRef.BuildNestedRequirement(InvalidConstraintEntity,
+                                          Satisfaction);
   }
 
   concepts::NestedRequirement *RebuildNestedRequirement(Expr *Constraint) {
     return SemaRef.BuildNestedRequirement(Constraint);
   }
 
   /// \brief Build a new Objective-C boxed expression.
   ///
return getDerived().RebuildExprRequirement(
       TransExpr.get<concepts::Requirement::SubstitutionDiagnostic *>(),
       Req->isSimple(), Req->getNoexceptLoc(), std::move(*TransRetReq));
 }
 
 template<typename Derived>
 concepts::NestedRequirement *
 TreeTransform<Derived>::TransformNestedRequirement(
     concepts::NestedRequirement *Req) {
-  if (Req->isSubstitutionFailure()) {
+  if (Req->hasInvalidConstraint()) {
     if (getDerived().AlwaysRebuild())
       return getDerived().RebuildNestedRequirement(
-          Req->getSubstitutionDiagnostic());
+          Req->getInvalidConstraintEntity(), Req->getConstraintSatisfaction());
     return Req;
   }
   ExprResult TransConstraint =
       getDerived().TransformExpr(Req->getConstraintExpr());
   if (TransConstraint.isInvalid())
     return nullptr;
   return getDerived().RebuildNestedRequirement(TransConstraint.get());
 }

clang/lib/Serialization/ASTReaderStmt.cpp

switch (RK) {
                   std::move(*Req), Status, SubstitutedConstraintExpr);
         else
           R = new (Record.getContext()) concepts::ExprRequirement(
                   E.get<concepts::Requirement::SubstitutionDiagnostic *>(),
                   RK == concepts::Requirement::RK_Simple, NoexceptLoc,
                   std::move(*Req));
       } break;
       case concepts::Requirement::RK_Nested: {
-        if (/* IsSubstitutionDiagnostic */Record.readInt()) {
+        bool HasInvalidConstraint = Record.readInt();
+        if (HasInvalidConstraint) {
+          std::string InvalidConstraint = Record.readString();
+          char *InvalidConstraintBuf =
+              new (Record.getContext()) char[InvalidConstraint.size()];
+          std::copy(InvalidConstraint.begin(), InvalidConstraint.end(),
+                    InvalidConstraintBuf);
           R = new (Record.getContext()) concepts::NestedRequirement(
-              readSubstitutionDiagnostic(Record));
+              Record.getContext(),
+              StringRef(InvalidConstraintBuf, InvalidConstraint.size()),
+              readConstraintSatisfaction(Record));
           break;
         }
         Expr *E = Record.readExpr();
         if (E->isInstantiationDependent())
           R = new (Record.getContext()) concepts::NestedRequirement(E);
         else
           R = new (Record.getContext())
               concepts::NestedRequirement(Record.getContext(), E,

clang/lib/Serialization/ASTWriterStmt.cpp

if (auto *TypeReq = dyn_cast<concepts::TypeRequirement>(R)) {
         } else {
           assert(RetReq.isEmpty());
           Record.push_back(0);
         }
       }
     } else {
       auto *NestedReq = cast<concepts::NestedRequirement>(R);
       Record.push_back(concepts::Requirement::RK_Nested);
-      Record.push_back(NestedReq->isSubstitutionFailure());
-      if (NestedReq->isSubstitutionFailure()){
-        addSubstitutionDiagnostic(Record,
-                                  NestedReq->getSubstitutionDiagnostic());
+      Record.push_back(NestedReq->hasInvalidConstraint());
+      if (NestedReq->hasInvalidConstraint()) {
+        Record.AddString(NestedReq->getInvalidConstraintEntity());
+        addConstraintSatisfaction(Record, *NestedReq->Satisfaction);
       } else {
-        Record.AddStmt(NestedReq->Value.get<Expr *>());
+        Record.AddStmt(NestedReq->getConstraintExpr());
         if (!NestedReq->isDependent())
           addConstraintSatisfaction(Record, *NestedReq->Satisfaction);
       }
     }
   }
   Record.AddSourceLocation(E->getEndLoc());
 
   Code = serialization::EXPR_REQUIRES;

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'}}
     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 =
namespace std_example {
   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;
+template<class T>  concept False = false;
+
+struct S { double value; };
+
+template <class T>
+concept Pipes = requires (T x) {
+   requires True<decltype(x.value)> || True<T> || False<T>;
+   requires False<T> || True<T> || True<decltype(x.value)>;
+};
+
+template <class T>
+concept Amps1 = requires (T x) {
+   requires True<decltype(x.value)> && True<T> && !False<T>; // #Amps1
+};
+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) { // #foo1
+  requires
+  True<decltype(x.value)> // #foo1Value
+  && True<T>;
+} {}
+template<class T> void fooPipes() requires Pipes<T> {}
+template<class T> void fooAmps1() requires Amps1<T> {} // #fooAmps1
+void foo() {
+  foo1<S>();
+  foo1<int>(); // expected-error {{no matching function for call to 'foo1'}}
+  // expected-note@#foo1Value {{because 'True<decltype(x.value)> && True<T>' would be invalid: member reference base type 'int' is not a structure or union}}
+  // expected-note@#foo1 {{candidate template ignored: constraints not satisfied [with T = int]}}
+  fooPipes<S>();
+  fooPipes<int>();
+  fooAmps1<S>();
+  fooAmps1<int>(); // expected-error {{no matching function for call to 'fooAmps1'}}
+  // expected-note@#fooAmps1 {{candidate template ignored: constraints not satisfied [with T = int]}}
+  // expected-note@#fooAmps1 {{because 'int' does not satisfy 'Amps1'}}
+  // expected-note@#Amps1 {{because 'True<decltype(x.value)> && True<T> && !False<T>' would be invalid: member reference base type 'int' is not a structure or union}}
+}
+
+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.
+// This happens also for requires expression and is not restricted to nested requirement.
+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 ErrorExpressions_NotSF {
+template<typename T> struct X { static constexpr bool value = T::value; }; // #X_Value
+struct True { static constexpr bool value = true; };
+struct False { static constexpr bool value = false; };
+template<typename T> concept C = true;
+template<typename T> concept F = false;
+
+template<typename T> requires requires(T) { requires C<T> || X<T>::value; } void foo();
+
+template<typename T> requires requires(T) { requires C<T> && X<T>::value; } void bar(); // #bar
+template<typename T> requires requires(T) { requires F<T> || (X<T>::value && C<T>); } void baz();
+
+void func() {
+  foo<True>();
+  foo<False>();
+  foo<int>();
+
+  bar<True>();
+  bar<False>();
+  // expected-error@-1 {{no matching function for call to 'bar'}}
+  // expected-note@#bar {{while substituting template arguments into constraint expression here}}
+  // expected-note@#bar {{while checking the satisfaction of nested requirement requested here}}
+  // expected-note@#bar {{candidate template ignored: constraints not satisfied [with T = False]}}
+  // expected-note@#bar {{because 'X<False>::value' evaluated to false}}
+
+  bar<int>();
+  // expected-note@-1 {{while checking constraint satisfaction for template 'bar<int>' required here}} \
+  // expected-note@-1 {{in instantiation of function template specialization}}
+  // expected-note@#bar {{in instantiation of static data member}}
+  // expected-note@#bar {{in instantiation of requirement here}}
+  // expected-note@#bar {{while checking the satisfaction of nested requirement requested here}}
+  // expected-note@#bar {{while substituting template arguments into constraint expression here}}
+  // expected-error@#X_Value {{type 'int' cannot be used prior to '::' because it has no members}}
+}
+}
+}

clang/test/PCH/cxx2a-requires-expr.cpp

 // RUN: %clang_cc1 -emit-pch -std=c++2a -o %t %s
 // RUN: %clang_cc1 -std=c++2a -x ast -ast-print %t | FileCheck %s
 
 // RUN: %clang_cc1 -emit-pch -std=c++2a -fpch-instantiate-templates -o %t %s
 // RUN: %clang_cc1 -std=c++2a -x ast -ast-print %t | FileCheck %s
 
 template<typename T>
 concept C = true;
 
 template<typename T, typename U>
 concept C2 = true;
 
 template<typename T>
 bool f() {
-  // CHECK: requires (T t) { t++; { t++ } noexcept -> C; { t++ } -> C2<int>; typename T::a; requires T::val; };
+  // CHECK: requires (T t) { t++; { t++ } noexcept -> C; { t++ } -> C2<int>; typename T::a; requires T::val; requires C<typename T::val> || (C<typename T::val> || C<T>); };
   return requires (T t) {
     t++;
     { t++ } noexcept -> C;
     { t++ } -> C2<int>;
     typename T::a;
     requires T::val;
+    requires C<typename T::val> || (C<typename T::val> || C<T>);
   };
 }

clang/tools/libclang/CIndex.cpp

if (ER.getKind() == Requirement::RK_Compound) {
         if (const auto *Cons = RTR.getTypeConstraint())
           VisitTypeConstraint(*Cons);
       }
     }
     break;
   }
   case Requirement::RK_Nested: {
     const NestedRequirement &NR = cast<NestedRequirement>(R);
-    if (!NR.isSubstitutionFailure()) {
+    if (!NR.hasInvalidConstraint()) {
       if (Visit(NR.getConstraintExpr()))
         return true;
     }
     break;
   }
   }
   return false;
 }