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

[dart2wasm] Compiler ignores library annotation for unwrapped functions #55359

Closed
ykmnkmi opened this issue Apr 3, 2024 · 6 comments
Closed
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler. web-js-interop Issues that impact all js interop

Comments

@ykmnkmi
Copy link
Contributor

ykmnkmi commented Apr 3, 2024

@JS('__test__')
library;

import 'dart:js_interop';

@JS('__log__')
external T log1<T extends JSObject>(T object);

@JS('__log__')
external T _log<T extends JSObject>(T object);

T log2<T extends JSObject>(T object) {
  return _log<T>(object);
}
const dart2wasm = {
  // log1(), incorect
  _123: x0 => globalThis.__log__(x0),
  // log2(), correct
  _13050: x0 => globalThis.__test__.__log__(x0)
};

Dart SDK version: 3.5.0-edge.8ca97716cc5f1be89eebd5a79a8f0daa541fd55c (main) on "windows_x64".

@lrhn lrhn added area-dart2wasm Issues for the dart2wasm compiler. web-js-interop Issues that impact all js interop labels Apr 3, 2024
@osa1
Copy link
Member

osa1 commented Apr 3, 2024

I don't think the repro is right, could you provide a full example with repro commands?

In your example _log and log1 are identical other than the function name so they can't generate different imports in the .mjs file.

log2 won't generate an import as it's not annotated with @JS, so the comments in your .mjs snippet are not right.

@ykmnkmi
Copy link
Contributor Author

ykmnkmi commented Apr 3, 2024

@osa1 I still don't know when it happens, this repro works right now:

// web/main.dart
import 'dart:js_interop';

import 'package:app/a.dart';

import 'test.dart';

void main() {
  print(log1(JSObject()));
  print(log2(JSObject()));
}
// lib/a.dart
export 'package:app/src/a.dart';
// lib/src/a.dart
@JS()
library;

import 'dart:js_interop';

For bigger example try to remove wrappers here:

wasm.mp4

(video link fix)

@srujzs
Copy link
Contributor

srujzs commented Apr 4, 2024

I think this is related to how we cache method calls in our interop specializers, which might be why you see the issue appear when you remove a call, because that may lead to slightly different interop generation. I also think this code might be contributing to why you're seeing the missing prefix: we use the current library's annotation prefix instead of the interop member's enclosing library's annotation prefix. This shouldn't be an issue due to how we generate procedures, but there's probably a bug somewhere. I'll do some more digging. Can you share the full contents of test.dart? Is it just the above interop code?

@srujzs srujzs self-assigned this Apr 4, 2024
@ykmnkmi
Copy link
Contributor Author

ykmnkmi commented Apr 4, 2024

@srujzs yes, first example.

@srujzs
Copy link
Contributor

srujzs commented Apr 4, 2024

https://dart-review.googlesource.com/c/sdk/+/361241 should fix this:

const dart2wasm = {
_261: x0 => globalThis.__test__.__log__(x0),
_262: x0 => globalThis.__test__.__log__(x0)
}

@ykmnkmi
Copy link
Contributor Author

ykmnkmi commented Apr 5, 2024

@srujzs thanks.

copybara-service bot pushed a commit that referenced this issue Apr 16, 2024
Closes #55359

The current library's annotation is used for the interop lowerings
in dart2wasm. For most members, this is okay because we emit the
equivalent JS code for the member when we visit the procedure and
not when we visit the invocation. However, for methods, the invocation
determines the resulting JS call due to the existence of optional
parameters. In that case, if the invocation was not in the same
library as the interop member declaration, it results in using the
wrong library's annotation value.

Adds tests for this case and does some cleanup of existing tests.

Specifically:
- Adds a consistent naming scheme for test libraries that are
namespaced.
- Adds code to delete the non-namespaced declarations so that the
namespaced interop methods don't accidentally call those declarations.
- Removes differentArgsMethod which was already tested in
js_default_test.
- Removes use of js_util in favor of js_interop_unsafe.

Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/361241
Cherry-pick-request: #55430
Bug: #55359
Change-Id: Ibf7125fea6da7722549f3c87aafdf881132b104f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362195
Commit-Queue: Srujan Gaddam <srujzs@google.com>
Reviewed-by: Kevin Chisholm <kevinjchisholm@google.com>
copybara-service bot pushed a commit that referenced this issue Apr 16, 2024
…ings

Closes #55359

The current library's annotation is used for the interop lowerings
in dart2wasm. For most members, this is okay because we emit the
equivalent JS code for the member when we visit the procedure and
not when we visit the invocation. However, for methods, the invocation
determines the resulting JS call due to the existence of optional
parameters. In that case, if the invocation was not in the same
library as the interop member declaration, it results in using the
wrong library's annotation value.

Adds tests for this case and does some cleanup of existing tests.

Specifically:
- Adds a consistent naming scheme for test libraries that are
namespaced.
- Adds code to delete the non-namespaced declarations so that the
namespaced interop methods don't accidentally call those declarations.
- Removes differentArgsMethod which was already tested in
js_default_test.
- Removes use of js_util in favor of js_interop_unsafe.

Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/361241
Cherry-pick-request: #55430
Change-Id: I37661ed200c6db367e3f29f50b0877834f4c1639
Bug: #55359
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362190
Reviewed-by: Kevin Chisholm <kevinjchisholm@google.com>
Commit-Queue: Srujan Gaddam <srujzs@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler. web-js-interop Issues that impact all js interop
Projects
Status: Done
Development

No branches or pull requests

4 participants