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

Introduce impl TryFrom for Number that succeeds iff the value is within the safe range #3847

Merged
merged 14 commits into from
Feb 23, 2024

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Feb 20, 2024

As a counterpoint, raised by MaulingMonkey on discord: it could also make sense to round the number, so maybe TryFrom & co should not be implemented for Number?

OTOH, I personally think the added convenience is worth it, and by implementing TryFrom rather than From we make it explicit that the conversion is fallible, which should make it obvious that the behavior is "error out if the number is outside the safe range". Especially considering that Number in JavaScript is often used to describe integers in the safe range, probably much more often than it is actually used to describe floating point numbers.

A design question I had while writing this: should the Error type be just the passed-in value? It seems to be the way other TryFrom implementations in js-sys are currently done, so I went ahead with the same idea here.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A design question I had while writing this: should the Error type be just the passed-in value? It seems to be the way other TryFrom implementations in js-sys are currently done, so I went ahead with the same idea here.

Where did you see that in js-sys?


I think I'm also in favor of doing the conversion like this, but FWIW alternatively we could make it infallible by using -/+infinity when the number is not in range.

Please add a changelog entry as well.

crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
crates/js-sys/tests/wasm/Number.rs Show resolved Hide resolved
@Liamolucko
Copy link
Collaborator

A design question I had while writing this: should the Error type be just the passed-in value? It seems to be the way other TryFrom implementations in js-sys are currently done, so I went ahead with the same idea here.

Where did you see that in js-sys?

The TryFrom<BigInt> impls for i64, u64, i128 and u128 do that, as well as all the TryFrom implementations in wasm-bindgen.

The argument against doing the same here would be that all those impls convert from JsValue (or an imported JS type) to a standard Rust type; you might want to get the JsValue back on failure to avoid having to needlessly clone it. In this case, we're converting from an integer to a JsValue, so there isn't as much need to get the original value back on failure when the input's Copy anyway.

That would line up with what std does: looking at all of std's implementations of TryFrom<T>, it seems like whenever T can't be cheaply cloned it gives back the T on failure, otherwise it uses a real error type. For example impl TryFrom<Vec<T>> for [T; N] has type Error = Vec<T>, whereas impl TryFrom<&[T]> for [T; N] has type Error = FromSliceError. I don't actually know if this is an official policy or anything but it seems to be the case.

The closest analogy to JsValue would be impl TryFrom<Arc<[T]>> for Arc<[T; N]>, since both are cheap but not free to clone: and it gives back the Arc on failure. So I guess using a proper error type would make more sense.

@Ekleog
Copy link
Contributor Author

Ekleog commented Feb 20, 2024

Thank you for your feedback! I have:

  • hoisted x_f64 as per the suggestion
  • added a changelog entry
  • switched the code to use a TryFromIntError type, copy-pasted from the libstd, the only change being that I didn't import Error globally. We apparently cannot construct the libstd's variant easily, and doing eg. u32::try_from(u64::MAX)? to return it would look like too much of a hack for me — let me know if you'd have preferred that approach

Hopefully this is good to go! :)

@Ekleog Ekleog requested a review from daxpedda February 20, 2024 19:00
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • switched the code to use a TryFromIntError type, copy-pasted from the libstd, the only change being that I didn't import Error globally. We apparently cannot construct the libstd's variant easily, and doing eg. u32::try_from(u64::MAX)? to return it would look like too much of a hack for me — let me know if you'd have preferred that approach

Yeah, my main concern with using Stds TryFromIntError type is that we aren't really in control of what it displays exactly so it could become confusing in the future. Though I'm not happy with adding a dedicated error type for this I don't have a good alternative suggestion.

So except the couple of nits its LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
Ekleog and others added 5 commits February 20, 2024 23:50
Co-authored-by: daxpedda <daxpedda@gmail.com>
Co-authored-by: daxpedda <daxpedda@gmail.com>
Co-authored-by: daxpedda <daxpedda@gmail.com>
@Ekleog Ekleog requested a review from daxpedda February 21, 2024 00:15
@Ekleog
Copy link
Contributor Author

Ekleog commented Feb 21, 2024

@daxpedda Looks like the CI started failing like the one I recently submitted a fix for on web-time. Here, it looks like the check is not required, so I think it'd probably be better to land this without the CI changes, for clarity? And to land the CI fixes independently, depending on whether you prefer bumping the msrv or pinning bumpalo to an older version in CI.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I will go ahead and fix the CI first.

EDIT: See #3850.

crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
@Ekleog
Copy link
Contributor Author

Ekleog commented Feb 21, 2024

Awesome, thank you! I just handled your review comment and merged master in, hopefully this is now good to go :)

@daxpedda
Copy link
Collaborator

@Liamolucko would appreciate your approval here as well.

@daxpedda daxpedda merged commit 557e2e6 into rustwasm:main Feb 23, 2024
25 checks passed
@Ekleog Ekleog deleted the try-from-for-number branch February 23, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants