boa-dev
We should have separate TimeRecord/etc types and expose RegulateTime
Temporal has a concept of a "time record" which is basically a bag of time types
Boa uses PartialTime for this: https://github.com/boa-dev/boa/blob/c73c3ac2204d34ebc74c8092784d50f5f04c2b13/core/engine/src/builtins/temporal/plain_time/mod.rs#L714
But this is incorrect: PartialTime has u8/u16 fields, whereas time records have no such restriction. These restrictions are instead handled by RegulateTime called in e.g. ToTemporalTime.
Not handling this correctly means that implementations like Boa will accept stuff like {hours: -1} since they get clamped to 0.
We should probably expose separate a separate TimeRecord type which has a regulate() method to produce a PartialTime.
It would also be valid to do RegulateTime in Boa/V8 code instead of Temporal.
For context, this has been something that was handled in Boa with functionality provided via temporal_rs using FiniteF64. For reference, the code can be found here.
That being said, now that I look at it, and the example you mentioned. I think using the above methods is definitely bugged. Either a new method would need to be added or we can look at doing the above with a new type that does the regulation in one go. Although, that may start to run into the same observable order of operations issue present with options re.
Actually, maybe the best answer is to update PartialTime to take a i8 and i16, then constrain to those values. These can then be handled properly in temporal_rs.
That could work, yeah. Would be a bit annoying to update FFI over, but it should be fine.
Note that for Date the gets are far more interleaved: https://tc39.es/proposal-temporal/#sec-temporal-totemporaldate
If there's a preference for one over the other, then I'm definitely open to a different approach.
As for PlainDate, I don't think those should be too bad. We're technically failing them in Boa currently, but it looks like it's because they are being called out of order 😅
Actually, how about this:
We move it over in temporal_rs, and over FFI we add new Record types that have the new behavior, keeping the old Partial types around for a bit. I can signal when they can be removed.
Might actually make sense to keep both around long term since one is more convenient for non browser Rust users who do not care about nitty gritty temporal spec ordering stuff.
(I can explain the details but currently doing backwards incompatible temporal_capi upgrades in V8 is painful)
We should definitely make 100% sure that this actually gets us spec compliance in every situation with a Record though. There are a bunch, and there's tricky ordering stuff to worry about.
An interesting thing here is that Temporal sometimes "extracts" records from one type and shoves them in another. When it does so, it usually does it via the ISO datetime, e.g. in ToTemporalDateTime it does:
c. If item has an [[InitializedTemporalDate]] internal slot, then
iii. Let isoDateTime be [CombineISODateAndTimeRecord](https://github.com/boa-dev/temporal/issues/334#sec-temporal-combineisodateandtimerecord)(item.[[ISODate]], [MidnightTimeRecord](https://github.com/boa-dev/temporal/issues/334#sec-temporal-midnighttimerecord)()).
iv. Return ? [CreateTemporalDateTime](https://github.com/boa-dev/temporal/issues/334#sec-temporal-createtemporaldatetime)(isoDateTime, item.[[Calendar]]).
In V8 right now I'm not using the ISO record: I'm just copying over the original date as y/m/d/calendar, which I think should be equivalent. But there's a chance it isn't.
This isn't just an order of operations issue; this affects doing things with overflow: constrain
The Duration test262 tests:
'built-ins/Temporal/Duration/prototype/add/argument-duration-precision-exact-numerical-values': [FAIL],
'built-ins/Temporal/Duration/prototype/round/out-of-range-when-converting-from-normalized-duration': [FAIL],
'built-ins/Temporal/Duration/prototype/subtract/argument-duration-precision-exact-numerical-values': [FAIL],
'built-ins/Temporal/Duration/prototype/total/precision-exact-mathematical-values-6': [FAIL],
'built-ins/Temporal/Duration/prototype/total/precision-exact-mathematical-values-7': [FAIL],
test behaviors that are only possible when an f64 can be passed down (not an integer). Good reason to migrate at least PartialDuration, though I suspect we have to migrate all of Duration
I think the above tests should be fixed by #189. Although there will have to be some resolution logic.
Thought: The amount of code needed for this is not much: implementors can choose to have TimeRecord, DateRecord, and DateTimeRecord objects and have a to_partial(overflow) on them that converts them into Partial objects; constraining/rejecting based on overflow as needed.
This will still have a minor observability issue in that out-of-integer-range fields will be caught before out-of-calendar-range fields are. Realistically, I don't think that's important.
I'm currently fixing this upstream: https://chromium-review.googlesource.com/c/v8/v8/+/6818056 , https://chromium-review.googlesource.com/c/v8/v8/+/6817937 . It's inefficient, but works; Boa can do something similar if it wishes.
It would be nice to fold these types into temporal_rs eventually to avoid the double-checking.
(Especially for durations; checking duration validity is expensive. Though we might be able to get away with having a clamp-check and a separate full-validity check)
Actually, come to think of it; the current V8 Duration code already does a clamp-with-error and then pass things down, and that doesn't seem to be against the spec; except when precise numerical values are desired, see built-ins/Temporal/Duration/prototype/total/precision-exact-mathematical-values-6 and built-ins/Temporal/Duration/prototype/total/precision-exact-mathematical-values-7.
I did update the integer handling in PlainTime's constructor with FiniteF64s. I still need to do this for the TimeRecord abstract operation. Would it be worthwhile exposing some type in temporal_capi for completing that code?
Also, PartialTime should probably be updated to a TimeRecord or something else (extra important after #487 is merged).
Would it be worthwhile exposing some type in temporal_capi for completing that code?
I'm not actually sure! We're able to do much of the same code in the engine and it's not a huge deal.