[CP] Use node's enclosing library annotation for lowerings #55430
Comments
srujzs
added
the
cherry-pick-review
|
LGTM! Minimal risk, but clearly fixing an issue with Firebase! |
|
Need to make sure this is merged into BETA, although getting it into the current stable is also nice |
|
Please open a beta CL also, we are currently prioritizing beta cherry-picks as we stabilize before the next quarterly release. |
|
Done! |
itsjustkevin
added
the
cherry-pick-approved
|
CC: @sigmundch |
devoncarew
added
the
area-web
|
I support this CP too, for both stable and beta. I expect we'll see more usage on dart2wasm after 3.4, so I'd especially want to include it in 3.4 beta before it goes out as stable. @srujzs - consider updating the description above to clarify which kind of JS interop member/methods are affected. I believe it's only top-level methods and possibly static class methods and properties? cc @mkustermann |
Updated. It affects any JS interop member that uses the enclosing library's JS annotation and could contain optional parameters - top-level methods, constructors, and static methods. |
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>
…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>
Commit(s) to merge
https://dart-review.googlesource.com/c/sdk/+/361241
Target
beta/stable
Prepared changelist for beta/stable
https://dart-review.googlesource.com/c/sdk/+/362195
https://dart-review.googlesource.com/c/sdk/+/362190
Issue Description
Some JS interop members rely on the enclosing library's
@JSannotation value to find the relevant JS member to dispatch to. In dart2wasm, the code to do so used the invocation's enclosing library's@JSannotation for this determination. This isn't an issue for many JS interop members because we replace the interop declaration for those members, but it is an issue for JS interop methods/constructors as we replace the interop invocation instead. This then leads to calling the wrong JS member because we failed to include the correct library annotation value, which may then lead to a crash due to a missing member.This is only an issue on dart2wasm. This affects the following JS interop members: top-level methods, constructors, and static methods.
What is the fix
Always use the declaration's enclosing library's annotation value and avoid caching the library's annotation value.
Why cherry-pick
JS interop code that may work fine when compiling to JS may now crash when compiling to Wasm, and may lead to a frustrating debugging experience.
This affected two users so far. It affected the original user who filed the issue: #55359. It also affected the I/O demo app that @kevmoo has been working on.
Risk
Low - this only affects the code to determine which JS interop member to use.
Issue link(s)
#55359
Extra Info
No response
The text was updated successfully, but these errors were encountered: