[Concepts] Avoid substituting into constraints for invalid TemplateDecls

[zyn0217]

Fixes #73885.

Substituting into constraints for invalid TemplateDecls might still yield dependent expressions and end up crashing later while attempting evaluation.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Fixes #73885.

Substituting into constraints for invalid TemplateDecls might still yield dependent expressions and end up crashing later while attempting evaluation.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/Sema/SemaConcept.cpp (+4)
• (modified) clang/test/SemaTemplate/instantiate-requires-expr.cpp (+10)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b4b5352a306c1c..83f98b97925250 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -831,6 +831,9 @@ Bug Fixes to C++ Support
 - Fix crash when parsing nested requirement. Fixes:
   (`#73112 <https://github.com/llvm/llvm-project/issues/73112>`_)
 
+- Fixed a crash when substituting into constraint expressions for invalid variable templates.
+  Fixes: (`#73885 <https://github.com/llvm/llvm-project/issues/73885>`_)
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 719c6aab74e017..73683fee7c1487 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -353,6 +353,10 @@ static ExprResult calculateConstraintSatisfaction(
           if (Inst.isInvalid())
             return ExprError();
 
+          // An empty expression for substitution failure messages.
+          if (Template && Template->isInvalidDecl())
+            return ExprEmpty();
+
           llvm::FoldingSetNodeID ID;
           if (Template &&
               DiagRecursiveConstraintEval(S, ID, Template, AtomicExpr, MLTAL)) {
diff --git a/clang/test/SemaTemplate/instantiate-requires-expr.cpp b/clang/test/SemaTemplate/instantiate-requires-expr.cpp
index ba82fc1313fc95..fb4127182d1cb6 100644
--- a/clang/test/SemaTemplate/instantiate-requires-expr.cpp
+++ b/clang/test/SemaTemplate/instantiate-requires-expr.cpp
@@ -227,3 +227,13 @@ struct r6 {};
 
 using r6i = r6<int>;
 // expected-error@-1 {{constraints not satisfied for class template 'r6' [with T = int]}}
+
+namespace GH73885 {
+template <typename T> // expected-error {{extraneous template parameter list}}
+template <typename T> // expected-error {{'T' shadows template parameter}}
+                      // expected-note@-2 {{template parameter is declared here}}
+requires(T{})
+T e_v;
+double e = e_v<double>;
+// expected-error@-1 {{constraints not satisfied for variable template 'e_v' [with T = double]}}
+}

[cor3ntin]

Can you explain why not return ExprError here? Does it just produce more diagnostics?

[zyn0217]

My initial thought was to emit substitution details ([T = double] here). But now I feel like this is puzzling since we're now giving "constraints not satisfied" rather than "constraint must be of type 'bool'" in the case.

I'm a bit torn here: either we lose substitution details or produce inaccurate error-recovery messages. The latter matches what we're doing now in assertion-free builds. https://cpp1.godbolt.org/z/3KzPEYhn3