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] Check dart:ffi imports and wasm:import/export pragmas in user code #55890

Closed
osa1 opened this issue May 31, 2024 · 9 comments
Closed

[CP] Check dart:ffi imports and wasm:import/export pragmas in user code #55890

osa1 opened this issue May 31, 2024 · 9 comments
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve

Comments

@osa1
Copy link
Member

osa1 commented May 31, 2024

Commit(s) to merge

dbcf23a
35bc17a

Target

stable

Prepared changelist for beta/stable

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

Issue Description

dart2wasm currently allows importing dart:ffi (to use @Native) and using wasm:import and wasm:export pragmas.

However none of these were designed for end users. We have no documentation on how to properly use them, and they can cause compile time or runtime crashes.

What is the fix

Disallow importing dart:ffi and using wasm:import and wasm:export pragmas.

Why cherry-pick

These features were never intended for users. They will cause confusion.

Risk

None, however we should make sure the checks allow uses of these features in Flutter.

Issue link(s)

#55744

Extra Info

No response

@osa1 osa1 added cherry-pick-review Issue that need cherry pick triage to approve area-dart2wasm Issues for the dart2wasm compiler. labels May 31, 2024
@osa1
Copy link
Member Author

osa1 commented May 31, 2024

None, however we should make sure the checks allow uses of these features in Flutter.

@mkustermann do you know how to test this?

@mkustermann
Copy link
Member

mkustermann commented May 31, 2024

None, however we should make sure the checks allow uses of these features in Flutter.

@mkustermann do you know how to test this?

If the actual commits on main have rolled up to flutter/engine and caused no issues, then we have sufficient proof to also do those changes as cherry-picks on stable (we can monitor here and here to see whether they landed in engine)

@mkustermann
Copy link
Member

LGTM

@athomas athomas added the cherry-pick-approved Label for approved cherrypick request label Jun 3, 2024
@mkustermann
Copy link
Member

35bc17a was reverted on main due to flutter engine tests using wasm import/export annotations in package:ui (which is actually dart:ui), we'll only cherry-pick dbcf23a (which is what users are mostly hitting e.g. #53910).

As @osa1 is OOO I've updated cl/369040 which we can then merge.

copybara-service bot pushed a commit that referenced this issue Jun 4, 2024
…agmas in user code

- Disallow importing `dart:ffi` in user code.
- Disallow `wasm:import` and `wasm:export` pragmas in user code.

Bug: #53910

Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/369040
Cherry-pick-request: #55890
Change-Id: Ifaaeb4f2c4b13bd5f564ffbf5281d6439ac6dd56
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/369040
Reviewed-by: Alexander Thomas <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
@mkustermann
Copy link
Member

The CP was merged now.

@magic-akari
Copy link

I am using wasm:export, please check
https://github.com/wasm-fmt/dart_fmt/blob/a90b6eb6929f85c099e97c0ef12ab61c81d889dc/lib/dart_fmt.dart#L20

I am not using Flutter, I am just porting dart to wasm.
Is there any migration guidance after wasm:export is disabled?

@mkustermann
Copy link
Member

mkustermann commented Aug 9, 2024

I am not using Flutter, I am just porting dart to wasm.
Is there any migration guidance after wasm:export is disabled?

Some background:

  • Right now dart2wasm compiled apps can only be used in a JavaScript environment, not in pure wasm environments.
  • There are some JavaScript environments that don't even support wasm.
  • The fast development mode for web is using DDC (dart2wasm & dart2js are whole-program optimizing compilers that should be used to deploy but not develop, as they take too long to run during development).

Because of all these reasons we strive to have solutions that work across DDC (our development tool), Dart2JS (our deployment compiler for all JS environments) and Dart2Wasm (for JS environments that support it).

=> The recommended strategy to expose APIs is to use JS interop.
=> See an example in my comment here on how to expose Dart to the JavaScript environment

@magic-akari let me know if that works for you.

@magic-akari
Copy link

@magic-akari let me know if that works for you.

Thank you for your reply. @mkustermann
I have resolved the issue through the following method.

@JS()
external set foo(JSFunction handler);

void main(List<String> arguments) {
  foo = fooWrapper.toJS;
}

https://github.com/wasm-fmt/dart_fmt/blob/3bf39269adba7a56305a422c4fc2582f3963aa8a/lib/binding.dart#L24-L29

However, there are still some issues:

The exported function is attached to globalThis instead of the exports of the wasm instance, a practice that should be strictly avoided in JavaScript development.

So, I made some modifications to the output JS file, changing globalThis.foo = value to foo = value, and added export let foo; at the end of the file.
https://github.com/wasm-fmt/dart_fmt/blob/3bf39269adba7a56305a422c4fc2582f3963aa8a/scripts/patch.mjs#L10-L11

To follow best practices, I hope to automate this process. Alternatively, attaching it to the exports of wasm is also a good choice.

@mkustermann
Copy link
Member

mkustermann commented Aug 9, 2024

The exported function is attached to globalThis instead of the exports of the wasm instance, a practice that should be strictly avoided in JavaScript development.

One can install the exported API functions on any JavaScript object, not just on globalThis.

Alternatively, attaching it to the exports of wasm is also a good choice.

We'd like to make dart apps work in DDC, dart2wasm & dart2js - so we'd want to avoid a dart2wasm specific solution. (Maybe in the future we extend the scope to make dart2wasm apps run in pure wasm environments and have a development mode for dart2wasm, but there's much longer term)

Hope you got unblocked now. I'm going to lock this issue as it's about a cherry-pick and that has landed.

@dart-lang dart-lang locked as resolved and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-dart2wasm Issues for the dart2wasm compiler. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve
Projects
None yet
Development

No branches or pull requests

7 participants