[Concepts] Correctly handle failure when checking concepts recursively

Based on discussion on the core reflector, it was made clear that a
concept that depends on itself should be a hard error, not a constraint
failure. This patch implements a stack of constraint-checks-in-progress
to make sure we quit, rather than hitting stack-exhaustion.

Note that we DO need to be careful to make sure we still check
constraints properly that are caused by a previous constraint, but not
derived from (such as when a check causes us to check special member
function generation), so we cannot use the existing logic to see if this
is being instantiated.

This fixes https://github.com/llvm/llvm-project/issues/44304 and
https://github.com/llvm/llvm-project/issues/50891.

Differential Revision: https://reviews.llvm.org/D136975

clang/docs/ReleaseNotes.rst

@@ -270,6 +270,10 @@ Bug Fixes
   `Issue 58679 <https://github.com/llvm/llvm-project/issues/58679>`_
 - Fix a crash when a ``btf_type_tag`` attribute is applied to the pointee of
   a function pointer.
+- Fix a number of recursively-instantiated constraint issues, which would possibly
+  result in a stack overflow.
+  `Issue 44304 <https://github.com/llvm/llvm-project/issues/44304>`_
+  `Issue 50891 <https://github.com/llvm/llvm-project/issues/50891>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -5205,6 +5205,8 @@ def err_function_template_partial_spec : Error<
 def err_template_recursion_depth_exceeded : Error<
   "recursive template instantiation exceeded maximum depth of %0">,
   DefaultFatal, NoSFINAE;
+def err_constraint_depends_on_self : Error<
+  "satisfaction of constraint '%0' depends on itself">, NoSFINAE;
 def note_template_recursion_depth : Note<
   "use -ftemplate-depth=N to increase recursive template instantiation depth">;
 

clang/include/clang/Sema/Sema.h

@@ -7221,7 +7221,43 @@ private:
       FunctionDecl *FD, llvm::Optional<ArrayRef<TemplateArgument>> TemplateArgs,
       LocalInstantiationScope &Scope);
 
+private:
+  // The current stack of constraint satisfactions, so we can exit-early.
+  llvm::SmallVector<llvm::FoldingSetNodeID, 10> SatisfactionStack;
+
 public:
+  void PushSatisfactionStackEntry(const llvm::FoldingSetNodeID &ID) {
+    SatisfactionStack.push_back(ID);
+  }
+
+  void PopSatisfactionStackEntry() { SatisfactionStack.pop_back(); }
+
+  bool SatisfactionStackContains(const llvm::FoldingSetNodeID &ID) const {
+    return llvm::find(SatisfactionStack, ID) != SatisfactionStack.end();
+  }
+
+  // Resets the current SatisfactionStack for cases where we are instantiating
+  // constraints as a 'side effect' of normal instantiation in a way that is not
+  // indicative of recursive definition.
+  class SatisfactionStackResetRAII {
+    llvm::SmallVector<llvm::FoldingSetNodeID, 10> BackupSatisfactionStack;
+    Sema &SemaRef;
+
+  public:
+    SatisfactionStackResetRAII(Sema &S) : SemaRef(S) {
+      SemaRef.SwapSatisfactionStack(BackupSatisfactionStack);
+    }
+
+    ~SatisfactionStackResetRAII() {
+      SemaRef.SwapSatisfactionStack(BackupSatisfactionStack);
+    }
+  };
+
+  void
+  SwapSatisfactionStack(llvm::SmallVectorImpl<llvm::FoldingSetNodeID> &NewSS) {
+    SatisfactionStack.swap(NewSS);
+  }
+
   const NormalizedConstraint *
   getNormalizedAssociatedConstraints(
       NamedDecl *ConstrainedDecl, ArrayRef<const Expr *> AssociatedConstraints);

clang/lib/Sema/SemaConcept.cpp

@@ -146,6 +146,17 @@ bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression,
   return true;
 }
 
+namespace {
+struct SatisfactionStackRAII {
+  Sema &SemaRef;
+  SatisfactionStackRAII(Sema &SemaRef, llvm::FoldingSetNodeID FSNID)
+      : SemaRef(SemaRef) {
+      SemaRef.PushSatisfactionStackEntry(FSNID);
+  }
+  ~SatisfactionStackRAII() { SemaRef.PopSatisfactionStackEntry(); }
+};
+} // namespace
+
 template <typename AtomicEvaluator>
 static ExprResult
 calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
@@ -258,6 +269,29 @@ calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
   return SubstitutedAtomicExpr;
 }
 
+static bool
+DiagRecursiveConstraintEval(Sema &S, llvm::FoldingSetNodeID &ID, const Expr *E,
+                            const MultiLevelTemplateArgumentList &MLTAL) {
+  E->Profile(ID, S.Context, /*Canonical=*/true);
+  for (const auto &List : MLTAL)
+    for (const auto &TemplateArg : List.Args)
+      TemplateArg.Profile(ID, S.Context);
+
+  // Note that we have to do this with our own collection, because there are
+  // times where a constraint-expression check can cause us to need to evaluate
+  // other constriants that are unrelated, such as when evaluating a recovery
+  // expression, or when trying to determine the constexpr-ness of special
+  // members. Otherwise we could just use the
+  // Sema::InstantiatingTemplate::isAlreadyBeingInstantiated function.
+  if (S.SatisfactionStackContains(ID)) {
+    S.Diag(E->getExprLoc(), diag::err_constraint_depends_on_self)
+        << const_cast<Expr *>(E) << E->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
 static ExprResult calculateConstraintSatisfaction(
     Sema &S, const NamedDecl *Template, SourceLocation TemplateNameLoc,
     const MultiLevelTemplateArgumentList &MLTAL, const Expr *ConstraintExpr,
@@ -278,6 +312,16 @@ static ExprResult calculateConstraintSatisfaction(
               AtomicExpr->getSourceRange());
           if (Inst.isInvalid())
             return ExprError();
+
+          llvm::FoldingSetNodeID ID;
+          if (DiagRecursiveConstraintEval(S, ID, AtomicExpr, MLTAL)) {
+            Satisfaction.IsSatisfied = false;
+            Satisfaction.ContainsErrors = true;
+            return ExprEmpty();
+          }
+
+          SatisfactionStackRAII StackRAII(S, ID);
+
           // We do not want error diagnostics escaping here.
           Sema::SFINAETrap Trap(S);
           SubstitutedExpression =
@@ -414,6 +458,7 @@ bool Sema::CheckConstraintSatisfaction(
   if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
                                     ConvertedConstraints, TemplateArgsLists,
                                     TemplateIDRange, *Satisfaction)) {
+    OutSatisfaction = *Satisfaction;
     return true;
   }
 

clang/lib/Sema/SemaDeclCXX.cpp

@@ -7193,6 +7193,11 @@ specialMemberIsConstexpr(Sema &S, CXXRecordDecl *ClassDecl,
                          bool ConstRHS,
                          CXXConstructorDecl *InheritedCtor = nullptr,
                          Sema::InheritedConstructorInfo *Inherited = nullptr) {
+  // Suppress duplicate constraint checking here, in case a constraint check
+  // caused us to decide to do this.  Any truely recursive checks will get
+  // caught during these checks anyway.
+  Sema::SatisfactionStackResetRAII SSRAII{S};
+
   // If we're inheriting a constructor, see if we need to call it for this base
   // class.
   if (InheritedCtor) {
@@ -7289,8 +7294,8 @@ static bool defaultedSpecialMemberIsConstexpr(
   //      class is a constexpr function, and
   for (const auto &B : ClassDecl->bases()) {
     const RecordType *BaseType = B.getType()->getAs<RecordType>();
-    if (!BaseType) continue;
-
+    if (!BaseType)
+      continue;
     CXXRecordDecl *BaseClassDecl = cast<CXXRecordDecl>(BaseType->getDecl());
     if (!specialMemberIsConstexpr(S, BaseClassDecl, CSM, 0, ConstArg,
                                   InheritedCtor, Inherited))

clang/lib/Sema/SemaOverload.cpp

@@ -13147,17 +13147,16 @@ DiagnoseTwoPhaseOperatorLookup(Sema &SemaRef, OverloadedOperatorKind Op,
 namespace {
 class BuildRecoveryCallExprRAII {
   Sema &SemaRef;
+  Sema::SatisfactionStackResetRAII SatStack;
+
 public:
-  BuildRecoveryCallExprRAII(Sema &S) : SemaRef(S) {
+  BuildRecoveryCallExprRAII(Sema &S) : SemaRef(S), SatStack(S) {
     assert(SemaRef.IsBuildingRecoveryCallExpr == false);
     SemaRef.IsBuildingRecoveryCallExpr = true;
   }
 
-  ~BuildRecoveryCallExprRAII() {
-    SemaRef.IsBuildingRecoveryCallExpr = false;
-  }
+  ~BuildRecoveryCallExprRAII() { SemaRef.IsBuildingRecoveryCallExpr = false; }
 };
-
 }
 
 /// Attempts to recover from a call where no functions were found.

clang/test/SemaTemplate/concepts-GH53213.cpp

@@ -1,49 +0,0 @@
-// RUN: %clang_cc1 -std=c++20 -verify %s
-namespace GH53213 {
-template<typename T>
-concept c = requires(T t) { f(t); }; // #CDEF
-
-auto f(c auto); // #FDEF
-
-void g() {
-  f(0);
-  // expected-error@-1{{no matching function for call to 'f'}}
-  // expected-note@#FDEF{{constraints not satisfied}}
-  // expected-note@#FDEF{{because 'int' does not satisfy 'c'}}
-  // expected-note@#CDEF{{because 'f(t)' would be invalid: no matching function for call to 'f'}}
-}
-} // namespace GH53213 
-
-namespace GH45736 {
-struct constrained;
-
-template<typename T>
-  struct type {
-  };
-template<typename T>
-  constexpr bool f(type<T>) {
-      return true;
-  }
-
-template<typename T>
-  concept matches = f(type<T>());
-
-
-struct constrained {
-    template<typename U> requires matches<U>
-        explicit constrained(U value) {
-            }
-};
-
-bool f(constrained const &) {
-    return true;
-}
-
-struct outer {
-    constrained state;
-};
-
-bool f(outer const & x) {
-    return f(x.state);
-}
-} // namespace GH45736

clang/test/SemaTemplate/concepts-recursive-inst.cpp

@@ -0,0 +1,122 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+namespace GH53213 {
+template<typename T>
+concept c = requires(T t) { f(t); }; // #CDEF
+
+auto f(c auto); // #FDEF
+
+void g() {
+  f(0);
+  // expected-error@-1{{no matching function for call to 'f'}}
+  // expected-note@#FDEF{{constraints not satisfied}}
+  // expected-note@#FDEF{{because 'int' does not satisfy 'c'}}
+  // expected-note@#CDEF{{because 'f(t)' would be invalid: no matching function for call to 'f'}}
+}
+} // namespace GH53213 
+
+namespace GH45736 {
+struct constrained;
+
+template<typename T>
+  struct type {
+  };
+template<typename T>
+  constexpr bool f(type<T>) {
+      return true;
+  }
+
+template<typename T>
+  concept matches = f(type<T>());
+
+
+struct constrained {
+    template<typename U> requires matches<U>
+        explicit constrained(U value) {
+            }
+};
+
+bool f(constrained const &) {
+    return true;
+}
+
+struct outer {
+    constrained state;
+};
+
+bool f(outer const & x) {
+    return f(x.state);
+}
+} // namespace GH45736
+
+namespace DirectRecursiveCheck {
+template<class T>
+concept NotInf = true;
+template<class T>
+concept Inf = requires(T& v){ // #INF_REQ
+  {begin(v)}; // #INF_BEGIN_EXPR
+};
+
+void begin(NotInf auto& v){ } // #NOTINF_BEGIN
+// This lookup should fail, since it results in a recursive check.
+// However, this is a 'hard failure'(not a SFINAE failure or constraints
+// violation), so it needs to cause the entire lookup to fail.
+void begin(Inf auto& v){ } // #INF_BEGIN
+
+struct my_range{
+} rng;
+
+void baz() {
+auto it = begin(rng); // #BEGIN_CALL
+// expected-error@#INF_BEGIN {{satisfaction of constraint 'Inf<Inf auto>' depends on itself}}
+// expected-note@#INF_BEGIN {{while substituting template arguments into constraint expression here}}
+// expected-note@#INF_BEGIN_EXPR {{while checking constraint satisfaction for template 'begin<DirectRecursiveCheck::my_range>' required here}}
+// expected-note@#INF_BEGIN_EXPR {{while substituting deduced template arguments into function template 'begin'}}
+// expected-note@#INF_BEGIN_EXPR {{in instantiation of requirement here}}
+// expected-note@#INF_REQ {{while substituting template arguments into constraint expression here}}
+// expected-note@#INF_BEGIN {{while checking the satisfaction of concept 'Inf<DirectRecursiveCheck::my_range>' requested here}}
+// expected-note@#INF_BEGIN {{while substituting template arguments into constraint expression here}}
+// expected-note@#BEGIN_CALL {{while checking constraint satisfaction for template 'begin<DirectRecursiveCheck::my_range>' required here}}
+// expected-note@#BEGIN_CALL {{in instantiation of function template specialization}}
+
+// Fallout of the failure is failed lookup, which is necessary to stop odd
+// cascading errors.
+// expected-error@#BEGIN_CALL {{no matching function for call to 'begin'}}
+// expected-note@#NOTINF_BEGIN {{candidate function}}
+// expected-note@#INF_BEGIN{{candidate template ignored: constraints not satisfied}}
+}
+} // namespace DirectRecursiveCheck
+
+namespace GH50891 {
+  template <typename T>
+  concept Numeric = requires(T a) { // #NUMERIC
+      foo(a); // #FOO_CALL
+    };
+
+  struct Deferred {
+    friend void foo(Deferred);
+    template <Numeric TO> operator TO(); // #OP_TO
+  };
+
+  static_assert(Numeric<Deferred>); // #STATIC_ASSERT
+  // expected-error@#OP_TO {{satisfaction of constraint 'Numeric<TO>' depends on itself}}
+  // expected-note@#OP_TO {{while substituting template arguments into constraint expression here}}
+  // FIXME: The following two refer to type-parameter-0-0, it would be nice to
+  // see if we could instead diagnose with the sugared name.
+  // expected-note@#FOO_CALL {{while checking constraint satisfaction for template}}
+  // expected-note@#FOO_CALL {{while substituting deduced template arguments into function template}}
+  // expected-note@#FOO_CALL {{in instantiation of requirement here}}
+  // expected-note@#NUMERIC {{while substituting template arguments into constraint expression here}}
+  // expected-note@#OP_TO {{skipping 2 contexts in backtrace}}
+  // expected-note@#FOO_CALL {{while checking constraint satisfaction for template}}
+  // expected-note@#FOO_CALL {{in instantiation of function template specialization}}
+  // expected-note@#FOO_CALL {{in instantiation of requirement here}}
+  // expected-note@#NUMERIC {{while substituting template arguments into constraint expression here}}
+  // expected-note@#STATIC_ASSERT{{while checking the satisfaction of concept 'Numeric<Deferred>' requested here}}
+
+  // Fallout of that failure is that deferred does not satisfy numeric,
+  // which is unfortunate, but about what we can accomplish here.
+  // expected-error@#STATIC_ASSERT {{static assertion failed}}
+  // expected-note@#STATIC_ASSERT{{because 'Deferred' does not satisfy 'Numeric'}}
+  // expected-note@#FOO_CALL {{because 'foo(a)' would be invalid}}
+} // namespace GH50891
+