Update Temporal ToIntegerIfIntegral, ToIntegerWithTruncation, and ToPositiveIntegerWithTruncation implementation #4081
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test262 conformance changes
Fixed tests (111):Broken tests (16): |
nekevss
force-pushed
the
update-integer-conversion
branch
from
43bfaf1 to
5e3edf5
Compare
|
I'm all for having the more specific / representative types in the
I'm not 100% updated on how the current APIs look, but I think instead of silently doing this, there should be helper APIs / types provided by
Does that make sense? |
|
Definitely agreed! 😄 That's why boa-dev/temporal#131 exists and this is still a draft. The helper functions were primarily to double check the approach was working how I thought it would be in Boa before committing to it. Right now, I'm mostly waiting for reviews on the temporal_rs PR and then this can be updated with the change and switched off draft. |
nekevss
added a commit
to boa-dev/temporal
that referenced
this pull request
Dec 16, 2024
…ntegerIfIntegral methods to `FiniteF64` (#131) This PR adds the `truncate` method to `FiniteF64`. The main goal of this PR is to provide engines with a better way of truncating and clamping JavaScript numbers. For reference, please see boa-dev/boa#4081. EDIT: Also, add positive truncation method and to integer if integral method.
|
After looking at the broken tests, one appears to be a regression (case insensitive calendar ids) with a PR submitted on |
nekevss
changed the title
hansl
reviewed
Dec 16, 2024
raskad
approved these changes
Dec 17, 2024
nekevss
force-pushed
the
update-integer-conversion
branch
from
0e743b0 to
156ad7c
Compare
jedel1043
approved these changes
Dec 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Posting this as a draft to hopefully get comments.
TLDR: I'm convinced the way we currently handle converting JsValue to integers in the Temporal implementation is wrong and this is a way to update it.
Baseline issue
Currently, we are using
to_integer_with_truncation,to_positive_integer_with_truncationandto_integer_if_integralto convert values intoi32. These methods are still hold over from the initial prototype of Temporal prior totemporal_rsbeing split off. At the time, I think it was ultimately an optimistic approach, but I'm now fairly certain it's wrong long term and should probably addressed (hence the draft PR) ASAP.Also, every argument being truncated to
i32negatively impactstemporal_rs's API by forcing us to keep the values ati32. This change would allow us to changetemporal_rs's API, and move it away from onlyi32.General approach / Order of Operations
So the general approach of this PR is to first get the value and transform it to a
FiniteF64by callingToNumber()and checking that it's finite, which was essentially all the previous functions were doing anyways besides the truncation.Once the values are made
FiniteF64s, we then get the options object and determine theArithmeticOverflowfrom the options object. This is needed for the observability tests which may check for the finite check throwing prior to a wrong options object throwing.After the options object, we then truncate the value into the integer type that is expected by
temporal_rswith the user definedArithmeticOverflow. If ArithmeticOverflow::Reject and the value is not contained within T::MIN and T::MAX, we throw an early far exceeds range RangeError. If ArithemeticOverflow::Constrain, then we clamp the value to T::MAX or T::MIN.There is also an argument to just always silently constrain the value to T::MAX or T::MIN and let
temporal_rshandle the error. I think I may like this option more, because it does allow us to truncate early withoutArithmeticOverflowand deferring truncation to after reading options.Anyways, general thoughts would be appreciated. Technically, this is probably more of a
temporal_rsleaning PR as I think the currenttruncatefunction in this PR should live intemporal_rs, but it does handle specifically the Boa's glue code to the library.