[Clang] Don't form a type constraint if the concept is invalid

[zyn0217]

After 0dedd6f and 03229e7, invalid concept declarations might lack expressions for evaluation and normalization. This could make it crash in certain scenarios, apart from the one of evaluation concepts showed in 03229e7, there's also an issue when checking specializations where the normalization also relies on a non-null expression.

This patch prevents that by avoiding building up a type constraint in such situations, thereafter the template parameter wouldn't have a concept specialization of a null expression.

With this patch, the assumption in ASTWriterDecl is no longer valid. Namely, HasConstraint and TypeConstraintInitialized must now represent different meanings for both source fidelity and semantic requirements.

Fixes #115004
Fixes #121980

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

After 0dedd6f and 03229e7, invalid concept declarations might lack expressions for evaluation and normalization. This could make it crash in certain scenarios, apart from the one of evaluation concepts showed in 03229e7, there's also an issue when checking specializations where the normalization also relies on a non-null expression.

This patch prevents that by avoiding building up a type constraint in such situations, thereafter the template parameter wouldn't have a concept specialization of a null expression.

With this patch, the assumption in ASTWriterDecl is no longer valid. Namely, HasConstraint and TypeConstraintInitialized must now represent different meanings for both source fidelity and semantic requirements.

Fixes #115004
Fixes #121980

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaTemplate.cpp (+3)
• (modified) clang/lib/Serialization/ASTWriterDecl.cpp (-1)
• (modified) clang/test/SemaTemplate/concepts.cpp (+12)

diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 20ec2fbeaa6a8b..ce672b00893b0d 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4557,6 +4557,9 @@ Sema::CheckConceptTemplateId(const CXXScopeSpec &SS,
                              const TemplateArgumentListInfo *TemplateArgs) {
   assert(NamedConcept && "A concept template id without a template?");
 
+  if (NamedConcept->isInvalidDecl())
+    return ExprError();
+
   llvm::SmallVector<TemplateArgument, 4> SugaredConverted, CanonicalConverted;
   if (CheckTemplateArgumentList(
           NamedConcept, ConceptNameInfo.getLoc(),
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 75c1d9a6d438ce..c7b8759916f421 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1951,7 +1951,6 @@ void ASTDeclWriter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   Record.push_back(D->wasDeclaredWithTypename());
 
   const TypeConstraint *TC = D->getTypeConstraint();
-  assert((bool)TC == D->hasTypeConstraint());
   if (TC) {
     auto *CR = TC->getConceptReference();
     Record.push_back(CR != nullptr);
diff --git a/clang/test/SemaTemplate/concepts.cpp b/clang/test/SemaTemplate/concepts.cpp
index 312469313fc535..f335ca3bd22bc3 100644
--- a/clang/test/SemaTemplate/concepts.cpp
+++ b/clang/test/SemaTemplate/concepts.cpp
@@ -1165,3 +1165,15 @@ concept C = invalid; // expected-error {{use of undeclared identifier 'invalid'}
 bool val2 = C<int>;
 
 } // namespace GH109780
+
+namespace GH121980 {
+
+template <class>
+concept has_member_difference_type; // expected-error {{expected '='}}
+
+template <has_member_difference_type> struct incrementable_traits; // expected-note {{declared here}}
+
+template <has_member_difference_type Tp>
+struct incrementable_traits<Tp>; // expected-error {{not more specialized than the primary}}
+
+}

[shafik]

After commit review.

Thank you for the fix.

[shafik]

The logic here is a little tricky. The new condition is really "it has a constraint and it does not matter if it is initialized".

I feel like this deserves a comment.

It also makes me wonder if "initialized" is the right word here, isn't it really that it was invalid?

I am just thinking of folks coming to maintain this code in the future and understanding the intent and I don't think the intent is 💯 clear as written.