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

Regression in allowInterop idempotence in DDC in Dart >=3.3.0 <3.5.0 #56897

Closed
greglittlefield-wf opened this issue Oct 14, 2024 · 5 comments
Closed
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler web-js-interop Issues that impact all js interop

Comments

@greglittlefield-wf
Copy link
Contributor

I noticed that a regression of #38311 was introduced in Dart 3.3.0, and was fixed in Dart 3.5.0.

The broken behavior is the same as described in that previous issue, and also is what's advertised in allowInterop's doc comment:

Calling this method repeatedly on a function will return the same result.

Using same reduced test case from that issue...

function() {}
final once = allowInterop(function);
final twice = allowInterop(once);
print(identical(once, twice)); // prints false

...I get the following results in these arbitrary Dart SDK versions that I tested:

  • prints true (good behavior)
    • 2.19.6, 3.0.0, 3.2.0, 3.2.6, 3.5.0, 3.5.3, 3.6.0-334.0.dev
  • prints false (bad behavior)
    • 3.3.0, 3.4.4

Even though the issue was fixed, it wasn't clear to me what exactly fixed the issue, whether it was fixed on purpose, or whether test coverage was added for this case.

And it looks like when the original issue was fixed (in this commit), no tests were added, so I'm worried that this issue might not have test coverage and could regress again 😕.

I noticed a potentially related changelog entry in the 3.5.0 release notes:

However, the linked issue that was resolved and the commit fixing it seemed more related to .toJS than allowInterop.


If it's helpful, I can also provide a little more context on how I'm currently relying on this behavior.

Right now, I have some code utilizing package:js interop and passing arbitrary values to JS code.

Some of those values are meant for consumption by JS code, and others (including Dart objects, and notably Dart functions) are meant only for consumption by Dart code reading them back from JS at a later point, and are fine to be treated as opaque to JS.

Values that have already been wrapped in allowInterop are assumed to be meant for consumption by JS code, and need to be passed through as-is, whereas Dart functions need to be wrapped in another object so they don't throw when passed over to JS. Without this, the assert in dart:js_util's setProperty fails with the message: Dart function requires allowInterop to be passed to JavaScript..

The utility that conditionally wraps functions lives here, and uses the idempotent behavior of allowInterop to test whether a function has already been wrapped in allowInterop.

I anticipate that this dynamic, conditional wrapping of functions may no longer be possible after switching to the latest Dart JS interop, and we'll need to refactor the code that prepares those values. But, for the time being, we rely on that behavior.

If there are any less-hacky ways of testing whether a function can be called from JS, I'm all ears 😄.

@dart-github-bot
Copy link
Collaborator

Summary: The issue reports a regression in the idempotence of allowInterop in Dart Dev Compiler (DDC) between Dart versions 3.3.0 and 3.5.0. The issue was previously fixed in Dart 3.5.0, but the fix lacked test coverage, raising concerns about potential future regressions.

@dart-github-bot dart-github-bot added area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 14, 2024
@lrhn lrhn added web-js-interop Issues that impact all js interop web-dev-compiler and removed triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. labels Oct 14, 2024
@srujzs srujzs self-assigned this Oct 15, 2024
@srujzs
Copy link
Contributor

srujzs commented Oct 15, 2024

cc @nshahan

Thanks for narrowing down the versions! I suspect it's the general RTI changes that accidentally regressed this and it's this CL that fixed it: 25a9fc0. The correct answer is indeed to test this so it doesn't regress again - I can take that on. Apologies for the temporary regression.

You're correct that that changelog is specific to toJS. The above CL was added likely to address the same issue for toJS since the mechanisms are very similar. We should still test allowInterop and allowInteropCaptureThis specifically, but there are tests for toJS and toJSCaptureThis for this.

If there are any less-hacky ways of testing whether a function can be called from JS, I'm all ears 😄.

This is an interesting question. There's a general necessity for differentiating between a JS and a Dart value. In some cases, you simply can't tell (Dart Strings compiled to JS and JS strings are identical). In some cases, you can tell, but the representation is the same (Dart Functions are JS functions in DDC). is JSObject (from dart:js_interop) can differentiate the two types of functions, but it isn't quite clear if that's intentional or not. We've also added a lint to warn against doing that since it may not be platform-consistent. Ideally, it'd be nice to have some API that tells you if this is a JS value, and then you can cast to JSAny and then do an isA.

@greglittlefield-wf
Copy link
Contributor Author

@srujzs Awesome, thank you for looking into that, and for discussing my use-case!

Gotcha, so it sounds like there may not be a stable way to do it with either dart:js_util or with the dart:js_interop.

Ideally, it'd be nice to have some API that tells you if this is a JS value, and then you can cast to JSAny and then do an isA.

Yes, that sounds like it would be useful! That way, I could also conditionally pass through any JSAny values, and convert or box other values with .toJSBox.

@srujzs
Copy link
Contributor

srujzs commented Oct 16, 2024

I've filed #56905 to track the request for that API. I have a change that adds test cases for idempotency in review, and after that lands, I'll mark this as closed.

@greglittlefield-wf
Copy link
Contributor Author

@srujzs Thank you very much!!

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. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dev-compiler web-js-interop Issues that impact all js interop
Projects
None yet
Development

No branches or pull requests

4 participants