Skip to content

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Aug 12, 2025

This PR makes a large portion of updates from #366 without bringing in the bnum crate.

The changes are primarily as follows:

  • Updates Duration to flat unsigned fields
  • Removes the previous TimeDuration
  • NormalizedDurationRecord -> InternalDurationRecord
  • NormalizedTimeDuration -> TimeDuration

There are a handful of API changes linked to this change that are primarily related to moving addition variations that were using the old TimeDuration.

@nekevss nekevss added the C-internal Internal library improvements label Aug 12, 2025
@nekevss nekevss requested a review from Manishearth August 12, 2025 00:50
@Manishearth
Copy link
Contributor

Running this on v8 introduces more failures.

What does Boa say?


=== test262/built-ins/Temporal/Duration/prototype/round/precision-exact-in-balance-time-duration ===
--- stderr ---
V8 is running with experimental features enabled. Stability and security will suffer.
--- stdout ---
/home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js:49: Test262Error: BalanceTimeDuration should implement floating-point calculation correctly for largestUnit nanoseconds: nanoseconds result Expected SameValue(«-8.692288669465521e+24», «8.692288669465521e+24») to be true
  throw new Test262Error(message);
  ^
Command: /home/manishearth/dev/v8/v8/out/x64.release/d8 --test /home/manishearth/dev/v8/v8/test/test262/data/harness/sta.js /home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js /home/manishearth/dev/v8/v8/test/test262/harness-adapt.js /home/manishearth/dev/v8/v8/test/test262/data/harness/temporalHelpers.js /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Duration/prototype/round/precision-exact-in-balance-time-duration.js --random-seed=551605885 --nohard-abort --testing-d8-test-runner --ignore-unhandled-promises --harmony-temporal --no-arguments
=== test262/built-ins/Temporal/Duration/prototype/round/precision-exact-in-balance-time-duration ===
--- stderr ---
V8 is running with experimental features enabled. Stability and security will suffer.
--- stdout ---
/home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js:49: Test262Error: BalanceTimeDuration should implement floating-point calculation correctly for largestUnit nanoseconds: nanoseconds result Expected SameValue(«-8.692288669465521e+24», «8.692288669465521e+24») to be true
  throw new Test262Error(message);
  ^
Command: /home/manishearth/dev/v8/v8/out/x64.release/d8 --test /home/manishearth/dev/v8/v8/test/test262/data/harness/sta.js /home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js /home/manishearth/dev/v8/v8/test/test262/harness-adapt.js /home/manishearth/dev/v8/v8/test/test262/data/harness/temporalHelpers.js /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Duration/prototype/round/precision-exact-in-balance-time-duration.js --random-seed=551605885 --nohard-abort --testing-d8-test-runner --use-strict --ignore-unhandled-promises --harmony-temporal --no-arguments
=== test262/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant ===
--- stderr ---
V8 is running with experimental features enabled. Stability and security will suffer.
--- stdout ---
/home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant.js:37: RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
assert.sameValue(min.add({nanoseconds: 86_40000_00000_00000_00000 * 2}).epochNanoseconds, max.epochNanoseconds,
                     ^
RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
    at Instant.add (<anonymous>)
    at /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant.js:37:22
Command: /home/manishearth/dev/v8/v8/out/x64.release/d8 --test /home/manishearth/dev/v8/v8/test/test262/data/harness/sta.js /home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js /home/manishearth/dev/v8/v8/test/test262/harness-adapt.js /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant.js --random-seed=551605885 --nohard-abort --testing-d8-test-runner --ignore-unhandled-promises --harmony-temporal --no-arguments
=== test262/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant ===
--- stderr ---
V8 is running with experimental features enabled. Stability and security will suffer.
--- stdout ---
/home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant.js:37: RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
assert.sameValue(min.add({nanoseconds: 86_40000_00000_00000_00000 * 2}).epochNanoseconds, max.epochNanoseconds,
                     ^
RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
    at Instant.add (<anonymous>)
    at /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant.js:37:22
Command: /home/manishearth/dev/v8/v8/out/x64.release/d8 --test /home/manishearth/dev/v8/v8/test/test262/data/harness/sta.js /home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js /home/manishearth/dev/v8/v8/test/test262/harness-adapt.js /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/add/minimum-maximum-instant.js --random-seed=551605885 --nohard-abort --testing-d8-test-runner --use-strict --ignore-unhandled-promises --harmony-temporal --no-arguments
=== test262/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant ===
--- stderr ---
V8 is running with experimental features enabled. Stability and security will suffer.
--- stdout ---
/home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant.js:37: RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
assert.sameValue(min.subtract({nanoseconds: -86_40000_00000_00000_00000 * 2}).epochNanoseconds, max.epochNanoseconds);
                     ^
RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
    at Instant.subtract (<anonymous>)
    at /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant.js:37:22
Command: /home/manishearth/dev/v8/v8/out/x64.release/d8 --test /home/manishearth/dev/v8/v8/test/test262/data/harness/sta.js /home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js /home/manishearth/dev/v8/v8/test/test262/harness-adapt.js /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant.js --random-seed=551605885 --nohard-abort --testing-d8-test-runner --ignore-unhandled-promises --harmony-temporal --no-arguments
=== test262/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant ===
--- stderr ---
V8 is running with experimental features enabled. Stability and security will suffer.
--- stdout ---
/home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant.js:37: RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
assert.sameValue(min.subtract({nanoseconds: -86_40000_00000_00000_00000 * 2}).epochNanoseconds, max.epochNanoseconds);
                     ^
RangeError: Temporal error: Instant nanoseconds are not within a valid epoch range.
    at Instant.subtract (<anonymous>)
    at /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant.js:37:22
Command: /home/manishearth/dev/v8/v8/out/x64.release/d8 --test /home/manishearth/dev/v8/v8/test/test262/data/harness/sta.js /home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js /home/manishearth/dev/v8/v8/test/test262/harness-adapt.js /home/manishearth/dev/v8/v8/test/test262/data/test/built-ins/Temporal/Instant/prototype/subtract/minimum-maximum-instant.js --random-seed=551605885 --nohard-abort --testing-d8-test-runner --use-strict --ignore-unhandled-promises --harmony-temporal --no-arguments

@Manishearth
Copy link
Contributor

It seems like there are two issues: one is that 86_40000_00000_00000_00000 * 2 is no longer a valid ns value, and the other is that the floating point calculations have changed behavior. We should add ctors and partial APIs for Duration that take floats anyway to fix the latter.

@nekevss
Copy link
Member Author

nekevss commented Aug 13, 2025

Oh shoot, we should adjust tests so that this fails then.

EDIT:

What does Boa say?

I have to double check that. I haven't gotten around to testing directly in Boa yet. (But I'd assume the same thing as above)

@nekevss
Copy link
Member Author

nekevss commented Aug 13, 2025

We should add ctors and partial APIs for Duration that take floats anyway to fix the latter.

I'm fine with the general concept for adding some f64 support in partials and the edges, but why would that be needed rather than emulating the shifting maximum/minimum in IsValidDuration? Per the specification, we should only really be receiving integers. A PrecisionPartialDuration or whatever would just automatically reject any non integer value.

@Manishearth
Copy link
Contributor

Per the specification, we should only really be receiving integers

Integer-valued floats, which aren't the same thing since for large integers they will get clipped.

We should have tests for built-ins/Temporal/Duration/prototype/total/precision-exact-mathematical-values-6 and built-ins/Temporal/Duration/prototype/total/precision-exact-mathematical-values-7, which currently fail with failures like this:

/home/manishearth/dev/v8/v8/test/test262/data/harness/assert.js:49: Test262Error: milliseconds=9007199254740992, microseconds=1999 Expected SameValue(«9007199254740992», «9007199254740994») to be true

I haven't fully investigated why. I think it's due to this problem but it might be something else!

@Manishearth
Copy link
Contributor

Manishearth commented Aug 15, 2025

Let me know when you'd like me to test it against v8 again

I would like to avoid new failures; though if those tests can't be replicated in #[test] then that's fine.

@nekevss
Copy link
Member Author

nekevss commented Aug 15, 2025

Let me know when you'd like me to test it against v8 again

The above fix should address the range issue. I'm not entirely sure about the others breaks yet though that aren't related to the range issue, so it may be worth rerunning it.

@Manishearth
Copy link
Contributor

No new test failures!

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Looks good!

}
}

impl TimeDuration {
Copy link
Contributor

Choose a reason for hiding this comment

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

note: deleting this does not affect v8

@nekevss nekevss merged commit 6fe1654 into main Aug 15, 2025
8 checks passed
@nekevss nekevss deleted the update-duration branch September 2, 2025 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-internal Internal library improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants