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] [dart2wasm] Fix bug in setter type checker argument checking implementation #56374

Closed
Tracked by #152953
mkustermann opened this issue Aug 5, 2024 · 6 comments
Closed
Tracked by #152953

[CP] [dart2wasm] Fix bug in setter type checker argument checking implementation #56374

mkustermann opened this issue Aug 5, 2024 · 6 comments
Assignees
@vsmenon @itsjustkevin @mit-mit @athomas
Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@mkustermann
Copy link
Member

Commit(s) to merge

2cf3222

Target

stable

Prepared changelist for beta/stable

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

Issue Description

Flutter code that uses dynamic calls where the dynamic call targets a setter with a generic argument will (in -O3 / -O4 mode - the ladder which is flutter builds default) sets null instead of the actual value.

What is the fix

Make compiler in -O3 / -O4 mode correctly set the field value in the implicit setter.

Why cherry-pick

Flutter users have hit this.

Risk

If tests pass: low

Issue link(s)

flutter/flutter#152029

Extra Info

No response
@mkustermann mkustermann added the cherry-pick-review Issue that need cherry pick triage to approve label Aug 5, 2024
@dart-github-bot
Copy link
Collaborator

Summary: In -O3 and -O4 optimization modes, Dart2Wasm incorrectly sets null instead of the actual value when a dynamic call targets a setter with a generic argument. This fix ensures the compiler correctly sets the field value in the implicit setter.

@dart-github-bot dart-github-bot added area-dart2wasm Issues for the dart2wasm compiler. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Aug 5, 2024
@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Aug 5, 2024
@lrhn lrhn changed the title [CP] [dart2wasm] Fix bug in seter type checker argument checking implementation [CP] [dart2wasm] Fix bug in setter type checker argument checking implementation Aug 5, 2024
@mkustermann mkustermann mentioned this issue Aug 5, 2024
Closed
@mkustermann
Copy link
Member Author

/cc @athomas I'd suggest we keep our origin/stable free of bugs and land this. We can then talk to flutter team whether they're willing to pick a new dart hash or not for their stable.

@vsmenon
Copy link
Member

vsmenon commented Aug 8, 2024

lgtm

@kevmoo kevmoo mentioned this issue Aug 9, 2024
Closed
7 tasks
@mkustermann
Copy link
Member Author

@itsjustkevin any chance we could get this approved & landed to be included in time for upcoming dot release?

@itsjustkevin itsjustkevin added the cherry-pick-approved Label for approved cherrypick request label Aug 12, 2024
@itsjustkevin
Copy link
Contributor

@mkustermann approved. Please write a changelog update pointing back to this issue to satisfy the submit requirements.

copybara-service bot pushed a commit that referenced this issue Aug 12, 2024
@mkustermann
[stable] [dart2wasm] Fix bug in setter type checker argument checking…

Verified

This commit was signed with the committer’s verified signature.
Byron Sebastian Thiel
GPG key ID: 9CB5EE7895E8268B
Verified
Learn about vigilant mode
Loading
Loading status checks…
475c337 
… implementation

The CL in [0] introduced a regression: By not performing the type
checking code in -O3/-O4 mode it didn't have the side-effect of the
type checking code anymore (setting the `argLocal`) which caused the setter to store `null` instead of the actual value.

[0] https://dart-review.googlesource.com/c/sdk/+/370280

Fixes flutter/flutter#152029

TEST=web/wasm/flutter_regress_152029_test

Bug: flutter/flutter#152029
Change-Id: Icea1ac478ff020e2056c4c7011aaea7a6222ec32
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/378800
Cherry-pick-request: #56374
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/378860
Reviewed-by: Ömer Ağacan <omersa@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
@mkustermann
Copy link
Member Author

@mkustermann approved. Please write a changelog update pointing back to this issue to satisfy the submit requirements.

The CL landed

@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@vsmenon @lrhn @mkustermann @itsjustkevin @mit-mit @athomas @dart-github-bot