Skip to content

[breaking change] Change return type of unsignedRightShift #55238

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

Closed
srujzs opened this issue Mar 18, 2024 · 3 comments
Closed

[breaking change] Change return type of unsignedRightShift #55238

srujzs opened this issue Mar 18, 2024 · 3 comments
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop breaking-change-request This tracks requests for feedback on breaking changes closed-not-planned Closed as we don't intend to take action on the reported issue enhancement-breaking-change An enhancement which is breaking. type-enhancement A request for a change that isn't a bug web-js-interop Issues that impact all js interop

Comments

@srujzs
Copy link
Contributor

srujzs commented Mar 18, 2024

Intended Change

dart:js_interop's unsignedRightShift should now return an int instead of a JSNumber.

dart:js_interop exposes an unsignedRightShift operator to be consistent with the dart:js_util API. It currently returns a JSNumber, but is always an integer value. Therefore, it should be an int.

Justification

Number and boolean conversions are a no-op in the JS compilers and fast in dart2wasm. Since the common use case will be to convert the JSNumber to an int, we should offer an easier API for users so they don't have to convert the value with a .toDartInt call. This is also consistent with the other exposed operators which return bool when the operator always returns a boolean value.

Impact

Minimal. There are no usages in google3, in Flutter, in the SDK, or on GitHub.

Mitigation

Convert the resulting int to a JSNumber with toJS if you still need the JSNumber type e.g.

JSNumber n = 10.toJS.unsignedRightShift(1.toJS);
JSNumber n = 10.toJS.unsignedRightShift(1.toJS).toJS;
@srujzs srujzs added the breaking-change-request This tracks requests for feedback on breaking changes label Mar 18, 2024
@lrhn lrhn added web-js-interop Issues that impact all js interop type-enhancement A request for a change that isn't a bug area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop enhancement-breaking-change An enhancement which is breaking. breaking-change-request This tracks requests for feedback on breaking changes and removed breaking-change-request This tracks requests for feedback on breaking changes labels Mar 19, 2024
@lrhn
Copy link
Member

lrhn commented Mar 19, 2024

As you say, if it returns an int, then you'll need to convert it back to JS if you want to continue doing JS operations on it.

That'd be something like x.toJS.unsignedRightShift(8.toJS).toJS.bitwiseAnd(255.js).

On Wasm, converting back and forth is not free.
Would it make sense to also have a version which returns the same result as a JSNumber?
x.toJS.unsignedRightShiftJS(8.toJS).bitwiseAnd(255.js).

(Generally I'd prefer if the API kept values as JS values until the user requested a conversion to Dart value. Implicit conversion, especially if only in some cases, is harder to reason about. That all depends on the intended use-cases for the API. If we already do it for other members, it may be inconsistent to not do it here too.)

@srujzs
Copy link
Contributor Author

srujzs commented Mar 19, 2024

Agreed that this is dependent on several factors. Some considerations:

  1. Whether the value is a boolean/number or some other type. The former two, while not free in dart2wasm, are much faster. Of course, if the conversion is in a busy loop, the discrepancy grows.
  2. Whether the user can write a member themselves. This is part of why we were okay with making package:web use Dart primitive types instead of only JS types. Users can write package:web members themselves if they wanted JS types.
  3. How the member is commonly used. If users would almost always convert to a Dart type anyways e.g. instanceOfString to type-check, it makes sense to write the Dart type.
  4. Consistency with other similar members.

These operators were added to support the interop API surface that already existed, so that users could migrate away from dart:js_util for dart2wasm.

In this case:

  1. It returns a number.
  2. Users can't write these operators themselves today, which may mean we want to make them JS types.
  3. The usage of these operators are so low that we don't really have any data to pull from. They were added in dart:js_util to handle a specific use-case internally. I've seen maybe a handful of users use them externally.
  4. Other operators that always return a boolean value return bool in dart:js_interop, but we can certainly change that, and if we don't make this change, I'll want to change the other operators for consistency. Members like typeofEquals return bool, but those members are quite different from operators.

cc @mkustermann

Would it make sense to also have a version which returns the same result as a JSNumber?

I generally want to avoid two versions for the same declaration just because of how large the API surface can get. We did this for members like getProperty because they have higher usage and it makes sense to support both the ergonomic case and the performant case. There are a lot of operators, so I'd want to avoid this.

@srujzs
Copy link
Contributor Author

srujzs commented Mar 21, 2024

@lrhn I think you convinced me of making these operators use JS types everywhere. :)

If users didn't care about conversion costs with operators, they can use many of the Dart-equivalent operators anyways, so I suspect any use of these operators will be in cases where users would rather avoid the conversion cost. Furthermore, unlike typeofEquals or similar methods, I expect operators to compose each other e.g. b1.and(b2.or(b3)), so the conversions would add up.

So instead, I'm filing 55267 to break the two APIs that return a bool and closing this. Note that there are a number of other operators that return bool that aren't mentioned in that breaking change. These were already somewhat broken in 3.3 and therefore do not require a breaking change. See #55024.

Closing this as not intended. Let me know if I labeled this incorrectly.

@srujzs srujzs closed this as completed Mar 21, 2024
@github-project-automation github-project-automation bot moved this from In review to Complete in Breaking Changes Mar 21, 2024
@srujzs srujzs moved this from Complete to Deprecated in Breaking Changes Mar 21, 2024
@srujzs srujzs moved this from Deprecated to In review in Breaking Changes Mar 21, 2024
@srujzs srujzs added the closed-not-planned Closed as we don't intend to take action on the reported issue label Mar 21, 2024
@srujzs srujzs reopened this Mar 21, 2024
@srujzs srujzs closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2024
@github-project-automation github-project-automation bot moved this from In review to Complete in Breaking Changes Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop breaking-change-request This tracks requests for feedback on breaking changes closed-not-planned Closed as we don't intend to take action on the reported issue enhancement-breaking-change An enhancement which is breaking. type-enhancement A request for a change that isn't a bug web-js-interop Issues that impact all js interop
Projects
Status: Complete
Development

No branches or pull requests

2 participants