[Sema] Preserve ContainsUnexpandedParameterPack in TransformLambdaExpr #86265

zyn0217 · 2024-03-22T10:17:37Z

The lambda ContainsUnexpandedParameterPack flag is used for the expressions' dependency computing and is therefore essential for pack expansion. We previously lost the flag's preservation during the lambda's transform, which caused some issues, e.g. a fold expression couldn't properly expand inside a template.

This patch alleviates the issue by retaining the flag in more scenarios. Note that we still have problems with constraints involving packs regarding lambdas, and dealing with that would take more effort, and we'd like to fix them in the future.

Fixes #56852
Fixes #85667
Mitigates #99877 because the attributes were not handled in this patch.

github-actions · 2024-03-23T11:45:08Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-03-23T11:45:08Z

✅ With the latest revision this PR passed the C/C++ code formatter.

zyn0217 · 2024-07-18T07:44:17Z

@cor3ntin I dropped the fix for #18873 in this patch because I feel like that requires additional work beyond setting the flag. I plan to leave that after the cut of 19, WDYT? Otherwise, can we make this patch into 19?

cor3ntin · 2024-07-18T08:30:53Z

What you have make sense i think. Let me know when you remove the draft status so i can start reviewing the details

zyn0217 · 2024-07-18T08:43:11Z

I was waiting for the CI, but it seems Linux CI would take hours. However that shouldn't block the review I think, because Windows CI is green now.

clang/test/SemaTemplate/lambda-capture-pack.cpp

clang/lib/Sema/TreeTransform.h

zyn0217 · 2024-08-01T14:17:59Z

clang/lib/Sema/SemaLambda.cpp

+  // Temporarily set the lexical declaration context to the current
+  // context, so that the Scope stack matches the lexical nesting.
  Method->setLexicalDeclContext(CurContext);


I recovered this comment from https://reviews.llvm.org/D124351 because at least it tries to explain something - better than nothing that just makes the lambda parsing dimmer.

cor3ntin · 2024-08-05T08:54:13Z

I might be stupid but i still do not get why fold expressions are special, they should not be.
Can you add tests for

void f(auto...);
template <class = void> void foo() {
 []<class... Is>() {
    f([]() requires (!C<Is>) {}()...);
  }.template operator()<char, int, float>();
}

other interesting expansions might be

f<[]() requires (!C<Is>) {}()...>();
//
f({+[]() requires (!C<Is>) {}()...});

// I am truly sorry.
[ ...fs = +[]() requires (!C<Is>) {}()]() {
  (fs(),...);
}

zyn0217 · 2024-08-05T12:31:08Z

I might be stupid but i still do not get why fold expressions are special, they should not be.

Sorry I was inaccurate suggesting fold expressions are special, it's actually constraints involving pack expansions who are special. (I admit inventing another collectUnexpandedParameterPacks overload for CXXFoldExpr is odd, probably insufficent for the solution because we seem to have more problems - as I observed a similar issue from your first example as well - regarding pack expansion in terms of the constraints on lambdas.)

void f(auto...);
template <class = void> void foo() {
[]<class... Is>() {
   f([]() requires (!C<Is>) {}()...);
 }.template operator()<char, int, float>();
}

This is awkward: no compiler currently compiles it. (https://gcc.godbolt.org/z/rce3rh7jK) And spuriously, both gcc and clang think that there are no unexpanded parameter packs for expansion.

Regardless of whether this is conforming, the diagnostic here is simple to explain: we didn't preserve the ContainsUnexpandedParameterPack flag while transforming the LambdaExpr, so as an intuitive solution, we could just retain the flag from TransformLambdaExpr:

(Though I didn't add the following to the patch yet, it doesn't solve the problem and instead would cause a crash)

Expr *TrailingRequires = E->getCallOperator()->getTrailingRequiresClause();
  if (TrailingRequires)
    LSI->ContainsUnexpandedParameterPack |=
        TrailingRequires->containsUnexpandedParameterPack();

We still fail to evaluate the constraint because it ends up being dependent even with proper template arguments. This is because the expression is not expanded anywhere before evaluating the constraint, which is one step when we forming up a CallExpr.

The situation could be slightly different (or rather, better?) if a fold expression gets involved. We would still have a chance to expand the fold expression into an expanded but unsubstituted state. So we can make them into the constraint evaluation and substitute arguments there, and get a correct result. So this is why it's convoluted: we probably miss out on some passes for constraint packs: Maybe we need to expand them somewhere before evaluation, but I'm not completely clear.

f<[]() requires (!C<Is>) {}()...>();
//
f({+[]() requires (!C<Is>) {}()...});
// I am truly sorry.
[ ...fs = +[]() requires (!C<Is>) {}()]() {
 (fs(),...);
}

(I promise I won't forget them but let's consider them when we get around to the constraints in the next patch. :)

cor3ntin · 2024-08-05T12:39:02Z

Okay so we seem to agree. Can you go over the pr and make sure "fold expression" says "pack expansion" everywhere that it should (release note, comments, function names) ? Thanks!

cor3ntin

LGTM (modulo nit)
We should probably backport that

clang/test/SemaCXX/fold_lambda_with_variadics.cpp

Co-authored-by: cor3ntin <[email protected]>

zyn0217 · 2024-08-06T04:57:13Z

Thanks for the review! I've just merged it.

We should probably backport that

SGTM but given that this bug persists in so many versions, I think it is also fine to leave the fix just on trunk - especially since this is actually a partial fix.

We could issue a backport to 19 after we have addressed remaining FIXMEs i.e. bugs on attributes/constraints/captures, if I can catch up the release schedule. WDYT?

cor3ntin · 2024-08-06T07:49:46Z

r3 is in 2 weeks and we have to be more conservatives in our backports, so it really depends on how long it took you and @ilya-biryukov to address these issues and the complexity of the patches.

In particular, I would be surprised if the attribute case impacts many people.

ilya-biryukov · 2024-08-06T09:39:32Z

r3 is in 2 weeks and we have to be more conservatives in our backports, so it really depends on how long it took you and @ilya-biryukov to address these issues and the complexity of the patches.

In particular, I would be surprised if the attribute case impacts many people.

To be clear, I don't have a real-world use-case for attributes. The only reason I have added it in my patch is because there was a test in Clang that started failing with my approach (because it started looking at thing structurally even on the parsing path, as discussed earlier).
I suspect we could live with that for a long time, although fixing that eventually is a good idea. (And is still in my TODO list, just slipped a little)

…ints (#109518) (This continues the effort of #86265, fixing another piece of issue in constraint evaluation on variadic lambdas.) We need the depth of the primary template parameters for constraint substitution. To that end, we avoided substituting type constraints by copying the constraint expression when instantiating a template. This, however, has left an issue in that for lambda's parameters, they can reference outer template packs that would be expanded in the process of an instantiation, where these parameters would make their way into the constraint evaluation, wherein we have no other way to expand them later in evaluation. For example, template <class... Ts> void foo() { bar([](C<Ts> auto value) {}...); } The lambda references a pack `Ts` that should be expanded when instantiating `foo()`. The `Ts` along with the constraint expression would not be transformed until constraint evaluation, and at that point, we would have no chance to expand `Ts` anyhow. This patch takes an approach that transforms `Ts` from an unexpanded TemplateTypeParmType into a SubstTemplateTypeParmType with the current pack substitution index, such that we could use that to expand the type during evaluation. Fixes #101754

…ints (llvm#109518) (This continues the effort of llvm#86265, fixing another piece of issue in constraint evaluation on variadic lambdas.) We need the depth of the primary template parameters for constraint substitution. To that end, we avoided substituting type constraints by copying the constraint expression when instantiating a template. This, however, has left an issue in that for lambda's parameters, they can reference outer template packs that would be expanded in the process of an instantiation, where these parameters would make their way into the constraint evaluation, wherein we have no other way to expand them later in evaluation. For example, template <class... Ts> void foo() { bar([](C<Ts> auto value) {}...); } The lambda references a pack `Ts` that should be expanded when instantiating `foo()`. The `Ts` along with the constraint expression would not be transformed until constraint evaluation, and at that point, we would have no chance to expand `Ts` anyhow. This patch takes an approach that transforms `Ts` from an unexpanded TemplateTypeParmType into a SubstTemplateTypeParmType with the current pack substitution index, such that we could use that to expand the type during evaluation. Fixes llvm#101754

zyn0217 added 3 commits March 22, 2024 18:15

[Sema] Preserve ContainsUnexpandedParameterPack in TransformLambdaExpr

6e7b38b

Consider the captured pack variables - gh18873

e76fedd

Merge branch 'main' into GH85667

cc74f6d

comments

faf5d87

This was referenced Mar 23, 2024

Nested Variadic Expansions in generic lambdas don't work #18873

Open

[clang][Sema] Ignore the parentheses in the guard of DiagnoseUnexpandedParameterPack #86401

Closed

shafik requested a review from cor3ntin March 29, 2024 04:12

zyn0217 added 7 commits May 1, 2024 23:35

format

c5765ec

Merge branch 'main' into GH85667

efb1e72

Retain the 'unexpanded' Decls

6c2d311

Update ReleaseNotes

1ac5ca7

Cleanup & silence the unused-expression warning

db36bc8

Fix the CI

66acffb

Fix typos

5285c10

This was referenced May 24, 2024

Clang crashes with nested std::apply() calls #93256

Closed

[Clang] Crash on diagnose_if attribute if first argument is an unexpanded pack #93269

Closed

zyn0217 mentioned this pull request Jul 11, 2024

[Clang] Correctly recognize unexpanded packs in lambda template params #98496

Merged

zyn0217 added 4 commits July 17, 2024 15:03

Merge branch 'main' into GH85667

b336807

Fixup

b2d2836

Fix release notes

26872a6

clarify a bit more

10b9d8b

zyn0217 marked this pull request as ready for review July 18, 2024 08:43

zyn0217 requested a review from Endilll as a code owner July 18, 2024 08:43

zyn0217 added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jul 18, 2024

zyn0217 commented Jul 18, 2024

View reviewed changes

clang/test/SemaTemplate/lambda-capture-pack.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/TreeTransform.h Outdated Show resolved Hide resolved

zyn0217 added 5 commits July 30, 2024 16:34

Use CurContext

ea94d6d

Remove the fixes for constraints

412bf5e

Remove extra blanks

de76522

Remove changes for constraints in TransformLambdaExpr

930e8f9

Simplify the RebuildLambdaExpr()

9ce3250

zyn0217 commented Aug 1, 2024

View reviewed changes

zyn0217 mentioned this pull request Aug 3, 2024

[clang] [frontend] crash when instantiating a parameter pack in a lambda constraint #101754

Closed

zyn0217 added 3 commits August 5, 2024 22:19

Merge branch 'main' into GH85667

0379733

Remove most self-explanatory comments

e74a968

Remove one more gratuitous comment

27bd832

cor3ntin approved these changes Aug 5, 2024

View reviewed changes

clang/test/SemaCXX/fold_lambda_with_variadics.cpp Outdated Show resolved Hide resolved

zyn0217 and others added 2 commits August 5, 2024 22:48

FIXME

ac89937

Co-authored-by: cor3ntin <[email protected]>

Fix CI

301037e

zyn0217 merged commit 874067a into llvm:main Aug 6, 2024
8 checks passed

zyn0217 deleted the GH85667 branch August 6, 2024 04:57

zyn0217 mentioned this pull request Sep 10, 2024

[Clang] Introduce FunctionParmPackDecl for expanded lambda captures #107995

Draft

zyn0217 mentioned this pull request Sep 21, 2024

[Clang][Sema] Retain the expanding index for unevaluated type constraints #109518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Sema] Preserve ContainsUnexpandedParameterPack in TransformLambdaExpr #86265

[Sema] Preserve ContainsUnexpandedParameterPack in TransformLambdaExpr #86265

zyn0217 commented Mar 22, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Mar 23, 2024

Uh oh!

github-actions bot commented Mar 23, 2024 •

edited

Loading

Uh oh!

zyn0217 commented Jul 18, 2024

Uh oh!

cor3ntin commented Jul 18, 2024

Uh oh!

zyn0217 commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

zyn0217 Aug 1, 2024

Uh oh!

cor3ntin commented Aug 5, 2024 •

edited

Loading

Uh oh!

zyn0217 commented Aug 5, 2024

Uh oh!

cor3ntin commented Aug 5, 2024

Uh oh!

cor3ntin left a comment

Uh oh!

Uh oh!

Uh oh!

zyn0217 commented Aug 6, 2024

Uh oh!

cor3ntin commented Aug 6, 2024

Uh oh!

ilya-biryukov commented Aug 6, 2024

Uh oh!

Uh oh!

[Sema] Preserve ContainsUnexpandedParameterPack in TransformLambdaExpr #86265

[Sema] Preserve ContainsUnexpandedParameterPack in TransformLambdaExpr #86265

Conversation

zyn0217 commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 23, 2024

Uh oh!

github-actions bot commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyn0217 commented Jul 18, 2024

Uh oh!

cor3ntin commented Jul 18, 2024

Uh oh!

zyn0217 commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

zyn0217 Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyn0217 commented Aug 5, 2024

Uh oh!

cor3ntin commented Aug 5, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zyn0217 commented Aug 6, 2024

Uh oh!

cor3ntin commented Aug 6, 2024

Uh oh!

ilya-biryukov commented Aug 6, 2024

Uh oh!

Uh oh!

zyn0217 commented Mar 22, 2024 •

edited

Loading

github-actions bot commented Mar 23, 2024 •

edited

Loading

cor3ntin commented Aug 5, 2024 •

edited

Loading