tc39 / proposal-intl-duration-format

https://tc39.es/proposal-intl-duration-format
MIT License
171 stars 18 forks source link

Sync IsValidDuration with Temporal, including precision range limits #199

Closed sffc closed 3 weeks ago

sffc commented 6 months ago

Temporal was changed to add limits to duration field values:

https://github.com/tc39/proposal-temporal/commit/a9abc12527a46106d54ee060b6fa91704ce0a379

Relevant Temporal discussions:

https://github.com/tc39/proposal-temporal/issues/1700

https://github.com/tc39/proposal-temporal/issues/2195

We should implement this change in Intl.DurationFormat in order to make it more consistent and also allow calculations without using BigInt / mathematical values.

The following test case will need to be updated:

https://github.com/tc39/test262/blob/main/test/intl402/DurationFormat/prototype/format/precision-exact-mathematical-values.js

sffc commented 6 months ago

I believe that the range check may be able to be performed using float math. I'll run some experiments. Either way, the Temporal spec confirms that this can be done using std::remquo.

anba commented 6 months ago

See also #157.

ben-allen commented 6 months ago

We have TC39-TG1 consensus from the November 2023 meeting for duplicating the limits from Temporal. https://github.com/tc39/proposal-intl-duration-format/pull/173#issuecomment-1830827127

sffc commented 6 months ago

I believe that the range check may be able to be performed using float math. I'll run some experiments.

Looking at the formula:

Let normalizedSeconds be days × 86,400 + hours × 3600 + minutes × 60 + seconds + (𝔽(milliseconds)) × 10-3 + (𝔽(microseconds)) × 10-6 + (𝔽(nanoseconds)) × 10**-9.

NOTE: The above step cannot be implemented directly using floating-point arithmetic. Multiplying by 10-3, 10-6, and 10**-9 respectively may be imprecise when milliseconds, microseconds, or nanoseconds is an unsafe integer. This multiplication can be implemented in C++ with an implementation of std::remquo() with sufficient bits in the quotient. String manipulation will also give an exact result, since the multiplication is by a power of 10.

If abs(normalizedSeconds) ≥ 2**53, return false.

It is easy to tell if milliseconds, microseconds, or nanoseconds alone cause us to reach the limit. We can simply find the value that causes us to start exceeding 2^53. It happens to be the case that (2^53) * (10^3), (2^53) * (10^6), and (2^53) * (10^9) are all representable exactly as an f64, so this check is quite simple.

We get problems when we start adding multiple fields together, because the intermediate value might lose decimal places. For example, 9007199254740990976 is an exact f64. If it is the millisecond value, we first multiply that by 10^3, getting the mathematical value 9007199254740990.976, which cannot be represented as an f64. If the seconds value is 1, the algorithm says we should return true in MV space, but in f64 space, the number rounds up and we return false.

However, it seems that this is a general problem, not specific to unsafe integers as the spec says. f64 will always be subject to loss of precision in a particular arithmetic operation.

I don't remember why Temporal decided to check bounds on the result of this weird formula instead of just checking each field individually, especially fractional seconds. If all the fractional seconds were just capped at MAX_SAFE_INTEGER, then it would never be possible to exceed 2^53 seconds when rounded to the nearest integer.

anba commented 6 months ago

If all the fractional seconds were just capped at MAX_SAFE_INTEGER, then it would never be possible to exceed 2^53 seconds when rounded to the nearest integer.

This formula is used to be able to represent all valid instant duration spans:

let minInstant = new Temporal.Instant(-864n * 10n**19n);
let maxInstant = new Temporal.Instant(864n * 10n**19n);
let duration = minInstant.until(maxInstant, {largestUnit: "milliseconds"});
console.log(duration.milliseconds);

This prints 17280000000000000 which is larger than Number.MAX_SAFE_INTEGER.

ben-allen commented 3 weeks ago

fixed in c2f2b4aacb93a46d4d84d16c68e301ef9fceaea1

(see #200, https://github.com/tc39/proposal-intl-duration-format/pull/173)