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] Erase static interop type in static invocation #53579

Closed
srujzs opened this issue Sep 20, 2023 · 3 comments
Closed

[CP] Erase static interop type in static invocation #53579

srujzs opened this issue Sep 20, 2023 · 3 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 web-dart2js web-js-interop Issues that impact all js interop

Comments

@srujzs
Copy link
Contributor

srujzs commented Sep 20, 2023 •
edited
Loading

Commit(s) to merge

b25873f

Target

stable

Prepared changelist for beta/stable

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

Issue Description

@anonymous @staticInterop classes using type parameters with factories crash on dart2js. An insufficient erasure led to a type mismatch in generated tear-offs that call these factories, which then triggered a crash. This is platform-agnostic but only materializes in dart2js.

What is the fix

The fix casts the invocation of the @staticInterop factory invocation to its erased type so that there isn't a type mismatch.

Why cherry-pick

This is an issue that is difficult to work around for users. Per my comment in #53479 (comment), users are forced to either move the factory to a static method, which won't be able to use class type parameters, use a top-level method, or get rid of class type parameters. All of these fixes will require breaking changes to users' packages to work around. Naturally, it's a breaking change again to go back to the old syntax once the issue is fixed. There is at least one existing package impacted by this: https://pub.dev/packages/record.

Risk

low

Issue link(s)

#53291 #53479 #53337

Extra Info

Note that the prepared changelist is slightly modified from the original CL. The only changes are in the test in that CL to support the fact that we didn't allow type parameters in extension members of @staticInterop classes (hence the Object? instead of T and U).
@srujzs srujzs added the cherry-pick-review Issue that need cherry pick triage to approve label Sep 20, 2023
@srujzs
Copy link
Contributor Author

srujzs commented Sep 20, 2023

cc @sigmundch

@srujzs srujzs added web-dart2js web-js-interop Issues that impact all js interop area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels Sep 20, 2023
@sigmundch
Copy link
Member

I'm very much in favor of cherry-picking to stable:

  • this fixes a crash and the issue has been noticed by the public 3 times so far (likely due to the indirect usage of pacakge:record in flutter apps)
  • As Srujan mention, the risk is low. In particular, the fix is simple and small, and the affected feature is limited too.

@itsjustkevin itsjustkevin added merge-to-stable cherry-pick-approved Label for approved cherrypick request labels Sep 22, 2023
copybara-service bot pushed a commit that referenced this issue Sep 22, 2023
@srujzs
[stable] [dart2js] Erase static interop type in static invocation
8f28795 
Static invocations of external factories are casted so that the
result, which is a @staticInterop type, can be treated as the erased
type instead. This CL fixes the issue where the type that it was
casted to was never replaced with the erased type.

Change-Id: Ic6eb529349ea2b5c42f91c2740d501d4f81bc38e
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/323505
Cherry-pick-request: #53579
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327120
Reviewed-by: Sigmund Cherem <[email protected]>
@srujzs
Copy link
Contributor Author

srujzs commented Sep 25, 2023

Closing out the issue as it's been landed.

@srujzs srujzs closed this as completed Sep 25, 2023
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 web-dart2js web-js-interop Issues that impact all js interop
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@sigmundch @vsmenon @whesse @itsjustkevin @mit-mit @athomas @srujzs