[clang] Fix bad error recovery when classes are defined inside template

[ArtyomZabroda]

Fixes #91564

This is my first pull request to the LLVM and I would appreciate any feedback.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Artyom Zabroda (ArtyomZabroda)

Changes

Fixes #91564

This is my first pull request to the LLVM and I would appreciate any feedback.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaConcept.cpp (+8)
• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+8-1)
• (modified) clang/test/SemaCXX/concept-crash-on-diagnostic.cpp (+13)

diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index c6a54dc141ded..1c654f46e23b3 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -220,6 +220,14 @@ static ExprResult EvaluateAtomicConstraint(
     if (Inst.isInvalid())
       return ExprError();
 
+    if (const TemplateTypeParmType *TTPT =
+        dyn_cast<TemplateTypeParmType>(AtomicExpr->getType().getDesugaredType(S.Context))) {
+      TemplateTypeParmDecl *TTPD = TTPT->getDecl();
+      if (TTPD->isInvalidDecl()) {
+        return ExprError();
+      }
+    }
+
     llvm::FoldingSetNodeID ID;
     if (Template &&
         DiagRecursiveConstraintEval(S, ID, Template, AtomicExpr, MLTAL)) {
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 55e078f3180a2..3efd18c0dcd96 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -13717,8 +13717,15 @@ Decl *Sema::ActOnAliasDeclaration(Scope *S, AccessSpecifier AS,
                                   const ParsedAttributesView &AttrList,
                                   TypeResult Type, Decl *DeclFromDeclSpec) {
 
-  if (Type.isInvalid())
+  if (Type.isInvalid()) {
+    for (TemplateParameterList *TPL : TemplateParamLists) {
+      for (NamedDecl *D : *TPL) {
+        D->setInvalidDecl(true);
+      }
+    }
     return nullptr;
+  }
+    
 
   bool Invalid = false;
   DeclarationNameInfo NameInfo = GetNameFromUnqualifiedId(Name);
diff --git a/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp b/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
index 1efed72522fef..af254828b0fe7 100644
--- a/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
+++ b/clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
@@ -60,3 +60,16 @@ concept atomicish = requires() {
 };
 atomicish<int> f(); // expected-error {{expected 'auto' or 'decltype(auto)' after concept name}}
 } // namespace GH138820
+
+namespace GH91564 {
+template <class T> using A = struct B { // expected-error {{'GH91564::B' cannot be defined in a type alias template}}
+  template <class> void f() requires (T()); // expected-note {{candidate template ignored: failed template argument deduction}}
+};
+template void B::f<void>(); // expected-error {{explicit instantiation of 'f' does not refer to a function template, variable template, member function, member class, or static data member}}
+
+template <class T> using C = struct D { // expected-error {{'GH91564::D' cannot be defined in a type alias template}}
+  using E = T;
+};
+template <class> void g() requires (D::E()); // expected-note {{candidate template ignored: failed template argument deduction}}
+template void g<void>(); // expected-error {{explicit instantiation of 'g' does not refer to a function template, variable template, member function, member class, or static data member}}
+}
\ No newline at end of file

[mizvekov]

Thanks for working on this!

So this fixes the problem too narrowly, only does anything for concepts.

I think a more general way to fix this, would be to make the type of a TemplateTypeParmType be non-dependent when it is invalid.

Since the type for a TTP decl is created when the declaration is created, you could use TTPDecl->setTypeForDecl( to change its type to the built-in int type, which is a common error recovery strategy in clang.
You can do that at the same time as you set the TTP decl as invalid.

Double check you are doing this early enough: You want to change the type before the template parameter has any chance to be used.

[ArtyomZabroda]

I don't understand how changing TTPDecl->setTypeForDecl to int can solve the issue for constraints. If I remove the check for invalid decl that I've made in EvaluateAtomicConstraint and set the type to int for the TemplateTypeParmDecl as you suggested, then the same errors pop up in the compiler. I'm not confident with the codebase yet, so maybe I've missed something, but I can't see any code in CheckInstantiatedFunctionTemplateConstraints and functions inside of it that checks dependence of a type inside TemplateTypeParmDecl before substituting an argument.

[mizvekov]

If you change the type for the TTPDecl to int, then that changes the type produced whenever a typename lookup is performed for that template parameter name, and the TTPDecl essentially becomes unreachable within the program. So any users of T wouldn't be able to figure out there was a template parameter there, and should see no dependency.

I suspect you might not be changing the type early enough, the type should be changed as soon as we figure out we are getting into a struct definition, before the template parameter could be used as a base class, inside an attribute, or in the class body.

[ArtyomZabroda]

You are right, I didn't change the type early enough, and after I did everything worked as expected. However, I've encountered a problem while implementing this fix. ParseAliasDeclarationAfterDeclarator initially parses an invalid struct and only then it checks whether the struct definition should be there in the first place. The problem is that I don't know of any nice way to obtain the template parameters of an alias while parsing a struct definition. In a new commit I've used an improper way of passing template parameters to check if it fixes the issue. Maybe it would be better not to change the type as soon as getting into the struct definition, but instead to let it build itself improperly, and then when we figure out that struct definition shouldn't be there use TreeTransform to change this struct?

[mizvekov]

Hello, I missed the earlier notification for your message, and now I will be unavailable for the next couple of weeks, sorry about that.

Using a TreeTransform to fix things up post facto is certainly possible, but I think it should still be possible to detect we are getting into a struct definition from within an alias template.

I think whenever we get into an alias template, we push it into an evaluation context, and you might be able to look there.

I will take a better look at this in a couple of weeks.