Fix minimum Duration value

[danwilliams]

Summary of changes

Note: See the Update section at the end for the final position on this PR after discussions.

This PR fixes a bug found in the representation and treatment of the minimum Duration value, as explained in detail below.

It will be best to review the changes per-commit, as the first commit adds tests and the second commit adds the fix. The second commit is very small.

The problem

The documentation clearly states in multiple locations that the possible range for a Duration is -i64::MAX through to +i64::MAX milliseconds (see References below). However, in testing it was found that the minimum value is inconsistently supported.

When creating a new Duration using Duration::milliseconds(), it is possible to create values using the full range of an i64, and create a Duration at the absolute limit of i64::MIN, which is of course not quite the same as -i64::MAX, but one millisecond below it.

However, when adding two Duration instances together, then even if the result would be within the limits of an i64, an overflow error occurs if the result falls within the bounds of the very smallest millisecond; i.e. this operation respects -i64::MAX as the limit rather than i64::MIN.

Creating a new Duration using Duration::seconds(), or higher magnitudes, is not affected by this issue due to the difference getting rounded away.

In other words, addition is artificially limiting the minimum value by an extra millisecond when compared to what is actually possible - but constructors are allowing the full i64 range.

List of values supported:

• Duration::nanoseconds(): i64::MIN
• Duration::microseconds(): i64::MIN
• Duration::milliseconds(): i64::MIN
• Duration::seconds(): Unaffected
• Duration::minutes(): Unaffected
• Duration::hours(): Unaffected
• Duration::days(): Unaffected
• Duration::weeks(): Unaffected
• Add -> Duration::add(): -i64::MAX
• Duration.checked_add(): -i64::MAX
• Sub -> Duration::sub(): -i64::MAX
• Duration.checked_sub(): -i64::MAX

The cause

The discrepancy was traced to the definition of duration::MIN, available through Duration::min_value(), which specifies:

pub(crate) const MIN: Duration = Duration {
    secs: -i64::MAX / MILLIS_PER_SEC - 1,
    nanos: NANOS_PER_SEC + (-i64::MAX % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
};

Meanwhile, creation through the various constructor functions follows two different approaches: those of a magnitude of seconds and above check MIN (and MAX), whereas those of a magnitude of milliseconds and below check against the range of the i64 type.

Seconds and above use:

pub fn try_seconds(seconds: i64) -> Option<Duration> {
    let d = Duration { secs: seconds, nanos: 0 };
    if d < MIN || d > MAX {
        return None;
    }
    Some(d)
}

Milliseconds and below use:

pub const fn milliseconds(milliseconds: i64) -> Duration {
    let (secs, millis) = div_mod_floor_64(milliseconds, MILLIS_PER_SEC);
    let nanos = millis as i32 * NANOS_PER_MILLI;
    Duration { secs, nanos }
}

Perhaps the best illustration of this bug is that subtracting zero from a Duration of i64::MIN fails - but it fails at the checked_sub() stage, and not at the creation stage:

// This gets created...
Duration::milliseconds(i64::MIN).checked_sub(&Duration::milliseconds(0))
// ...but it fails here .........^

The solution

Which of the two approaches is correct? That is not entirely clear. However, there seems to be no good reason to constrain the minimum value by an extra millisecond, so it seems the premise of the code in the definition of MIN is erroneous - possibly due to a miscalculation when the code was originally written.

When considering MAX, the range is the maximimum possible number of milliseconds, with zero nanoseconds.

The behaviour of MIN should be the same in reverse, surely?

Also supporting this reasoning is the fact that there are no try_*() methods available for magnitudes lower than seconds, as the functions for milliseconds, microseconds, and nanoseconds all rely on the i64 type limiting the possible input. If the artificial constraint is required, then these should have try_*() methods added and should fail for values in that lowest millisecond - which seems annoying.

Furthermore, the impact onto existing code relying upon Chrono is higher by restricting those areas currently using i64::MIN to use -i64::MAX than vice versa.

Therefore, this PR addresses the issue by modifying the allowed minimum limit in MIN, thereby bringing it into line with the i64-based calculations and requiring no further changes to logic.

If this is not the right outcome then this approach can of course be changed, which will involved modifying the logic of various functions that are otherwise inconsistent, and it would be good to understand the reason for the lowest millisecond being removed from usable range in order to add appropriate documentation.

The fix

The first step was to implement a range of tests against the MAX and MIN values, and the various constructors and possible arithmetic operations, in order to ensure complete coverage and show the issue clearly before making any changes. The first commit shows this - and demonstrates the inconsistency, as some of the tests fail. These failures are noted with comments.

The next step was to amend the MIN definition to bring it in line with the full range of i64, after which the tests all pass.

Previously:

pub(crate) const MIN: Duration = Duration {
    secs: -i64::MAX / MILLIS_PER_SEC - 1,
    nanos: NANOS_PER_SEC + (-i64::MAX % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
 };

After the implementation of the fix:

pub(crate) const MIN: Duration = Duration {
    secs: i64::MIN / MILLIS_PER_SEC - 1,
    nanos: NANOS_PER_SEC + (i64::MIN % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
 };

Note, the change for MIN.secs is for clarity and consistency only - the new and previous expressions here are equivalent, and result in the same number being computed, due to rounding. The actual difference comes from the change to MIN.nanos - changing the modulo to use i64::MIN instead of -i64::MAX results in a modulus of 192 instead of 193, which fixes the bug and allows the full i64 range to be used for representing milliseconds, aligning with the constructor behaviour.

The documentation has also been updated to reflect the range is from i64::MIN and not -i64::MAX.

Impact

Because this fix only affects the minimum Duration, and because it furthermore only affects comparison operators and not construction, the impact should be minimal if not nil. Once the fix was implemented, the failing tests passed, and there was no further effect on any other tests. Any code using Chrono should therefore be unaffected, as the values created will be unaffected, and the only different is that some values that were previously rejected (the ones in the lowest possible millisecond) will now be accepted.

References:

In the definition for duration::MIN:

/// The minimum possible `Duration`: `-i64::MAX` milliseconds.

And in the definition for Duration::try_seconds():

/// Makes a new `Duration` with given number of seconds.
/// Returns `None` when the duration is more than `i64::MAX` milliseconds
/// or less than `-i64::MAX` milliseconds.

And again in the definition for Duration.num_milliseconds():

// A proper Duration will not overflow, because MIN and MAX are defined
// such that the range is exactly i64 milliseconds.

And finally in the definition of Duration::min_value():

/// The minimum possible `Duration`: `-i64::MAX` milliseconds.

Points of note

Tests

The tests added have been done so with the purpose of specifically identifying and explaining the issue being fixed, whilst also providing useful information for longer-term purposes. To this end, explanatory comments have been added.

Branch point

The branch point chosen has been the latest tagged release, with the PR targeting the 0.4.x branch.

Update

After discussions, it was established that use of -64::MAX as the minimum value was intentional (and implemented recently, under #1334), due to the ease of swapping sign (e.g. using abs()) without having to perform checks. Although this does incur some surprise when first encountered, it does make sense; and, providing it is documented sufficiently, is completely usable and suitable.

The fix in this PR has therefore been changed, so that the creation of milliseconds is constrained to -i64::MAX instead of adjusting MIN to i64::MIN. This satisfies the disconnect between creation and arithmetic operations.

This has been effected through the introduction of a new Duration::try_milliseconds() function, allowing a fallible check to be made. Duration::milliseconds() has been updated to use this. Unfortunately this does mean that Duration::milliseconds() can now panic, which is hugely undesirable, but this is in keeping with the rest of the Chrono constructors and so this PR follows that pattern.

Additional documentation has been added to try and add clarity to the behaviour and intention.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bf70419) 91.69% compared to head (0024cee) 91.84%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1385      +/-   ##
==========================================
+ Coverage   91.69%   91.84%   +0.15%     
==========================================
  Files          38       38              
  Lines       17608    17491     -117     
==========================================
- Hits        16145    16064      -81     
+ Misses       1463     1427      -36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djc]

@danwilliams thanks for working on this! I quickly looked over the library changes and I think they make sense.

@pitdicker do you have time to review this? You may have more context on the desirability of such a change.

[danwilliams]

@djc no worries! 🙂 I initially thought I had a bug in my code, until I traced the cause - and I knew a change like this would need very careful corroboration and indeed a full explanation of cause, effect, and impact in order to be convincing and get considered for merge.

@pitdicker I'm very open to any feedback you may have - I believe I have chosen the most appropriate fix, but let me know!

[djc]

full explanation of cause, effect, and impact in order to be convincing and get considered for merge.

To be honest, for me your OP is not ideal: the summary doesn't really go into what the fix is or what motivated the fix, and the other sections are... a little hard to digest.

Which of the two approaches is correct? That is not entirely clear. However, there seems to be no good reason to constrain the minimum value by an extra millisecond, so it seems the premise of the code in the definition of MIN is erroneous - possibly due to a miscalculation when the code was originally written.

In practice, we've found that keeping a little margin can be useful, see #1317 and subsequent discussion in #1382.

[danwilliams]

@djc well colour me confused!

the summary doesn't really go into what the fix is

There's a section called "The fix" that explains this in detail.

or what motivated the fix

There's a section called "The problem" that covers this 🙂

Basically I found in my own codebase that I was experiencing odd behaviour. My tests were showing inconsistencies, which I tracked down to Chrono.

I do understand that it takes time to read, but I've laid out the PR description and the accompanying commits in sufficient detail to make the problem clear. The problem is that Chrono currently has a bug in that the treatment of the minimum Duration value is different when creating a Duration versus performing operations on Durations.

The crux of it is what I mentioned here:

Perhaps the best illustration of this bug is that subtracting zero from a Duration of i64::MIN fails - but it fails at the checked_sub() stage, and not at the creation stage:

// This gets created...
Duration::milliseconds(i64::MIN).checked_sub(&Duration::milliseconds(0))
// ...but it fails here .........^

In short there is currently a bug, and I've chosen the least-impactful change in terms of a fix. The alternate fix would require additional logic changes and have a wider impact, but it's up to you guys. I personally don't feel Duration requires the same padding as DateTime, as it doesn't have the same cases to consider, but that's just my opinion. Also, this current bug is not a case where padding has been added purposefully, or with any beneficial effect - it's simply a bug. I feel any such padding would have to have a reason behind it? And presumably should apply to the maximum value as well as the minimum? And I don't see a motivation for that 😅

I hope that helps! ⭐

[pitdicker]

@danwilliams Thank you for investigating and all the effort you already put into it!

Duration::MIN was changed to be the same as -Duration::MAX in #1334. The goal was to make Duration easier to use by removing the possibility of panics when negating or taking the absolute value of a duration. It does not really make sense for our Duration type to behave like a two-complements integer, given that its range is chosen pretty much arbitrary and is far less than its types support.

Even in just chrono this fixes potential panics in something like 10 methods that happen to negate a duration hidden under a bunch of abstractions. I expect it is no different for user code.

But as you correctly point out there now is a bug, as Duration::milliseconds is missing the range check and has no fallible variant. I think it is best to add the range check there, similar to all other initialization methods with larger units. Would you be willing to adjust your PR to do so?

[danwilliams]

@pitdicker aha, thanks for the explanation! Perhaps I should have looked a bit harder for a cause, but it just seemed like a mistake 😅

That also explains now some of the confusion I've had in encountering this, because I had some code that was partly-done which seemed okay, and then I stumbled upon this. Unfortunately I didn't put two and two together and realise that it was affected by the 0.4.32 version of Chrono that I had just upgraded to, and although I had scanned the changelog, that was before I found this problem and therefore I missed the connection. That makes a lot of sense now!

I'm happy to update my PR to implement the "other" option, which is the one you mention - i.e. preserving use of -i64::MAX and updating various locations elsewhere to respect this. Your reasoning works for me - I don't think use of i64::MIN is bad, but as you reason under #1334, it is arbitrary, and therefore there is some nicety in being able to reverse the sign without concern. I think that people will need to perform checks either way, and so in some ways it's much of a muchness. The avoidance of issues when using abs() is compelling, though 👍

I will add the changes to this PR later today. Please note, though, that they will have a wider impact than the "smaller" fix because use of -i64::MAX not only requires more locations to be changed but also has the potential to affect existing codebases in the wider community, as it will no longer accept that particular value for creation. I don't know if that was considered for #1334, but I doubt it matters very much in reality.

In terms of the required changes, I feel there are different levels I could go to:

1. Just fix the bug - this will require adding the range checks to the constructor functions that omit them
2. Also add in equivalent try_*() methods for those constructors that will now be able to fail

I'm happy to cover both, but I am mindful of keeping PRs focused. My suggestion would be that I cover the first level under this PR, to get the bug fixed, and then raise a second PR to then add in the additional checking functions? Will that be okay with you? 🙂

Also, thank you for reviewing so quickly! ❤️

[djc]

I'm happy to cover both, but I am mindful of keeping PRs focused. My suggestion would be that I cover the first level under this PR, to get the bug fixed, and then raise a second PR to then add in the additional checking functions? Will that be okay with you? 🙂

Sounds great, thanks!

[pitdicker]

Thank you. It is only since 0.4.30 that chrono has a completely standalone Duration type, independent of the time 0.1 crate. 0.4.32 was the first release with serious changes to make the methods on that type less panic-prone.

I think the easiest route is to add Duration:try_milliseconds, and then use that in Duration::milliseconds. And maybe make some adjustments to the comments that still claim we match the exact range of i64 milliseconds?

[danwilliams]

@pitdicker gotcha! That makes sense, and I've gone ahead on that basis 🙂

I think the easiest route is to add Duration:try_milliseconds, and then use that in Duration::milliseconds.

I have done exactly that. I reverted my original fix, adjusted the tests to align with your explanation, and added Duration::try_milliseconds() in the style used elsewhere. I have indeed updated Duration::milliseconds() to use Duration::try_milliseconds(), but it does pain me somewhat to be introducing a panic... 😅 Although it makes me wince slightly, I've justified it as being in line with existing behaviour, and hopefully will be properly corrected in v0.5 where you can make more extensive changes 🤞 I do think I will need to offer up a sacrifice to the coding gods of Rust later to appease them though! 😂

And maybe make some adjustments to the comments that still claim we match the exact range of i64 milliseconds?

I've gone through and made adjustments, ensuring that the descriptions are accurate and explanatory, and I've also taken the liberty of adding the expected-as-standard sections for "Errors" and "Panics" where appropriate, to draw attention to these cases. I have also added a short description and explanation to the Duration type itself. I haven't wanted to go too far, as this is a fix - so I've tried to keep it all in scope; but please let me know if I've overstepped.

I've also added an "Update" section to the PR description to make it clear as to the outcome.

Hopefully this is all okay - please let me know if you would like any changes? 🙂

[pitdicker]

Thank you! I'll have time to look at it later today, and like your doc changes and the change to milliseconds and try_milliseconds.
Can you please rework the commits to remove your earlier approach?

The tests seem a bit excessive, but I'll have to read them a bit better first 😄.

[danwilliams]

@pitdicker No worries! I'm glad you like the documentation changes 👍

Can you please rework the commits to remove your earlier approach?

Hmmm... I kept them in simply because when you guys merge, you squash, and therefore those commits will disappear. So the final result will have the outcome only, and meanwhile I felt that retaining them gives clarity to what has happened over the course of the PR, and how the update has changed things from the original fix I proposed. I'd prefer to keep them here for that purpose only, as they align?

The tests seem a bit excessive

Quite possibly the accompanying comments are a little verbose 😊 However, I added those for clarity over the behaviour here, as anyone looking into these ranges in future will benefit from knowing the intent. Each actual test is chosen carefully in order to ensure sensible things are being tested. Arguably the ones for Ord could be reduced a little... let me know!

[pitdicker]

Hmmm... I kept them in simply because when you guys merge, you squash, and therefore those commits will disappear.

Well, I might do so but your are not going to get @djc's approval without reworking the commits. Sorry. And we don't squash, your commits will not end up to be useless.

[danwilliams]

@pitdicker Interesting - I didn't see any/many branch merge points, but upon closer inspection that might be because you're using rebase merge - that's unclear.

Anyway, thank you for the explanation - that makes sense 👍 I'll rebase and pop up a changed version now 🙂

[pitdicker]

Thank you! Yes, we use rebase and merge with a small number of exceptions.

[danwilliams]

@pitdicker All updated - I removed the commits that applied and reverted the fix, and merged the commit amending the test changes with the original commit where I added the additional tests. I also reworked the commit message to be accurate now that it contains the sum of both actions.

Given these commits will all come into the main branch, this should form a clearer narrative of the ultimate outcome, whilst retaining atomicity of changes. Thanks again for your clarification on your process in this regard 👍

Does this seem okay now, or would you like any further changes? 🙂

[pitdicker]

Excellent work (better than what I do 😆)!

I only have one request.

[danwilliams]

@pitdicker thanks again for your time reviewing this - and for the unexpected compliment! 😊

Further to your feedback request I have tweaked the failing test to be ignored initially, until the fix is applied, thereby ensuring each commit still passes all tests. I hope that's okay 🤞

[pitdicker]

Thank you! @djc will have the final say.

[djc]

Thanks, this looks good modulo a few nits.

[djc]

I don't think we should test against MAX here. Instead we should have a test that catches this case, shifting the burden from run time to development time. (Keeping a comment around about why we don't need to test against MAX here is useful, though.)

I'd also prefer to spell this

match d < MIN {
    true => None,
    false => Some(d),
}

[danwilliams]

Hmmmm. I see a failure to check against MAX in the code would be poor in case MAX is changed in future. I am not clear on how a test would necessarily catch this, as there would likely be a self-dependency and it would definitely obfuscate?

[djc]

I'm also fine not having a test, but I don't think it makes to sense to reassert things that the type system has already checked for us. (I also think it is very unlikely MAX will change.)

[danwilliams]

...sorry, I overlooked replying to the style of the match you have proposed. I believe a simple if is more idiomatic here, and the match is less clear. Additionally, the short-circuit fits with the same logic elsewhere, making the similarity obvious. I'd like to keep that code as-is.

[pitdicker]

Hmmmm. I see a failure to check against MAX in the code would be poor in case MAX is changed in future.

If we change MAX we have to look at the entire module anyway (and possibly code outside it). I do have some experience in how it is easy to forget a check 😉. But I also think it is very unlikely for MAX to change.

[djc]

...sorry, I overlooked replying to the style of the match you have proposed. I believe a simple if is more idiomatic here, and the match is less clear. Additionally, the short-circuit fits with the same logic elsewhere, making the similarity obvious. I'd like to keep that code as-is.

Sorry, but I stand by my earlier comment. In cases like these, having the match express the last expression in one IMO makes the code easier to grok compared to having an early return path, and it lays out the two return paths right next to each other instead of them being separated both by more vertical space and offset by one horizontal tab stop. The surrounding code is probably older and hasn't gotten recent review so I don't think it's a great guide as to what makes for good style.

[danwilliams]

I don't mind removing the check for MAX, having explained why I think it's a good idea to retain it. The point I was making is that the type system having checked it for us is coincidentally correct, and not infallibly correct, which is why usually codebase-specific limits would be carefully checked. But given the context here, I agree that it is unlikely to change, and if it does, is likely to be picked up. So although I think the change is not ideal, I don't see it as particularly harmful - done! 🙂 Pushed 👍

[djc]

I'd prefer to change "far less" to just "less", since I think the "far" provides unnecessar(il)y (opinionated) details (same for the next one)/.

[danwilliams]

Fair...? I can't say I mind either way, but "far less" is accurate and not particularly opinionated.

[djc]

IMO in this context it is irrelevant how much less it is, so better to leave it out.

[danwilliams]

I'm just musing on this - it's a million times less; that does seem "far less" 😄 It's not like -i64::MAX vs i64::MIN.

[djc]

I'm not remarking on the accuracy but on the relevancy.

[pitdicker]

Just a general feeling I have with documentation:
Documentation should not look like a cigarette pack by listing all the ways in which it is harmful. It should above all show how to solve problems, and give details and warnings as appropriate.

In the last few releases of chrono I added documentation to a lot of methods around panic cases and errors. I have not compensated it enough with positive documentation 😄.

While reviewing my though about this documentation was: "This function is infallible" is enough, but this is okay.

Ready to finish up with the requests by @djc? Then I'll rebase #1337 on your work.

[danwilliams]

Fair enough 🙂 I don't see it as an issue, but equally I don't really care either way on this one. I'll push up a change for it.

[pitdicker]

Thank you for working on this!

[danwilliams]

@pitdicker no worries! I've enjoyed getting involved ❤️