Initial implementation of Duration.prototype.total

[lockels]

Fixes #139

Tests adapted from sample code in https://tc39.es/proposal-temporal/docs/duration.html under total now pass :)

I have a question in duration.rs about return types!

There's also another question in normalized.rs regarding some code i'd like to refactor that makes this PR quite a bit larger than it needs to be

[lockels]

Duration.prototype.total seems to return floating point numbers according to the temporal documentation, but the signature was orignally returning a TemporalResult<i64>?? I changed this, but I'm not sure if this is correct. I'm pretty sure it shouldn't be an integer like it originally was.

[nekevss]

Disclaimer: I haven't fully reviewed the code here, but I'll provide a couple early comments below (so take it with a grain of salt).

Hmm, this is one of the reasons that Duration is sometimes hard to reason through and also still retains it's FiniteF64 fields instead of integer fields.

It's fine to switch off the i64 return that might have been generous thinking on my part. Although, I would recommend using FiniteF64 over f64 for the added invariant checks of FiniteF64. If we are going to be exposing floating points in the API long term, I think I'd prefer using that type over f64.

My temptation is to remove any reference of FiniteF64 from these APIs, but after looking through this one. It may need to stay. The biggest question is 100% going to be how to handle 7.5.31 TotalTimeDuration in a way that's sane.

[lockels]

Yeah makes sense why'd you'd prefer to use FiniteF64 over f64.

Regarding 7.5.31, there's a comment in the spec saying this:

2. NOTE: The following step cannot be implemented directly using floating-point arithmetic when 𝔽(timeDuration) is not a safe integer.

Which is a bit concerning since timeduration is an i128

[nekevss]

This is part of the reason for using i128 or i64 over f64. The note essentially means that the value can exceed 2^53 - 1, which is the maximum safe integer.

The specification text is written mainly with C++ in mind. There isn't any f128 or emulator for it (hence the move to non-floating point math).

[lockels]

I had to create a new NudgeRecord64 and a duplicate nudge_calendar_unit_f64() that doesn't typecast and store total from a f64 to a i128 to make everything work :/

Any suggestions on how to refactor this bit?

[nekevss]

Hmm, if possible try to adjust the original struct and methods as necessary.

Initial reaction is that it may require taking changing total: 128 to something like total: DurationTotal where the arithmetic of TotalTimeDuraiton is completed on the DurationTotal type.

General thoughts:

struct DurationTotal {
    total: 128,
    unit: TemporalUnit,
}

impl DurationTotal {
    pub fn to_total(self) -> FiniteF64 {
        // Create FiniteF64 from fields here, i.e. handle casting here too
    }
}

Basically my general thought process is that the more that we can implement without using floating points internally, the better.

[lockels]

Looking into this even more. I think i've found the simpest solution to this. problem

struct NudgeRecord {
    normalized: NormalizedDurationRecord,
    _total: Option<FiniteF64>, // Previously Option<i128>
    nudge_epoch_ns: i128,
    expanded: bool,
}

just changing the NudgeRecord to where total should be is just an Option<FiniteF64> is fine and doesn't break things for round afaik, since it doesn't seem to use the member total in the struct.

[nekevss]

This is looking great!! I had a couple nits and then another thought on the DurationTotal.

[nekevss]

thought: Step 2's note basically means that while try_from here might not fail. It does have the potential to exceed the safe integer range.

We've already discussed this elsewhere, but now that I'm looking at this. I'd be curious if we could div_rem to deal with the total, then a DurationTotal could be used:

/// NOTE: I'm mostly guessing integer types, so it could be changeable
struct DurationTotal {
   quotient: i128, // Q: what is the maximum possible quotient
   remainder: u64,
   unit_nanos: u64
}

The fraction can then be reconstructed as a method on DurationTotal.

// pseudo code
impl DurationTotal {
    pub fn to_fractional_total(&self) -> TemporalResult<FiniteF64> {
        let fractional_part = FiniteF64(self.rem) / FiniteF64(self.unit_nanos);
        FiniteF64(self.quotient) + fractional_part
    }
}

Let me know if you think I'm missing something. The nice part about this is that we know that rem / unit is within the safe integer range because none of the available units exceed the maximum safe integer, so the operation is safe and we also minimize floating point operations to a specific call ... or we can just return FinifteF64 that's also a valid option.

Either way, I think the operations listed above may be the better option for the above to avoid any Step 2 issues.

[lockels]

Hey, i've attempted this solution. This part of the spec turned out to a bit more difficult and nuanced than i first assumed! Thanks for pointing it out to me 😅😅

[lockels]

   quotient: i128, // Q: what is the maximum possible quotient

wouldn't it be the maximum possible value for an i128 divided by the smallest case from unit.asnanoseconds()

so 2^127 - 1 / 1 ?

[nekevss]

wouldn't it be the maximum possible value for an i128 divided by the smallest case from unit.asnanoseconds()

By definition, yes. But this is more so what are the constraints on the actual size of the inputs. For instance, Duration can only be a valid Duration. See IsValidDuration op. So could we get away with truncating that field to a smaller integer based on inputs and add a unit test to verify? I don't have an answer to this btw; this is mostly me thinking out loud via GitHub comment. Probably something to test in the future.

This part of the spec turned out to a bit more difficult and nuanced than i first assumed!

Yeah 😅 The handling of Duration rounding has taken me the most time to think through and come up with something that I was happy with. I'm still nervous that there might be some edge case that was missed. If you look at the contrib graph for early last year and see it was rather low for a bit, it was probably because I was trying to think through the best way to handle Duration rounding and the NormalizedDuration/TimeDuration part of the spec in a way I was happy with. Also, partially why I was a bit overly opinionated on the PR review.

[nekevss]

I think that DurationTotal as a concept can potentially be iterated on moving forward, but I think this can be completed in future iterations.

Thanks again for your work on this!