Skip to content

[Clang] Don't assert on substituted-but-yet-expanded packs for nested lambdas #112896

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.

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

Merged
merged 3 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ Bug Fixes to C++ Support
- Clang no longer tries to capture non-odr used default arguments of template parameters of generic lambdas (#GH107048)
- Fixed a bug where defaulted comparison operators would remove ``const`` from base classes. (#GH102588)
- Fix a crash when using ``source_location`` in the trailing return type of a lambda expression. (#GH67134)
- A follow-up fix was added for (#GH61460), as the previous fix was not entirely correct. (#GH86361)
- A follow-up fix was added for (#GH61460), as the previous fix was not entirely correct. (#GH86361), (#GH112352)
- Fixed a crash in the typo correction of an invalid CTAD guide. (#GH107887)
- Fixed a crash when clang tries to subtitute parameter pack while retaining the parameter
pack. (#GH63819), (#GH107560)
Expand Down
49 changes: 34 additions & 15 deletions clang/lib/Sema/SemaTemplateVariadic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ namespace {
unsigned DepthLimit = (unsigned)-1;

#ifndef NDEBUG
bool ContainsFunctionParmPackExpr = false;
bool ContainsIntermediatePacks = false;
#endif

void addUnexpanded(NamedDecl *ND, SourceLocation Loc = SourceLocation()) {
Expand Down Expand Up @@ -114,6 +114,11 @@ namespace {
addUnexpanded(TTP);
}

#ifndef NDEBUG
ContainsIntermediatePacks |=
(bool)Template.getAsSubstTemplateTemplateParmPack();
#endif

return inherited::TraverseTemplateName(Template);
}

Expand Down Expand Up @@ -297,13 +302,28 @@ namespace {

#ifndef NDEBUG
bool TraverseFunctionParmPackExpr(FunctionParmPackExpr *) {
ContainsFunctionParmPackExpr = true;
ContainsIntermediatePacks = true;
return true;
}

bool TraverseSubstNonTypeTemplateParmPackExpr(
SubstNonTypeTemplateParmPackExpr *) {
ContainsIntermediatePacks = true;
return true;
}

bool VisitSubstTemplateTypeParmPackType(SubstTemplateTypeParmPackType *) {
ContainsIntermediatePacks = true;
return true;
}

bool containsFunctionParmPackExpr() const {
return ContainsFunctionParmPackExpr;
bool
VisitSubstTemplateTypeParmPackTypeLoc(SubstTemplateTypeParmPackTypeLoc) {
ContainsIntermediatePacks = true;
return true;
}

bool containsIntermediatePacks() const { return ContainsIntermediatePacks; }
#endif
};
}
Expand Down Expand Up @@ -439,21 +459,20 @@ bool Sema::DiagnoseUnexpandedParameterPack(Expr *E,
if (!E->containsUnexpandedParameterPack())
return false;

// FunctionParmPackExprs are special:
//
// 1) they're used to model DeclRefExprs to packs that have been expanded but
// had that expansion held off in the process of transformation.
//
// 2) they always have the unexpanded dependencies but don't introduce new
// unexpanded packs.
//
// We might encounter a FunctionParmPackExpr being a full expression, which a
// larger CXXFoldExpr would expand.
SmallVector<UnexpandedParameterPack, 2> Unexpanded;
CollectUnexpandedParameterPacksVisitor Visitor(Unexpanded);
Visitor.TraverseStmt(E);
assert((!Unexpanded.empty() || Visitor.containsFunctionParmPackExpr()) &&
#ifndef NDEBUG
// The expression might contain a type/subexpression that has been substituted
// but has the expansion held off, e.g. a FunctionParmPackExpr which a larger
// CXXFoldExpr would expand. It's only possible when expanding a lambda as a
// pattern of a fold expression, so don't fire on an empty result in that
// case.
bool LambdaReferencingOuterPacks =
getEnclosingLambdaOrBlock() && Visitor.containsIntermediatePacks();
assert((!Unexpanded.empty() || LambdaReferencingOuterPacks) &&
"Unable to find unexpanded parameter packs");
#endif
return DiagnoseUnexpandedParameterPacks(E->getBeginLoc(), UPPC, Unexpanded);
}

Expand Down
21 changes: 13 additions & 8 deletions clang/lib/Sema/TreeTransform.h
Original file line number Diff line number Diff line change
Expand Up @@ -8385,14 +8385,19 @@ TreeTransform<Derived>::TransformDeclStmt(DeclStmt *S) {
if (Transformed != D)
DeclChanged = true;

if (LSI && isa<TypeDecl>(Transformed))
LSI->ContainsUnexpandedParameterPack |=
getSema()
.getASTContext()
.getTypeDeclType(cast<TypeDecl>(Transformed))
.getCanonicalType()
.getTypePtr()
->containsUnexpandedParameterPack();
if (LSI) {
if (auto *TD = dyn_cast<TypeDecl>(Transformed))
LSI->ContainsUnexpandedParameterPack |=
getSema()
.getASTContext()
.getTypeDeclType(TD)
.getCanonicalType()
->containsUnexpandedParameterPack();

if (auto *VD = dyn_cast<VarDecl>(Transformed))
LSI->ContainsUnexpandedParameterPack |=
VD->getType()->containsUnexpandedParameterPack();
}
Comment on lines +8388 to +8400
Copy link
Contributor

Choose a reason for hiding this comment

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

The more I see that sort of things, the more @zygoloid 's idea of tracking unexpanded packs lexically seems compelling.

we probably should have have a separate function for "does this declaration contains an unexpanded pack"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I understand "tracking unexpanded packs lexically". Do you have a link to that context? It sounds like we need a new bit to track that in Decls, despite that flag being accessible through their QualTypes' dependencies for the status quo. In the latter regard, it looks like we just need a shortcut method for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, this is mostly a "gosh, this is getting unwieldy, i wonder if there is a better way", rather than a request for a change. I was referring to this #9395 (comment)


Decls.push_back(Transformed);
}
Expand Down
44 changes: 44 additions & 0 deletions clang/test/SemaCXX/lambda-pack-expansion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,47 @@ template <typename... Ts> void g2(Ts... p1s) {
void f1() { g(); }

} // namespace GH61460

namespace GH112352 {

template <class>
constexpr bool foo = false;

template <int>
constexpr bool bar = false;

template <template<class> class>
constexpr bool baz = false;

struct S {
template <typename... Types, int... Values> void foldExpr1() {
(void)[]<int... Is> {
([] {
Is;
// Propagate up the flag ContainsUnexpandedParameterPack from VarDecl.
S var(foo<Types>);
foo<Types>;
bar<Values>;
int a = Values;
} &&
...);
};
}

template <template<class> class... TTPs> void foldExpr2() {
(void)[]<int... Is> {
([] {
Is;
baz<TTPs>;
TTPs<int> D;
} && ...);
};
}
};

void use() {
S().foldExpr1();
S().foldExpr2();
}

} // namespace GH112352
Loading