Skip to content
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

Jump to bottom

[CP] Fix Dart2JS crash on record pattern outside function body #53449

Closed
biggs0125 opened this issue Sep 6, 2023 · 2 comments
Closed

[CP] Fix Dart2JS crash on record pattern outside function body #53449

biggs0125 opened this issue Sep 6, 2023 · 2 comments
Assignees
@vsmenon @whesse @itsjustkevin @mit-mit @athomas
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@biggs0125
Copy link

Commit(s) to merge

https://dart-review.googlesource.com/c/sdk/+/323020

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/324600

Issue Description

Dart2JS was crashing when a record pattern was used outside the scope of a function body. This could happen in constructor initializers or field initializers.

The crash was due to the CFE's lowering of these patterns to statements containing late local variables. Dart2JS's late lowering assumed that late locals would always be within function bodies and this was true prior to the introduction of the CFE's record pattern lowering.

What is the fix

The fix is to allow for late locals within more contexts. This just means setting up a scope context for them around field initializers and around constuctors.

Why cherry-pick

Dart2JS crashes on valid inputs. The fix is simple enough to be low risk.

Risk

low

Issue link(s)

#53358

Extra Info

No response
@biggs0125 biggs0125 added the cherry-pick-review Issue that need cherry pick triage to approve label Sep 6, 2023
@devoncarew devoncarew added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Sep 6, 2023
@sigmundch
Copy link
Member

I'm supportive of this cherry-pick

It fixes a crasher in dart2js, while the presence of the issue may be rare, in my opinion the low risk makes the cherry-pick worthwhile.

@itsjustkevin itsjustkevin added merge-to-stable cherry-pick-approved Label for approved cherrypick request labels Sep 6, 2023
@itsjustkevin
Copy link
Contributor

Approved, @natebiggs merge when ready.

copybara-service bot pushed a commit that referenced this issue Sep 6, 2023
@natebiggs
[stable][dart2js] Fix late local lowering with record destructuring.
d5a6568 
The CFE's lowering for record patterns can sometimes make use of late local fields. Dart2JS's late lowerer assumes that all late local fields live in the scope of some function body. Record patterns allow late locals to be created in other contexts though. Some examples that break this assumption include field initializers and expressions in constructor initializer lists.

To fix this we add a late local scope in some extra contexts where these record patterns can show up.

Fixes: 53358

Bug: #53358
Change-Id: Ic730f41253782241b3394c0b0353d1731e122c85
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/323020
Cherry-pick-request: #53449
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324600
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
@sortie sortie closed this as completed Sep 7, 2023
@sigmundch sigmundch mentioned this issue Sep 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@whesse whesse

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@devoncarew @sigmundch @vsmenon @biggs0125 @sortie @whesse @itsjustkevin @mit-mit @athomas