[Sema] When checking for constraint equivalence, do not calculate satisfaction

[ilya-biryukov]

... and only look at equivalence of substituted expressions, not results of constraint satisfaction.

Fixes #74314.

There is already some existing machinery for that in TemplateInstantiator and Sema exposed separate functions for substituting expressions with intention to do that:

• Sema::SubstExpr should not evaluate constraints.
• Sema::SubstConstraintExpr should.

However, both functions used to be equivalent. This commit changes the former to actually avoid calculating constraint satisfaction and uses it in the code path that matches definition and declaration.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ilya Biryukov (ilya-biryukov)

Changes

... and only look at equivalence of substituted expressions, not results of constraint satisfaction.

Fixes #74314.

There is already some existing machinery for that in TemplateInstantiator and Sema exposed separate functions for substituting expressions with intention to do that:

• Sema::SubstExpr should not evaluate constraints.
• Sema::SubstConstraintExpr should.

However, both functions used to be equivalent. This commit changes the former to actually avoid calculating constraint satisfaction and uses it in the code path that matches definition and declaration.

---

Full diff: https://github.com/llvm/llvm-project/pull/74490.diff

4 Files Affected:

• (modified) clang/include/clang/Sema/Template.h (+1)
• (modified) clang/lib/Sema/SemaConcept.cpp (+8-7)
• (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+8)
• (modified) clang/test/SemaTemplate/concepts-out-of-line-def.cpp (+21)

diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h
index 2a553054a0ce5..ce44aca797b0f 100644
--- a/clang/include/clang/Sema/Template.h
+++ b/clang/include/clang/Sema/Template.h
@@ -564,6 +564,7 @@ enum class TemplateSubstitutionKind : char {
     const MultiLevelTemplateArgumentList &TemplateArgs;
     Sema::LateInstantiatedAttrVec* LateAttrs = nullptr;
     LocalInstantiationScope *StartingScope = nullptr;
+    // Whether to evaluate the C++20 constraints or simply substitute into them.
     bool EvaluateConstraints = true;
 
     /// A list of out-of-line class template partial
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 719c6aab74e01..be3541573377b 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -771,10 +771,9 @@ namespace {
   };
 } // namespace
 
-static const Expr *
-SubstituteConstraintExpression(Sema &S,
-                               const Sema::TemplateCompareNewDeclInfo &DeclInfo,
-                               const Expr *ConstrExpr) {
+static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
+    Sema &S, const Sema::TemplateCompareNewDeclInfo &DeclInfo,
+    const Expr *ConstrExpr) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
       DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(), /*Final=*/false,
       /*Innermost=*/nullptr,
@@ -798,7 +797,7 @@ SubstituteConstraintExpression(Sema &S,
   if (auto *RD = dyn_cast<CXXRecordDecl>(DeclInfo.getDeclContext()))
     ThisScope.emplace(S, const_cast<CXXRecordDecl *>(RD), Qualifiers());
   ExprResult SubstConstr =
-      S.SubstConstraintExpr(const_cast<clang::Expr *>(ConstrExpr), MLTAL);
+      S.SubstExpr(const_cast<clang::Expr *>(ConstrExpr), MLTAL);
   if (SFINAE.hasErrorOccurred() || !SubstConstr.isUsable())
     return nullptr;
   return SubstConstr.get();
@@ -814,12 +813,14 @@ bool Sema::AreConstraintExpressionsEqual(const NamedDecl *Old,
   if (Old && !New.isInvalid() && !New.ContainsDecl(Old) &&
       Old->getLexicalDeclContext() != New.getLexicalDeclContext()) {
     if (const Expr *SubstConstr =
-            SubstituteConstraintExpression(*this, Old, OldConstr))
+            SubstituteConstraintExpressionWithoutSatisfaction(*this, Old,
+                                                              OldConstr))
       OldConstr = SubstConstr;
     else
       return false;
     if (const Expr *SubstConstr =
-            SubstituteConstraintExpression(*this, New, NewConstr))
+            SubstituteConstraintExpressionWithoutSatisfaction(*this, New,
+                                                              NewConstr))
       NewConstr = SubstConstr;
     else
       return false;
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 6ad70109c8cee..971a4c99add0c 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1186,6 +1186,7 @@ namespace {
     const MultiLevelTemplateArgumentList &TemplateArgs;
     SourceLocation Loc;
     DeclarationName Entity;
+    // Whether to evaluate the C++20 constraints or simply substitute into them.
     bool EvaluateConstraints = true;
 
   public:
@@ -2489,6 +2490,12 @@ TemplateInstantiator::TransformNestedRequirement(
       Req->getConstraintExpr()->getBeginLoc(), Req,
       Sema::InstantiatingTemplate::ConstraintsCheck{},
       Req->getConstraintExpr()->getSourceRange());
+  if (!getEvaluateConstraints()) {
+    ExprResult TransConstraint = TransformExpr(Req->getConstraintExpr());
+    if (TransConstraint.isInvalid() || !TransConstraint.get())
+      return nullptr;
+    return RebuildNestedRequirement(TransConstraint.get());
+  }
 
   ExprResult TransConstraint;
   ConstraintSatisfaction Satisfaction;
@@ -4077,6 +4084,7 @@ Sema::SubstExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) {
   TemplateInstantiator Instantiator(*this, TemplateArgs,
                                     SourceLocation(),
                                     DeclarationName());
+  Instantiator.setEvaluateConstraints(false);
   return Instantiator.TransformExpr(E);
 }
 
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index f134394615fb2..7e79eeb6d279a 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -504,3 +504,24 @@ struct bar {
 bar<int> x;
 } // namespace GH61763
 
+
+namespace GH74314 {
+template <class T, class U> constexpr bool is_same_v = __is_same(T, U);
+template <class T, class U> constexpr bool is_not_same_v = !__is_same(T, U);
+
+template <class Result>
+concept something_interesting = requires {
+      true;
+      requires is_same_v<int, Result>;
+};
+
+template <class T>
+struct X {
+      void foo() requires requires { requires is_not_same_v<T, int>; };
+};
+
+template <class T>
+void X<T>::foo() requires requires { requires something_interesting<T>; } {}
+// expected-error@-1{{definition of 'foo' does not match any declaration}}
+// expected-note@*{{}}
+} // namespace GH74314
\ No newline at end of file

[ilya-biryukov]

@cor3ntin I have tried getting some lambda examples, but because lambda produces a unique type every time, I don't see how I can get a matching declaration with a lambda inside requires clause.

[erichkeane]

need a newline here.

[erichkeane]

I believe this ends up being incorrect? We end up calling SubstExpr pretty frequently 'inside' of other constraint expression checking I think, and I think we set whether to evaluate constraints with further checks based on the current instantiator.

So I think this ends up failing to check constraints in a cascading way. I DO know I tried something similar ot this patch at one point when doing the deferred diagnostics and ran into trouble.

IF we are going to do something like this (as I don't have sufficient evidence here to block I think), we need to hold this off until after the release so it has time to bake.

[ilya-biryukov]

I will sanity-check some call sites and try to break it with tests to understand this better.
The reason why I went with this change, is because of this comment on the SubstConstraintExpr:

  // Unlike the above, this evaluates constraints, which should only happen at
  // 'constraint checking' time.

The 'above' mentioned here is SubstExpr. If we don't update this function, I think we should update the comment (and remove SubstConstraintExpr as both functions are exactly the same right now).

[erichkeane]

Yeah, that comment is a little out of date I think. I initially split those two with the intent of making them do something different (and infact, almost exactly the change you made!), but ended up with a ton of test failures, particularly in libc++ for a while.

I think i ended up picking up most/all of those tests as lit tests during my implementation, but I'm not confident in it enough to have us merge this before the release branch next month.

[ilya-biryukov]

Oh, and another signal that gave me optimism is that all existing tests pass. However, I am not sure if they are extensive.

[erichkeane]

Oh, absolutely! I'm reassured somewhat by that fact of course, I was as comprehensive as I could be at the time. BUT when doing this the first time, I ended up with about a dozen reverts trying to work through it, so there is definitely possibility that I missed some. I'm reasonably confident/ok with this patch, just not until the release branch is taken.

[ilya-biryukov]

I have spent some time to understand this better and I agree that changing the default behavior of SubstExpr is quite a bet. So I made a more localized fix instead, please take a look. I believe we don't need to wait for a branch cut to land this change.

I have also changed SubstConstraintExpr to simply call SubstExpr and updated the comment to avoid confusion (as mentioned before, the comment used to say they're different, but they are actually the same).

I have also added tests to make sure the uses of requires-expr in SFINAE contexts decltype does not cause issues, I believe it tests other code paths a bit.