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.

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

[CP] DDC support for record subtype checks with web numeric semantics #52531

Closed
Markzipan opened this issue May 26, 2023 · 5 comments
Closed
Assignees
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-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable web-dev-compiler

Comments

@Markzipan
Copy link
Contributor

Markzipan commented May 26, 2023

Commit(s) to merge

44b5ca7
8dfd25a

Target

stable

Prepared changelist for beta/stable

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

Issue Description

The subtype check implementation for records in DDC did not respect web semantics for numerics (int being a subtype of double)

What is the fix

Update DDC's subtype check implementation to dispatch on the subtype checks for each of its fields.

Why cherry-pick

This pattern can be triggered extremely easily in DDC. E.g.:

void main() {
  var recordList = <(double, String)>[];
  recordList.add((1.0, 'hello')); // crashes but should be allowed
}

The issue has a high impact potential due records being widely advertised in the Dart 3.0 release.

Risk

low

Issue link(s)

#52480

Extra Info

This contains cherry-picks for the CLs in this chain. The latter just adds a regression test. Additionally, this change diverges very slightly from main in RecordType's as check. For a clean merge, RecordImpl (in main) is renamed to _RecordImpl.

@Markzipan Markzipan added web-dev-compiler area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-review Issue that need cherry pick triage to approve labels May 26, 2023
@Markzipan
Copy link
Contributor Author

@nshahan @sigmundch for context

@itsjustkevin
Copy link
Contributor

itsjustkevin commented May 30, 2023

Hey @Markzipan, I noticed that your request points to the CL that landed on main. To continue triaging this cherry-pick request, we would need that field to point to the cherry-pick to the stable channel.

For further instructions, please refer to: https://github.com/dart-lang/sdk/wiki/Cherry-picks-to-a-release-channel.

Thank you.

@itsjustkevin
Copy link
Contributor

@sigmundch and @nshahan could you take a look at this cherry-pick request?

@Markzipan
Copy link
Contributor Author

Thanks Kevin - updated the link

@sigmundch
Copy link
Member

I'm very supportive of this cherry-pick. It's low risk but highly visible problem.

@itsjustkevin itsjustkevin added cherry-pick-approved Label for approved cherrypick request merge-to-stable labels May 31, 2023
@sigmundch sigmundch added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label May 31, 2023
@itsjustkevin itsjustkevin reopened this Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable web-dev-compiler
Projects
None yet
Development

No branches or pull requests

6 participants