Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ArtyomZabroda wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fc280b
[clang] Fix bad error recovery when classes are defined inside templa…
ArtyomZabroda May 31, 2025
c52af95
very inaccurate fix
ArtyomZabroda Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/lib/Sema/SemaConcept.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Copy link
Contributor

@mizvekov mizvekov May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ArtyomZabroda ArtyomZabroda Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mizvekov mizvekov Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ArtyomZabroda ArtyomZabroda Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mizvekov mizvekov Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

llvm::FoldingSetNodeID ID;
if (Template &&
DiagRecursiveConstraintEval(S, ID, Template, AtomicExpr, MLTAL)) {
Expand Down
9 changes: 8 additions & 1 deletion clang/lib/Sema/SemaDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 13 additions & 0 deletions clang/test/SemaCXX/concept-crash-on-diagnostic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
}