[Concepts] Recover properly from a RecoveryExpr in a concept

[erichkeane]

Summary

Discovered by reducing a different problem, we currently assert because
we failed to make the constraint expressions not dependent, since a
RecoveryExpr cannot be transformed.

This patch fixes that, and gets reasonably nice diagnostics by
introducing a concept (hah!) of "ContainsErrors" to the Satisfaction
types, which causes us to treat the candidate as non-viable.

However, just making THAT candidate non-viable would result in choosing
the 'next best' canddiate, which can result in awkward errors, where we
start evaluating a candidate that is not intended to be selected.
Because of this, and to make diagnostics more relevant, we now just
cause the entire lookup to result in a 'no-viable-candidates'.

This means we will only emit the list of candidates, rather than any
cascading failures.

[ychen]

The patch looks good. Two high-level questions:

• Does the similar thing happen for class templates? Like a constraint expr of a partial specialization has an error. Maybe we don't assert in this case?
• I suppose the constraint error does not always affect the overload resolution results. When it does not, an alternative would be to assume the constraint is a satisfaction failure and the compilation continues. Do you see any value in this approach? Personally, I could go either way. Basically a trade-off between pessimistic and optimistic.

clang/docs/ReleaseNotes.rst

213

clang/include/clang/Sema/Overload.h

931

How about ConstraintExprHasError? Cascading sounds like more details than useful.

clang/lib/Sema/SemaOverload.cpp

10123–10127

nit: might be easier to read

[erichkeane]

In D134542#3812211, @ychen wrote:

The patch looks good. Two high-level questions:

• Does the similar thing happen for class templates? Like a constraint expr of a partial specialization has an error. Maybe we don't assert in this case?

WE currently crash in that case as well: https://godbolt.org/z/MGMqz1x59 . This patch still crashes in that case, and we should fix that in a similar way. I'll put it on my list of things to do soon! I don't want to do it in the same patch, simply because the type resolution parts are going to be completely different, and would likely just double the size of this patch.

• I suppose the constraint error does not always affect the overload resolution results. When it does not, an alternative would be to assume the constraint is a satisfaction failure and the compilation continues. Do you see any value in this approach? Personally, I could go either way. Basically a trade-off between pessimistic and optimistic.

In cases where the constraint error does not affect overload resolution (like with short-circuiting), this patch makes no changes, and will continue without it. ONLY when a constraint that references a RecoveryExpr in some way is used will we 'quit' overload resolution.

I ALSO considered just marking as a failure, and continuing, but @tahonermann made a point in a private chat that the result is that we'll end up getting wacky follow-up errors. Consider something like:

template<typename T> concept HasFoo = /*Test for has foo*/;
   template<typename T> concept HasBarAlternative = /*test for has bar, but with a typo!*/;

   template<typename T> requires HasFoo<T>
   void do_thing(T &t) {
     t.Foo();
     t.Bar();
   }
   template<typename T> requires HasFoo<T> && HasBarAlternative<T>
   void do_thing(T&t) {
     t.Foo();
     t.BarAlternative();
   }

The result of just marking HasBarAlternative' as not satisfied, is the 1st do_thing` will be called. THEN you'd get an error on instantiating because of the lack of Bar. This seems like a worse behavior to me, and results in just nonsense-errors/not useful errors most of the time.

clang/include/clang/Sema/Overload.h

931

Yeah, my name is awful here... I'm not sure ConstraintExprHasError is correc tin this case (since this is an OverloadCandidate), so the question is "Does this candidate fail because this is a constraint that contains an error". I'll try to come up with something better.

Feel free to help bikeshed/workshop something better!

clang/lib/Sema/SemaOverload.cpp

10123–10127

yep! I like it. I tend to do early exit when I can, particularly on large functions, but this ended up being pretty small in the final version of this patch.

[erichkeane]

In D134542#3812292, @erichkeane wrote:

In D134542#3812211, @ychen wrote:

The patch looks good. Two high-level questions:

• Does the similar thing happen for class templates? Like a constraint expr of a partial specialization has an error. Maybe we don't assert in this case?

WE currently crash in that case as well: https://godbolt.org/z/MGMqz1x59 . This patch still crashes in that case, and we should fix that in a similar way. I'll put it on my list of things to do soon! I don't want to do it in the same patch, simply because the type resolution parts are going to be completely different, and would likely just double the size of this patch.

• I suppose the constraint error does not always affect the overload resolution results. When it does not, an alternative would be to assume the constraint is a satisfaction failure and the compilation continues. Do you see any value in this approach? Personally, I could go either way. Basically a trade-off between pessimistic and optimistic.

In cases where the constraint error does not affect overload resolution (like with short-circuiting), this patch makes no changes, and will continue without it. ONLY when a constraint that references a RecoveryExpr in some way is used will we 'quit' overload resolution.

I ALSO considered just marking as a failure, and continuing, but @tahonermann made a point in a private chat that the result is that we'll end up getting wacky follow-up errors. Consider something like:

template<typename T> concept HasFoo = /*Test for has foo*/;
   template<typename T> concept HasBarAlternative = /*test for has bar, but with a typo!*/;

   template<typename T> requires HasFoo<T>
   void do_thing(T &t) {
     t.Foo();
     t.Bar();
   }
   template<typename T> requires HasFoo<T> && HasBarAlternative<T>
   void do_thing(T&t) {
     t.Foo();
     t.BarAlternative();
   }

The result of just marking HasBarAlternative' as not satisfied, is the 1st do_thing` will be called. THEN you'd get an error on instantiating because of the lack of Bar. This seems like a worse behavior to me, and results in just nonsense-errors/not useful errors most of the time.

WOOPS! I was wrong about 'still crashes' here. I mis-applied one of your suggestions and it crashed for another reason! When properly done, this stops that crash, and the Class-template-partial-specializations are just invalid at that point. I think we should still do the same thing as far as failing-lookup for the same reasons, so I'll do that separately.

[erichkeane]

Changes as suggested by @ychen. Thanks!

[ychen]

In D134542#3812292, @erichkeane wrote:

In D134542#3812211, @ychen wrote:

The patch looks good. Two high-level questions:

• Does the similar thing happen for class templates? Like a constraint expr of a partial specialization has an error. Maybe we don't assert in this case?

WE currently crash in that case as well: https://godbolt.org/z/MGMqz1x59 . This patch still crashes in that case, and we should fix that in a similar way. I'll put it on my list of things to do soon! I don't want to do it in the same patch, simply because the type resolution parts are going to be completely different, and would likely just double the size of this patch.

• I suppose the constraint error does not always affect the overload resolution results. When it does not, an alternative would be to assume the constraint is a satisfaction failure and the compilation continues. Do you see any value in this approach? Personally, I could go either way. Basically a trade-off between pessimistic and optimistic.

In cases where the constraint error does not affect overload resolution (like with short-circuiting), this patch makes no changes, and will continue without it. ONLY when a constraint that references a RecoveryExpr in some way is used will we 'quit' overload resolution.
I ALSO considered just marking as a failure, and continuing, but @tahonermann made a point in a private chat that the result is that we'll end up getting wacky follow-up errors. Consider something like:

template<typename T> concept HasFoo = /*Test for has foo*/;
   template<typename T> concept HasBarAlternative = /*test for has bar, but with a typo!*/;

   template<typename T> requires HasFoo<T>
   void do_thing(T &t) {
     t.Foo();
     t.Bar();
   }
   template<typename T> requires HasFoo<T> && HasBarAlternative<T>
   void do_thing(T&t) {
     t.Foo();
     t.BarAlternative();
   }

The result of just marking HasBarAlternative' as not satisfied, is the 1st do_thing` will be called. THEN you'd get an error on instantiating because of the lack of Bar. This seems like a worse behavior to me, and results in just nonsense-errors/not useful errors most of the time.

Agreed that short-circuiting cases would not hit this issue. I actually meant cases where a supposedly failed constraint has errors (https://clang.godbolt.org/z/5P7fYYvao). But that could be reconsidered in the future if use cases arise. With this patch, we already handle this better than GCC/MSVC.

constexpr bool True = true;
constexpr bool False = b;
template <typename T> concept C = True;
template <typename T> concept D = False;

template<D T>
void foo(T) = delete;
template<C T>
void foo(T);

void g() {
    foo(1);
}

clang/include/clang/Sema/Overload.h

931

Thanks. This looks good to me.

[ychen]

LGTM.

[erichkeane]

In D134542#3812711, @ychen wrote:

In D134542#3812292, @erichkeane wrote:

In D134542#3812211, @ychen wrote:

The patch looks good. Two high-level questions:

• Does the similar thing happen for class templates? Like a constraint expr of a partial specialization has an error. Maybe we don't assert in this case?

WE currently crash in that case as well: https://godbolt.org/z/MGMqz1x59 . This patch still crashes in that case, and we should fix that in a similar way. I'll put it on my list of things to do soon! I don't want to do it in the same patch, simply because the type resolution parts are going to be completely different, and would likely just double the size of this patch.

• I suppose the constraint error does not always affect the overload resolution results. When it does not, an alternative would be to assume the constraint is a satisfaction failure and the compilation continues. Do you see any value in this approach? Personally, I could go either way. Basically a trade-off between pessimistic and optimistic.

In cases where the constraint error does not affect overload resolution (like with short-circuiting), this patch makes no changes, and will continue without it. ONLY when a constraint that references a RecoveryExpr in some way is used will we 'quit' overload resolution.
I ALSO considered just marking as a failure, and continuing, but @tahonermann made a point in a private chat that the result is that we'll end up getting wacky follow-up errors. Consider something like:

template<typename T> concept HasFoo = /*Test for has foo*/;
   template<typename T> concept HasBarAlternative = /*test for has bar, but with a typo!*/;

   template<typename T> requires HasFoo<T>
   void do_thing(T &t) {
     t.Foo();
     t.Bar();
   }
   template<typename T> requires HasFoo<T> && HasBarAlternative<T>
   void do_thing(T&t) {
     t.Foo();
     t.BarAlternative();
   }

The result of just marking HasBarAlternative' as not satisfied, is the 1st do_thing` will be called. THEN you'd get an error on instantiating because of the lack of Bar. This seems like a worse behavior to me, and results in just nonsense-errors/not useful errors most of the time.

Agreed that short-circuiting cases would not hit this issue. I actually meant cases where a supposedly failed constraint has errors (https://clang.godbolt.org/z/5P7fYYvao). But that could be reconsidered in the future if use cases arise. With this patch, we already handle this better than GCC/MSVC.

constexpr bool True = true;
constexpr bool False = b;
template <typename T> concept C = True;
template <typename T> concept D = False;

template<D T>
void foo(T) = delete;
template<C T>
void foo(T);

void g() {
    foo(1);
}

Thanks for the quick review! Yeah, that case is a tough one, we don't really have a good way of determining whether that was supposed to fail, or be the best match. GCC does the option of just treating it as failed, so it'll have the problem that I mentioned above. No idea what MSVC is doing though.... thats frightening.

[ychen]

In D134542#3814029, @erichkeane wrote:

Thanks for the quick review! Yeah, that case is a tough one, we don't really have a good way of determining whether that was supposed to fail, or be the best match. GCC does the option of just treating it as failed, so it'll have the problem that I mentioned above. No idea what MSVC is doing though.... thats frightening.

This paper (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2429r0.pdf) makes me like the current approach more. Basically, we better strive for intuitive diagnostics instead of diagnosing as many errors as possible but run the risk of lower-quality diagnostic messages.

clang/docs/ReleaseNotes.rst

- New analysis pass will now help preserve sugar when combining deductions, in an
   in a way that strips the sugar which is different between terms, and preserves
   those which are common.
 - Correctly diagnose use of an integer literal without a suffix whose
   underlying type is ``long long`` or ``unsigned long long`` as an extension in
   C89 mode . Clang previously only diagnosed if the literal had an explicit
   ``LL`` suffix.
 - Clang now correctly diagnoses index that refers past the last possible element
   of FAM-like arrays.
+- Clang will now diagnose an overload set where an option has a constraint that
+  refers to an expression with a previous error as nothing viable, so that it
+  doesn't generate strange cascading errors, particularly in cases where a
+  subsuming constraint fails, which would result in a less-specific overload to
+  be selected.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/include/clang/AST/ASTConcept.h

ConstraintSatisfaction(const NamedDecl *ConstraintOwner,
                          ArrayRef<TemplateArgument> TemplateArgs) :
       ConstraintOwner(ConstraintOwner), TemplateArgs(TemplateArgs.begin(),
                                                      TemplateArgs.end()) { }
 
   using SubstitutionDiagnostic = std::pair<SourceLocation, StringRef>;
   using Detail = llvm::PointerUnion<Expr *, SubstitutionDiagnostic *>;
 
   bool IsSatisfied = false;
+  bool ContainsErrors = false;
 
   /// \brief 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.
   llvm::SmallVector<std::pair<const Expr *, Detail>, 4> Details;
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &C) {

 /// necessary information to diagnose an unsatisfied constraint.
 ///
 /// This is safe to store in an AST node, as opposed to ConstraintSatisfaction.
 struct ASTConstraintSatisfaction final :
     llvm::TrailingObjects<ASTConstraintSatisfaction,
                           UnsatisfiedConstraintRecord> {
   std::size_t NumRecords;
   bool IsSatisfied : 1;
+  bool ContainsErrors : 1;
 
   const UnsatisfiedConstraintRecord *begin() const {
     return getTrailingObjects<UnsatisfiedConstraintRecord>();
   }
 
   const UnsatisfiedConstraintRecord *end() const {
     return getTrailingObjects<UnsatisfiedConstraintRecord>() + NumRecords;
   }

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_non_constant_constraint_expression : Error<
   "expression">;
 def err_non_bool_atomic_constraint : Error<
   "atomic constraint must be of type 'bool' (found %0)">;
 def err_template_arg_list_constraints_not_satisfied : Error<
   "constraints not satisfied for %select{class template|function template|variable template|alias template|"
   "template template parameter|template}0 %1%2">;
 def note_substituted_constraint_expr_is_ill_formed : Note<
   "because substituted constraint expression is ill-formed%0">;
+def note_constraint_references_error
+    : Note<"constraint depends on a previously diagnosed expression">;
 def note_atomic_constraint_evaluated_to_false : Note<
   "%select{and|because}0 '%1' evaluated to false">;
 def note_concept_specialization_constraint_evaluated_to_false : Note<
   "%select{and|because}0 '%1' evaluated to false">;
 def note_single_arg_concept_specialization_constraint_evaluated_to_false : Note<
   "%select{and|because}0 %1 does not satisfy %2">;
 def note_atomic_constraint_evaluated_to_false_elaborated : Note<
   "%select{and|because}0 '%1' (%2 %3 %4) evaluated to false">;

clang/include/clang/Sema/Overload.h

unsigned getNumParams() const {
           STy = STy->getPointeeType();
         return STy->castAs<FunctionProtoType>()->getNumParams();
       }
       if (Function)
         return Function->getNumParams();
       return ExplicitCallArguments;
     }
 
+    bool ConstraintFailureBecauseCascadingError() const;
+
   private:
     friend class OverloadCandidateSet;
     OverloadCandidate()
         : IsSurrogate(false), IsADLCandidate(CallExpr::NotADL), RewriteKind(CRK_None) {}
   };
 
   /// OverloadCandidateSet - A set of overload candidates, used in C++
   /// overload resolution (C++ 13.3).

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

clang/lib/AST/ComputeDependence.cpp

for (const TemplateArgumentLoc &ArgLoc :
        E->getTemplateArgsAsWritten()->arguments()) {
     TA |= ArgLoc.getArgument().getDependence() & InterestingDeps;
     if (TA == InterestingDeps)
       break;
   }
 
   ExprDependence D =
       ValueDependent ? ExprDependence::Value : ExprDependence::None;
-  return D | toExprDependence(TA);
+  auto Res = D | toExprDependence(TA);
+  if(!ValueDependent && E->getSatisfaction().ContainsErrors)
+    Res |= ExprDependence::Error;
+  return Res;
 }
 
 ExprDependence clang::computeDependence(ObjCArrayLiteral *E) {
   auto D = ExprDependence::None;
   Expr **Elements = E->getElements();
   for (unsigned I = 0, N = E->getNumElements(); I != N; ++I)
     D |= turnTypeToValueDependence(Elements[I]->getDependence());
   return D;

clang/lib/Sema/SemaConcept.cpp

calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
 
   if (SubstitutedAtomicExpr.isInvalid())
     return ExprError();
 
   if (!SubstitutedAtomicExpr.isUsable())
     // Evaluator has decided satisfaction without yielding an expression.
     return ExprEmpty();
 
+  // We don't have the ability to evaluate this, since it contains a
+  // RecoveryExpr, so we want to fail overload resolution.  Otherwise,
+  // we'd potentially pick up a different overload, and cause confusing
+  // diagnostics. SO, add a failure detail that will cause us to make this
+  // overload set not viable.
+  if (SubstitutedAtomicExpr.get()->containsErrors()) {
+    Satisfaction.IsSatisfied = false;
+    Satisfaction.ContainsErrors = true;
+
+    PartialDiagnostic Msg = S.PDiag(diag::note_constraint_references_error);
+    SmallString<128> DiagString;
+    DiagString = ": ";
+    Msg.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(
+        ConstraintExpr,
+        new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic{
+            SubstitutedAtomicExpr.get()->getBeginLoc(),
+            StringRef(Mem, MessageSize)});
+    return SubstitutedAtomicExpr;
+  }
+
   EnterExpressionEvaluationContext ConstantEvaluated(
       S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   SmallVector<PartialDiagnosticAt, 2> EvaluationDiags;
   Expr::EvalResult EvalResult;
   EvalResult.Diag = &EvaluationDiags;
   if (!SubstitutedAtomicExpr.get()->EvaluateAsConstantExpr(EvalResult,
                                                            S.Context) ||
       !EvaluationDiags.empty()) {

clang/lib/Sema/SemaOverload.cpp

void Sema::diagnoseEquivalentInternalLinkageDeclarations(
 
   for (auto *E : Equiv) {
     Module *M = getOwningModule(E);
     Diag(E->getLocation(), diag::note_equivalent_internal_linkage_decl)
         << !M << (M ? M->getFullModuleName() : "");
   }
 }
 
+bool OverloadCandidate::ConstraintFailureBecauseCascadingError() const {
+  if (DeductionFailure.Result != Sema::TDK_ConstraintsNotSatisfied)
+    return false;
+
+  return static_cast<CNSInfo *>(DeductionFailure.Data)
+      ->Satisfaction.ContainsErrors;
+}
+
 /// Computes the best viable function (C++ 13.3.3)
 /// within an overload candidate set.
 ///
 /// \param Loc The location of the function name (or operator symbol) for
 /// which overload resolution occurs.
 ///
 /// \param Best If overload resolution was successful or found a deleted
 /// function, \p Best points to the candidate function found.
if (ContainsSameSideCandidate) {
       llvm::erase_if(Candidates, IsWrongSideCandidate);
     }
   }
 
   // Find the best viable function.
   Best = end();
   for (auto *Cand : Candidates) {
     Cand->Best = false;
-    if (Cand->Viable)
+    if (Cand->Viable) {
       if (Best == end() ||
           isBetterOverloadCandidate(S, *Cand, *Best, Loc, Kind))
         Best = Cand;
+    } else if (Cand->ConstraintFailureBecauseCascadingError()) {
+      // This candidate has constraint that we were unable to evaluate because
+      // it referenced an expression that contained an error. Rather than fall
+      // back onto a potentially unintended candidate (made worse by by
+      // subsuming constraints), treat this as 'no viable candidate'.
+      Best = end();
+      return OR_No_Viable_Function;
+    }
   }
 
   // If we didn't find any viable functions, abort.
   if (Best == end())
     return OR_No_Viable_Function;
 
   llvm::SmallVector<const NamedDecl *, 4> EquivalentCands;
 

clang/lib/Serialization/ASTReaderStmt.cpp

void ASTStmtReader::VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
   E->setOperatorLoc(readSourceLocation());
   E->setRParenLoc(readSourceLocation());
 }
 
 static ConstraintSatisfaction
 readConstraintSatisfaction(ASTRecordReader &Record) {
   ConstraintSatisfaction Satisfaction;
   Satisfaction.IsSatisfied = Record.readInt();
+  Satisfaction.ContainsErrors = Record.readInt();
   if (!Satisfaction.IsSatisfied) {
     unsigned NumDetailRecords = Record.readInt();
     for (unsigned i = 0; i != NumDetailRecords; ++i) {
       Expr *ConstraintExpr = Record.readExpr();
       if (/* IsDiagnostic */Record.readInt()) {
         SourceLocation DiagLocation = Record.readSourceLocation();
         std::string DiagMessage = Record.readString();
         Satisfaction.Details.emplace_back(

clang/lib/Serialization/ASTWriterStmt.cpp

for (Stmt *S : E->children())
     Record.AddStmt(S);
   Code = serialization::EXPR_DEPENDENT_COAWAIT;
 }
 
 static void
 addConstraintSatisfaction(ASTRecordWriter &Record,
                           const ASTConstraintSatisfaction &Satisfaction) {
   Record.push_back(Satisfaction.IsSatisfied);
+  Record.push_back(Satisfaction.ContainsErrors);
   if (!Satisfaction.IsSatisfied) {
     Record.push_back(Satisfaction.NumRecords);
     for (const auto &DetailRecord : Satisfaction) {
       Record.AddStmt(const_cast<Expr *>(DetailRecord.first));
       auto *E = DetailRecord.second.dyn_cast<Expr *>();
       Record.push_back(E == nullptr);
       if (E)
         Record.AddStmt(E);

clang/test/SemaTemplate/concepts-recovery-expr.cpp

+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+// expected-error@+1{{use of undeclared identifier 'b'}}
+constexpr bool CausesRecoveryExpr = b;
+
+template<typename T>
+concept ReferencesCRE = CausesRecoveryExpr;
+
+template<typename T> requires CausesRecoveryExpr // #NVC1REQ
+void NoViableCands1(){} // #NVC1
+
+template<typename T> requires ReferencesCRE<T> // #NVC2REQ
+void NoViableCands2(){} // #NVC2
+
+template<ReferencesCRE T> // #NVC3REQ
+void NoViableCands3(){} // #NVC3
+
+void NVCUse() {
+  NoViableCands1<int>();
+  // expected-error@-1 {{no matching function for call to 'NoViableCands1'}}
+  // expected-note@#NVC1{{candidate template ignored: constraints not satisfied}}
+  // expected-note@#NVC1REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+
+  NoViableCands2<int>();
+  // expected-error@-1 {{no matching function for call to 'NoViableCands2'}}
+  // expected-note@#NVC2{{candidate template ignored: constraints not satisfied}}
+  // expected-note@#NVC2REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  NoViableCands3<int>();
+  // expected-error@-1 {{no matching function for call to 'NoViableCands3'}}
+  // expected-note@#NVC3{{candidate template ignored: constraints not satisfied}}
+  // expected-note@#NVC3REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+}
+
+template<typename T> requires CausesRecoveryExpr // #OVC1REQ
+void OtherViableCands1(){} // #OVC1
+
+template<typename T>
+void OtherViableCands1(){} // #OVC1_ALT
+
+template<typename T> requires ReferencesCRE<T> // #OVC2REQ
+void OtherViableCands2(){} // #OVC2
+
+template<typename T>
+void OtherViableCands2(){} // #OVC2_ALT
+
+template<ReferencesCRE T> // #OVC3REQ
+void OtherViableCands3(){} // #OVC3
+template<typename T>
+void OtherViableCands3(){} // #OVC3_ALT
+
+void OVCUse() {
+  OtherViableCands1<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherViableCands1'}}
+  // expected-note@#OVC1_ALT {{candidate function}}
+  // expected-note@#OVC1 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OVC1REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  OtherViableCands2<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherViableCands2'}}
+  // expected-note@#OVC2_ALT {{candidate function}}
+  // expected-note@#OVC2 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OVC2REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  OtherViableCands3<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherViableCands3'}}
+  // expected-note@#OVC3_ALT {{candidate function}}
+  // expected-note@#OVC3 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OVC3REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+}
+
+template<typename T> requires CausesRecoveryExpr // #OBNVC1REQ
+void OtherBadNoViableCands1(){} // #OBNVC1
+
+template<typename T> requires false // #OBNVC1REQ_ALT
+void OtherBadNoViableCands1(){} // #OBNVC1_ALT
+
+template<typename T> requires ReferencesCRE<T> // #OBNVC2REQ
+void OtherBadNoViableCands2(){} // #OBNVC2
+
+template<typename T> requires false// #OBNVC2REQ_ALT
+void OtherBadNoViableCands2(){} // #OBNVC2_ALT
+
+template<ReferencesCRE T> // #OBNVC3REQ
+void OtherBadNoViableCands3(){} // #OBNVC3
+template<typename T> requires false // #OBNVC3REQ_ALT
+void OtherBadNoViableCands3(){} // #OBNVC3_ALT
+
+void OBNVCUse() {
+  OtherBadNoViableCands1<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherBadNoViableCands1'}}
+  // expected-note@#OBNVC1_ALT {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OBNVC1REQ_ALT {{because 'false' evaluated to false}}
+  // expected-note@#OBNVC1 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OBNVC1REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  OtherBadNoViableCands2<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherBadNoViableCands2'}}
+  // expected-note@#OBNVC2_ALT {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OBNVC2REQ_ALT {{because 'false' evaluated to false}}
+  // expected-note@#OBNVC2 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OBNVC2REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  OtherBadNoViableCands3<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherBadNoViableCands3'}}
+  // expected-note@#OBNVC3_ALT {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OBNVC3REQ_ALT {{because 'false' evaluated to false}}
+  // expected-note@#OBNVC3 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#OBNVC3REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+}
+
+
+// Same tests with member functions.
+struct OVC {
+template<typename T> requires CausesRecoveryExpr // #MEMOVC1REQ
+void OtherViableCands1(){} // #MEMOVC1
+
+template<typename T>
+void OtherViableCands1(){} // #MEMOVC1_ALT
+
+template<typename T> requires ReferencesCRE<T> // #MEMOVC2REQ
+void OtherViableCands2(){} // #MEMOVC2
+
+template<typename T>
+void OtherViableCands2(){} // #MEMOVC2_ALT
+
+template<ReferencesCRE T> // #MEMOVC3REQ
+void OtherViableCands3(){} // #MEMOVC3
+template<typename T>
+void OtherViableCands3(){} // #MEMOVC3_ALT
+};
+
+void MemOVCUse() {
+  OVC S;
+  S.OtherViableCands1<int>();
+  // expected-error@-1 {{no matching member function for call to 'OtherViableCands1'}}
+  // expected-note@#MEMOVC1_ALT {{candidate function}}
+  // expected-note@#MEMOVC1 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#MEMOVC1REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  S.OtherViableCands2<int>();
+  // expected-error@-1 {{no matching member function for call to 'OtherViableCands2'}}
+  // expected-note@#MEMOVC2_ALT {{candidate function}}
+  // expected-note@#MEMOVC2 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#MEMOVC2REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  S.OtherViableCands3<int>();
+  // expected-error@-1 {{no matching member function for call to 'OtherViableCands3'}}
+  // expected-note@#MEMOVC3_ALT {{candidate function}}
+  // expected-note@#MEMOVC3 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#MEMOVC3REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+}
+
+struct StaticOVC {
+template<typename T> requires CausesRecoveryExpr // #SMEMOVC1REQ
+static void OtherViableCands1(){} // #SMEMOVC1
+
+template<typename T>
+static void OtherViableCands1(){} // #SMEMOVC1_ALT
+
+template<typename T> requires ReferencesCRE<T> // #SMEMOVC2REQ
+static void OtherViableCands2(){} // #SMEMOVC2
+
+template<typename T>
+static void OtherViableCands2(){} // #SMEMOVC2_ALT
+
+template<ReferencesCRE T> // #SMEMOVC3REQ
+static void OtherViableCands3(){} // #SMEMOVC3
+template<typename T>
+static void OtherViableCands3(){} // #SMEMOVC3_ALT
+};
+
+void StaticMemOVCUse() {
+  StaticOVC::OtherViableCands1<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherViableCands1'}}
+  // expected-note@#SMEMOVC1_ALT {{candidate function}}
+  // expected-note@#SMEMOVC1 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#SMEMOVC1REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  StaticOVC::OtherViableCands2<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherViableCands2'}}
+  // expected-note@#SMEMOVC2_ALT {{candidate function}}
+  // expected-note@#SMEMOVC2 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#SMEMOVC2REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+  StaticOVC::OtherViableCands3<int>();
+  // expected-error@-1 {{no matching function for call to 'OtherViableCands3'}}
+  // expected-note@#SMEMOVC3_ALT {{candidate function}}
+  // expected-note@#SMEMOVC3 {{candidate template ignored: constraints not satisfied}}
+  // expected-note@#SMEMOVC3REQ{{because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression}}
+}