[Sema] Fix computations of "unexpanded packs" in substituted lambdas #99882
Conversation
…andedPacks This addresses the FIXME in the code. There are tests for the new behavior in a follow up fix for llvm#99877, which also addresses other bugs that prevent exposing the wrong results of `ContainsUnexpandedPacks` in the outputs of the compiler without crashes.
This addresses a crash in llvm#99877 that happens on nested lambdas that use template parameter packs, see the added test. Before this patch, the code computing the 'ContainsUnexpandedPacks' was relying on a flag from `LambdaScopeInfo`, that was updated differently in parsing and template substitution. This led to a discrepancy in how template parameters and captures that contain unexpanded packs were handled. In particular, the substitution missed some cases where it was supposed to mark the lambda as containing unexpanded packs. In turn, this lead to invalid state when the corresponding lambda was used to create a substituted `CXXFoldExpr` as `CXXFoldExpr` relies on this particular flag to distinguish left and right folds, and the code using it relies on `getPattern` to have unexpanded packs. The latter was causing assertion failures and subsequent crashes in non-assertion builds. This commit addresses those issues by moving the code computing `ContainsUnexpandedPack` value into `ComputeDependence.cpp` and relying on structurally checking the dependencies of various components that substitute a lambda expression instead of relying on the code parity between parsing and template substitution. This should help to avoid issues like this in the future, as checking the correctness of structural code is much easier than ensuring that global state manipulation is correct across two versions of the code. It's also all in one file, as opposed to being spread to multiple files. The only exception to this rule right now is the lambda body, as statements do not carry around any dependency flags and we have to rely on the global state for them. Another tricky case is attributes, one of which was actually checked in the tests (`diagnose_if`). The new code path handles only `diagnose_if`, but the long-term solution there would likely be propagating the `ContainsUnexpandedPack` flag into `Attr`, similar to the `Attr::IsPackExpansion` flag we already have. Previously, the code examples like the one in the added test would crash on all attributes. However, simpler examples used to work and will now report an error if used inside fold expressions. This is a potential regression, but it also seems very rare and I think we can address it in a follow up. Lastly, this commit adds an asssertion that exactly one of the arguments to `CXXFoldExpr` contains an unexpanded parameter pack. It should help catch issues like this
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Ilya Biryukov (ilya-biryukov) ChangesThis addresses a crash in #99877 that happens on nested lambdas that use Before this patch, the code computing the 'ContainsUnexpandedPacks' was This commit addresses those issues by moving the code computing The only exception to this rule right now is the lambda body, as Another tricky case is attributes, one of which was actually checked in Lastly, this commit adds an asssertion that exactly one of the arguments Full diff: https://github.com/llvm/llvm-project/pull/99882.diff 9 Files Affected:
diff --git a/clang/include/clang/AST/ComputeDependence.h b/clang/include/clang/AST/ComputeDependence.h
index 6d3a51c379f9d..848c1f203c296 100644
--- a/clang/include/clang/AST/ComputeDependence.h
+++ b/clang/include/clang/AST/ComputeDependence.h
@@ -166,7 +166,7 @@ ExprDependence computeDependence(CXXTemporaryObjectExpr *E);
ExprDependence computeDependence(CXXDefaultInitExpr *E);
ExprDependence computeDependence(CXXDefaultArgExpr *E);
ExprDependence computeDependence(LambdaExpr *E,
- bool ContainsUnexpandedParameterPack);
+ bool BodyContainsUnexpandedPacks);
ExprDependence computeDependence(CXXUnresolvedConstructExpr *E);
ExprDependence computeDependence(CXXDependentScopeMemberExpr *E);
ExprDependence computeDependence(MaterializeTemporaryExpr *E);
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index c2feac525c1ea..c0d58cab58b9b 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -1975,7 +1975,8 @@ class LambdaExpr final : public Expr,
LambdaCaptureDefault CaptureDefault,
SourceLocation CaptureDefaultLoc, bool ExplicitParams,
bool ExplicitResultType, ArrayRef<Expr *> CaptureInits,
- SourceLocation ClosingBrace, bool ContainsUnexpandedParameterPack);
+ SourceLocation ClosingBrace,
+ bool BodyContainsUnexpandedParameterPack);
/// Construct an empty lambda expression.
LambdaExpr(EmptyShell Empty, unsigned NumCaptures);
@@ -1996,7 +1997,7 @@ class LambdaExpr final : public Expr,
LambdaCaptureDefault CaptureDefault, SourceLocation CaptureDefaultLoc,
bool ExplicitParams, bool ExplicitResultType,
ArrayRef<Expr *> CaptureInits, SourceLocation ClosingBrace,
- bool ContainsUnexpandedParameterPack);
+ bool BodyContainsUnexpandedParameterPack);
/// Construct a new lambda expression that will be deserialized from
/// an external source.
@@ -4854,15 +4855,7 @@ class CXXFoldExpr : public Expr {
CXXFoldExpr(QualType T, UnresolvedLookupExpr *Callee,
SourceLocation LParenLoc, Expr *LHS, BinaryOperatorKind Opcode,
SourceLocation EllipsisLoc, Expr *RHS, SourceLocation RParenLoc,
- std::optional<unsigned> NumExpansions)
- : Expr(CXXFoldExprClass, T, VK_PRValue, OK_Ordinary),
- LParenLoc(LParenLoc), EllipsisLoc(EllipsisLoc), RParenLoc(RParenLoc),
- NumExpansions(NumExpansions ? *NumExpansions + 1 : 0), Opcode(Opcode) {
- SubExprs[SubExpr::Callee] = Callee;
- SubExprs[SubExpr::LHS] = LHS;
- SubExprs[SubExpr::RHS] = RHS;
- setDependence(computeDependence(this));
- }
+ std::optional<unsigned> NumExpansions);
CXXFoldExpr(EmptyShell Empty) : Expr(CXXFoldExprClass, Empty) {}
diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
index 700e361ef83f1..c998670377208 100644
--- a/clang/include/clang/Sema/ScopeInfo.h
+++ b/clang/include/clang/Sema/ScopeInfo.h
@@ -895,8 +895,9 @@ class LambdaScopeInfo final :
/// Whether any of the capture expressions requires cleanups.
CleanupInfo Cleanup;
- /// Whether the lambda contains an unexpanded parameter pack.
- bool ContainsUnexpandedParameterPack = false;
+ /// Whether the lambda body contains an unexpanded parameter pack.
+ /// Note that the captures and template paramters are handled separately.
+ bool BodyContainsUnexpandedParameterPack = false;
/// Packs introduced by this lambda, if any.
SmallVector<NamedDecl*, 4> LocalPacks;
diff --git a/clang/lib/AST/ComputeDependence.cpp b/clang/lib/AST/ComputeDependence.cpp
index 62ca15ea398f5..fbb3c72438a56 100644
--- a/clang/lib/AST/ComputeDependence.cpp
+++ b/clang/lib/AST/ComputeDependence.cpp
@@ -850,9 +850,40 @@ ExprDependence clang::computeDependence(CXXDefaultArgExpr *E) {
}
ExprDependence clang::computeDependence(LambdaExpr *E,
- bool ContainsUnexpandedParameterPack) {
+ bool BodyContainsUnexpandedPacks) {
auto D = toExprDependenceForImpliedType(E->getType()->getDependence());
- if (ContainsUnexpandedParameterPack)
+
+ // Record the presence of unexpanded packs.
+ bool ContainsUnexpandedPack =
+ BodyContainsUnexpandedPacks ||
+ (E->getTemplateParameterList() &&
+ E->getTemplateParameterList()->containsUnexpandedParameterPack());
+ if (!ContainsUnexpandedPack) {
+ // Also look at captures.
+ for (const auto &C : E->explicit_captures()) {
+ if (!C.capturesVariable() || C.isPackExpansion())
+ continue;
+ auto *Var = C.getCapturedVar();
+ if ((!Var->isInitCapture() && Var->isParameterPack()) ||
+ (Var->isInitCapture() && !Var->isParameterPack() &&
+ cast<VarDecl>(Var)->getInit()->containsUnexpandedParameterPack())) {
+ ContainsUnexpandedPack = true;
+ break;
+ }
+ }
+ }
+ // FIXME: Support other attributes, e.g. by storing corresponding flag inside
+ // Attr (similar to Attr::IsPackExpansion).
+ if (!ContainsUnexpandedPack) {
+ for (auto *A : E->getCallOperator()->specific_attrs<DiagnoseIfAttr>()) {
+ if (A->getCond() && A->getCond()->containsUnexpandedParameterPack()) {
+ ContainsUnexpandedPack = true;
+ break;
+ }
+ }
+ }
+
+ if (ContainsUnexpandedPack)
D |= ExprDependence::UnexpandedPack;
return D;
}
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 722c7fcf0b0df..f95be88e6c087 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -61,27 +61,43 @@ TemplateParameterList::TemplateParameterList(const ASTContext& C,
bool IsPack = P->isTemplateParameterPack();
if (const auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(P)) {
- if (!IsPack && NTTP->getType()->containsUnexpandedParameterPack())
- ContainsUnexpandedParameterPack = true;
+ if (!IsPack) {
+ if (NTTP->getType()->containsUnexpandedParameterPack())
+ ContainsUnexpandedParameterPack = true;
+ else if (NTTP->hasDefaultArgument() &&
+ NTTP->getDefaultArgument()
+ .getArgument()
+ .containsUnexpandedParameterPack())
+ ContainsUnexpandedParameterPack = true;
+ }
if (NTTP->hasPlaceholderTypeConstraint())
HasConstrainedParameters = true;
} else if (const auto *TTP = dyn_cast<TemplateTemplateParmDecl>(P)) {
- if (!IsPack &&
- TTP->getTemplateParameters()->containsUnexpandedParameterPack())
- ContainsUnexpandedParameterPack = true;
- } else if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(P)) {
- if (const TypeConstraint *TC = TTP->getTypeConstraint()) {
- if (TC->getImmediatelyDeclaredConstraint()
- ->containsUnexpandedParameterPack())
+ if (!IsPack) {
+ if (TTP->getTemplateParameters()->containsUnexpandedParameterPack())
ContainsUnexpandedParameterPack = true;
+ else if (TTP->hasDefaultArgument() &&
+ TTP->getDefaultArgument()
+ .getArgument()
+ .containsUnexpandedParameterPack())
+ ContainsUnexpandedParameterPack = true;
+ }
+ } else if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(P)) {
+ if (!IsPack && TTP->hasDefaultArgument() &&
+ TTP->getDefaultArgument()
+ .getArgument()
+ .containsUnexpandedParameterPack()) {
+ ContainsUnexpandedParameterPack = true;
+ } else if (const TypeConstraint *TC = TTP->getTypeConstraint();
+ TC && TC->getImmediatelyDeclaredConstraint()
+ ->containsUnexpandedParameterPack()) {
+ ContainsUnexpandedParameterPack = true;
}
if (TTP->hasTypeConstraint())
HasConstrainedParameters = true;
} else {
llvm_unreachable("unexpected template parameter type");
}
- // FIXME: If a default argument contains an unexpanded parameter pack, the
- // template parameter list does too.
}
if (HasRequiresClause) {
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 8d2a1b5611ccc..a8f7aee7df1b3 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1254,7 +1254,7 @@ LambdaExpr::LambdaExpr(QualType T, SourceRange IntroducerRange,
SourceLocation CaptureDefaultLoc, bool ExplicitParams,
bool ExplicitResultType, ArrayRef<Expr *> CaptureInits,
SourceLocation ClosingBrace,
- bool ContainsUnexpandedParameterPack)
+ bool BodyContainsUnexpandedParameterPack)
: Expr(LambdaExprClass, T, VK_PRValue, OK_Ordinary),
IntroducerRange(IntroducerRange), CaptureDefaultLoc(CaptureDefaultLoc),
ClosingBrace(ClosingBrace) {
@@ -1276,7 +1276,7 @@ LambdaExpr::LambdaExpr(QualType T, SourceRange IntroducerRange,
// Copy the body of the lambda.
*Stored++ = getCallOperator()->getBody();
- setDependence(computeDependence(this, ContainsUnexpandedParameterPack));
+ setDependence(computeDependence(this, BodyContainsUnexpandedParameterPack));
}
LambdaExpr::LambdaExpr(EmptyShell Empty, unsigned NumCaptures)
@@ -1295,7 +1295,7 @@ LambdaExpr *LambdaExpr::Create(const ASTContext &Context, CXXRecordDecl *Class,
bool ExplicitParams, bool ExplicitResultType,
ArrayRef<Expr *> CaptureInits,
SourceLocation ClosingBrace,
- bool ContainsUnexpandedParameterPack) {
+ bool BodyContainsUnexpandedParameterPack) {
// Determine the type of the expression (i.e., the type of the
// function object we're creating).
QualType T = Context.getTypeDeclType(Class);
@@ -1305,7 +1305,7 @@ LambdaExpr *LambdaExpr::Create(const ASTContext &Context, CXXRecordDecl *Class,
return new (Mem)
LambdaExpr(T, IntroducerRange, CaptureDefault, CaptureDefaultLoc,
ExplicitParams, ExplicitResultType, CaptureInits, ClosingBrace,
- ContainsUnexpandedParameterPack);
+ BodyContainsUnexpandedParameterPack);
}
LambdaExpr *LambdaExpr::CreateDeserialized(const ASTContext &C,
@@ -1944,3 +1944,21 @@ CXXParenListInitExpr *CXXParenListInitExpr::CreateEmpty(ASTContext &C,
alignof(CXXParenListInitExpr));
return new (Mem) CXXParenListInitExpr(Empty, NumExprs);
}
+
+CXXFoldExpr::CXXFoldExpr(QualType T, UnresolvedLookupExpr *Callee,
+ SourceLocation LParenLoc, Expr *LHS,
+ BinaryOperatorKind Opcode, SourceLocation EllipsisLoc,
+ Expr *RHS, SourceLocation RParenLoc,
+ std::optional<unsigned> NumExpansions)
+ : Expr(CXXFoldExprClass, T, VK_PRValue, OK_Ordinary), LParenLoc(LParenLoc),
+ EllipsisLoc(EllipsisLoc), RParenLoc(RParenLoc),
+ NumExpansions(NumExpansions ? *NumExpansions + 1 : 0), Opcode(Opcode) {
+ // We rely on asserted invariant to distnguish left and right folds.
+ assert(((LHS && LHS->containsUnexpandedParameterPack()) !=
+ (RHS && RHS->containsUnexpandedParameterPack())) &&
+ "Exactly one of LHS or RHS should contain an unexpanded pack");
+ SubExprs[SubExpr::Callee] = Callee;
+ SubExprs[SubExpr::LHS] = LHS;
+ SubExprs[SubExpr::RHS] = RHS;
+ setDependence(computeDependence(this));
+}
diff --git a/clang/lib/Sema/SemaLambda.cpp b/clang/lib/Sema/SemaLambda.cpp
index 601077e9f3334..b4cb38c133a54 100644
--- a/clang/lib/Sema/SemaLambda.cpp
+++ b/clang/lib/Sema/SemaLambda.cpp
@@ -1109,8 +1109,6 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
PushDeclContext(CurScope, Method);
- bool ContainsUnexpandedParameterPack = false;
-
// Distinct capture names, for diagnostics.
llvm::DenseMap<IdentifierInfo *, ValueDecl *> CaptureNames;
@@ -1312,8 +1310,6 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
// Just ignore the ellipsis.
}
- } else if (Var->isParameterPack()) {
- ContainsUnexpandedParameterPack = true;
}
if (C->Init.isUsable()) {
@@ -1328,7 +1324,6 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
}
finishLambdaExplicitCaptures(LSI);
- LSI->ContainsUnexpandedParameterPack |= ContainsUnexpandedParameterPack;
PopDeclContext();
}
@@ -1380,8 +1375,6 @@ void Sema::ActOnLambdaClosureParameters(
AddTemplateParametersToLambdaCallOperator(LSI->CallOperator, LSI->Lambda,
TemplateParams);
LSI->Lambda->setLambdaIsGeneric(true);
- LSI->ContainsUnexpandedParameterPack |=
- TemplateParams->containsUnexpandedParameterPack();
}
LSI->AfterParameterList = true;
}
@@ -2079,7 +2072,7 @@ ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
bool ExplicitParams;
bool ExplicitResultType;
CleanupInfo LambdaCleanup;
- bool ContainsUnexpandedParameterPack;
+ bool BodyContainsUnexpandedParameterPack;
bool IsGenericLambda;
{
CallOperator = LSI->CallOperator;
@@ -2088,7 +2081,8 @@ ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
ExplicitParams = LSI->ExplicitParams;
ExplicitResultType = !LSI->HasImplicitReturnType;
LambdaCleanup = LSI->Cleanup;
- ContainsUnexpandedParameterPack = LSI->ContainsUnexpandedParameterPack;
+ BodyContainsUnexpandedParameterPack =
+ LSI->BodyContainsUnexpandedParameterPack;
IsGenericLambda = Class->isGenericLambda();
CallOperator->setLexicalDeclContext(Class);
@@ -2227,11 +2221,10 @@ ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
Cleanup.mergeFrom(LambdaCleanup);
- LambdaExpr *Lambda = LambdaExpr::Create(Context, Class, IntroducerRange,
- CaptureDefault, CaptureDefaultLoc,
- ExplicitParams, ExplicitResultType,
- CaptureInits, EndLoc,
- ContainsUnexpandedParameterPack);
+ LambdaExpr *Lambda = LambdaExpr::Create(
+ Context, Class, IntroducerRange, CaptureDefault, CaptureDefaultLoc,
+ ExplicitParams, ExplicitResultType, CaptureInits, EndLoc,
+ BodyContainsUnexpandedParameterPack);
// If the lambda expression's call operator is not explicitly marked constexpr
// and we are not in a dependent context, analyze the call operator to infer
// its constexpr-ness, suppressing diagnostics while doing so.
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 6df7f2223d267..8e47e9ee339f2 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -353,7 +353,11 @@ Sema::DiagnoseUnexpandedParameterPacks(SourceLocation Loc,
}
if (!EnclosingStmtExpr) {
- LSI->ContainsUnexpandedParameterPack = true;
+ // It is ok to have unexpanded packs in captures, template parameters
+ // and parameters too, but only the body statement does not store this
+ // flag, so we have to propagate it through LamdaScopeInfo.
+ if (LSI->AfterParameterList)
+ LSI->BodyContainsUnexpandedParameterPack = true;
return false;
}
} else {
diff --git a/clang/test/SemaCXX/fold_lambda_with_variadics.cpp b/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
new file mode 100644
index 0000000000000..45505d4f4c434
--- /dev/null
+++ b/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// expected-no-diagnostics
+struct tuple {
+ int x[3];
+};
+
+template <class F>
+int apply(F f, tuple v) {
+ return f(v.x[0], v.x[1], v.x[2]);
+}
+
+int Cartesian1(auto x, auto y) {
+ return apply([&](auto... xs) {
+ return (apply([xs](auto... ys) {
+ return (ys + ...);
+ }, y) + ...);
+ }, x);
+}
+
+int Cartesian2(auto x, auto y) {
+ return apply([&](auto... xs) {
+ return (apply([zs = xs](auto... ys) {
+ return (ys + ...);
+ }, y) + ...);
+ }, x);
+}
+
+template <int ...> struct Ints{};
+template <int> struct Choose {
+ template<class> struct Templ;
+};
+template <int ...x>
+int Cartesian3(auto y) {
+ return [&]<int ...xs>(Ints<xs...>) {
+ // check in default template arguments for
+ // - type template parameters,
+ (void)(apply([]<class = decltype(xs)>(auto... ys) {
+ return (ys + ...);
+ }, y) + ...);
+ // - template template parameters.
+ (void)(apply([]<template<class> class = Choose<xs>::template Templ>(auto... ys) {
+ return (ys + ...);
+ }, y) + ...);
+ // - non-type template parameters,
+ return (apply([]<int = xs>(auto... ys) {
+ return (ys + ...);
+ }, y) + ...);
+
+ }(Ints<x...>());
+}
+
+template <int ...x>
+int Cartesian4(auto y) {
+ return [&]<int ...xs>(Ints<xs...>) {
+ return (apply([]<decltype(xs) xx = 1>(auto... ys) {
+ return (ys + ...);
+ }, y) + ...);
+ }(Ints<x...>());
+}
+
+int Cartesian5(auto x, auto y) {
+ return apply([&](auto... xs) {
+ return (apply([](auto... ys) __attribute__((diagnose_if(!__is_same(decltype(xs), int), "message", "error"))) {
+ return (ys + ...);
+ }, y) + ...);
+ }, x);
+}
+
+
+int main() {
+ auto x = tuple({1, 2, 3});
+ auto y = tuple({4, 5, 6});
+ Cartesian1(x, y);
+ Cartesian2(x, y);
+ Cartesian3<1,2,3>(y);
+ Cartesian4<1,2,3>(y);
+ Cartesian5(x, y);
+}
|
|
This PR also includes changes from #99880, which I separated into another commit to simplify the review as this PR is a bit involved. |
|
At first glance this seems like a duplicate of #86265 |
That seems right, I was not aware of the other PR, thanks for pointing this out. |
There was a problem hiding this comment.
Thanks for working on this.
I did a rough look, and I think the general approach looks good. Although this unfortunately happens to be a duplicate work of #86265 (sorry I didn't get around to that during the 19 development cycle due to my timeframe), I think it's fine to follow this patch because it will tidy up things like how ContainsUnexpandedPacks are propagated and could avoid scattered flag setting, just as the PR said.
As @cor3ntin is our lambda's code owner, I'd like to leave the approval to him.
| // It is ok to have unexpanded packs in captures, template parameters | ||
| // and parameters too, but only the body statement does not store this | ||
| // flag, so we have to propagate it through LamdaScopeInfo. | ||
| if (LSI->AfterParameterList) | ||
| LSI->BodyContainsUnexpandedParameterPack = true; |
There was a problem hiding this comment.
Yeah, one thing I didn't notice in #86265 is that we could propagate up the unexpanded pack flag for statements through calls to DiagnoseUnexpandedParameterPacks(). (IIUC we call this function at the end of every expressions?)
There was a problem hiding this comment.
I think this just force us to do more work - going over the lambda twice is not ideal
There was a problem hiding this comment.
Wait I thought it is my approach that goes through the lambda body twice in a transformation. I didn't see additional calls to DiagnoseUnexpandedParameterPacks in this patch?
There was a problem hiding this comment.
Yeah, there aren't any additional recursive traversals. It's only a shallow traversal over the fields of the lambda itself and a few of their members.
There was a problem hiding this comment.
It's shallow but it's also work that we could avoid doing entirely - at least during parsing just by not having this if.
| @@ -0,0 +1,78 @@ | |||
| // RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s | |||
There was a problem hiding this comment.
It would be very nice if you can merge other tests from #86265 e.g. tests including constraints/noexcepts specifiers. That way this could completely supersede that.
There was a problem hiding this comment.
I think I prefer the approach chosen by @zyn0217.
The changes are more targeted, and using a visitor for the call operator avoid a lot of code duplication. In particular, if we want to properly handle attributes, we might need code in CollectUnexpandedParameterPacksVisitor. Or maybe we just need a generic solution to store a flag on attributes.
I'd also would like us to avoid going over the lambda multiple times if we don't need to.
I would suggest:
- Make a separate PR for template default arguments
- Make an NFC commit for the fold expression assert
- Help @zyn0217 ensure his PR covers all the failing tests
- Find a good solution for attributes. If you tell me that there is a business reason to get
diagnose_ifworking in 19, i think an ad-hoc solution is fine, as long as we replace it soon.
Does that seem reasonable?
| SubExprs[SubExpr::RHS] = RHS; | ||
| setDependence(computeDependence(this)); | ||
| } | ||
| std::optional<unsigned> NumExpansions); |
There was a problem hiding this comment.
This change is completely unrelated, feel free to make an NFC patch for that
| // It is ok to have unexpanded packs in captures, template parameters | ||
| // and parameters too, but only the body statement does not store this | ||
| // flag, so we have to propagate it through LamdaScopeInfo. | ||
| if (LSI->AfterParameterList) | ||
| LSI->BodyContainsUnexpandedParameterPack = true; |
There was a problem hiding this comment.
I think this just force us to do more work - going over the lambda twice is not ideal
The trick is that
I still feel that having the code computing unexpanded parameters scattered across the codebase and duplicated between parsing and tree-transform/template-instantiations is a bigger price to pay in terms of complexity and readability than the runtime costs here.
I've already sent it out in #99880. It has no tests, though, because I could not find another way to dump these or trigger, but the changes are quite obvious and you'll get tests from either of the PRs anyway.
Will do and rebase.
Sure, I am happy to either have the tests included here copied into #86265 or send follow up fixes covering anything that won't be included for whatever reason.
As mentioned above, I'm happy to write a patch for this as it seems relatively independent. Will post it when it's ready.
Yes, absolutely, it's a very reasonable option. I just wanted to reiterate that the costs of having the code handling unexpanded packs scattered and duplicated around parsing and template instantiations do seem high to me. |
|
@ilya-biryukov I'm a bit torn about how we would proceed: if Corentin insists on the current (which my patch continues the effort) implementation, do you think it feasible for me to merge part of your codes (specifically, the way we propagate up unexpanded flags for lambda bodies, some tests apart from those for lambda attributes) into #86265? Of course I'll add you as a co-author of that patch, if you're happy. (The attribute part could be split up as an individual PR afterward, if I understand our discussion correctly?) |
Yes, absolutely, feel free to borrow the stuff from here, both tests and code! |
|
I think we should first rebase on top of the default argument PR, remove the attribute change (definitively the wrong way to do that), and reassess where we go from there. My main concern is this line which seems very artificial https://github.com/llvm/llvm-project/pull/99882/files#diff-10901a3bb08d903a57820f09c2eb5f40cb4cd47c54ca3cad472106814c4bf15dR359 If we both agree that you would rather centralize everything during transform, I think it's fine but during parsing we already have the information available so we should try to use it - unless I am missing something? |
|
I can get around to my patch again very soon :) |
Yeah, I can see your point. My north-start vision for that issue is that is should all be structural and even We could actually get away without having this line, but it's there precisely to make sure that parsing and transform get the same outputs. The bugs we are trying to fix are an evidence of the fact that parsing is tested more rigorously and sharing the same inputs with transform gives more confidence that bugs won't show up.
Technically, we also have this information during transform in the same way, but, unfortunately, it requires a duplicate code path. I think my objections to a flag in I'm looking forward to what @zyn0217 comes up with, I'm sure we'll be in a better state when his patch lands. I will close this PR in the meantime to put more focus on #86265. |
|
On Tue, Jul 23, 2024, 16:04 Ilya Biryukov ***@***.***> wrote:
My main concern is this line which seems very artificial
https://github.com/llvm/llvm-project/pull/99882/files#diff-10901a3bb08d903a57820f09c2eb5f40cb4cd47c54ca3cad472106814c4bf15dR359
Yeah, I can see your point. My north-start vision for that issue is that
is should all be structural and even
LambdaScopeInfo::ContainsUnexpandedPack should go away in favor of
storing ContainsUnexpandedPack bit inside Stmt.
This would be excellent, but I think we would still need that just because
building a lambda is an horrible state machine.
We could actually get away without having this line, but it's there
precisely to make sure that parsing and transform get the same outputs. The
bugs we are trying to fix are an evidence of the fact that parsing is
tested more rigorously and sharing the same inputs with transform gives
more confidence that bugs won't show up.
If we both agree that you would rather centralize everything during
transform, I think it's fine but during parsing we already have the
information available so we should try to use it - unless I am missing
something?
Technically, we *also* have this information during transform in the same
way, but, unfortunately, it requires a duplicate code path. I think my
objections to a flag in LambdaScopeInfo would be much weaker if we
rewrote the code in a way that was guaranteed to be shared across parsing
and transform (putting it in computeDependence is just one way to do it).
That would also work for me, I think.
I'm looking forward to what @zyn0217 <https://github.com/zyn0217> comes up
… with, I'm sure we'll be in a better state when his patch lands.
I will close this PR in the meantime to put more focus on #86265
<#86265>.
—
Reply to this email directly, view it on GitHub
<#99882 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKX76YPCO6D6VGJ7FSNE5LZNZPHVAVCNFSM6AAAAABLIOJLFKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBVGM2TCOJZGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yeah, unfortunately there probably isn't a way to get rid of the state machine. |
This addresses a crash in #99877 that happens on nested lambdas that use
template parameter packs, see the added test.
Before this patch, the code computing the 'ContainsUnexpandedPacks' was
relying on a flag from
LambdaScopeInfo, that was updated differentlyin parsing and template substitution.
This led to a discrepancy in how template parameters and captures that
contain unexpanded packs were handled. In particular, the substitution
missed some cases where it was supposed to mark the lambda as containing
unexpanded packs.
In turn, this lead to invalid state when the corresponding lambda was used
to create a substituted
CXXFoldExprasCXXFoldExprrelies on thisparticular flag to distinguish left and right folds, and the code using
it relies on
getPatternto have unexpanded packs. The latter wascausing assertion failures and subsequent crashes in non-assertion
builds.
This commit addresses those issues by moving the code computing
ContainsUnexpandedPackvalue intoComputeDependence.cppand relyingon structurally checking the dependencies of various components that
substitute a lambda expression instead of relying on the code parity
between parsing and template substitution.
This should help to avoid issues like this in the future, as checking
the correctness of structural code is much easier than ensuring that
global state manipulation is correct across two versions of the code.
It's also all in one file, as opposed to being spread to multiple files.
The only exception to this rule right now is the lambda body, as
statements do not carry around any dependency flags and we have to rely
on the global state for them.
Another tricky case is attributes, one of which was actually checked in
the tests (
diagnose_if). The new code path handles onlydiagnose_if,but the long-term solution there would likely be propagating the
ContainsUnexpandedPackflag intoAttr, similar to theAttr::IsPackExpansionflag we already have. Previously, the codeexamples like the one in the added test would crash on all attributes.
However, simpler examples used to work and will now report an error if
used inside fold expressions. This is a potential regression, but it
also seems very rare and I think we can address it in a follow up.
Lastly, this commit adds an asssertion that exactly one of the arguments
to
CXXFoldExprcontains an unexpanded parameter pack. It should helpcatch issues like this.