-
Notifications
You must be signed in to change notification settings - Fork 517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix minimum Duration value #1385
Fix minimum Duration value #1385
Conversation
- Added tests for creating the maximum and minimum allowable values of Durations having a magnitude of seconds, testing the limits plus one value beyond the limits in both directions. These tests all pass. - Expanded the tests for creating the maximum and minimum allowable values of Durations having a magnitude of milliseconds. These tests examine the results in more detail, document what is being tested, and also test one value beyond the limits in both directions. Notably, the test for Duration::milliseconds() construction for i64::MIN currently fails, as it is erroneously allowed. This test is ignored for now, until the fix is applied. - Expanded the tests for creating the maximum and minimum allowable values of Durations having a magnitude of microseconds and nanoseconds. These tests examine the results in more detail, document what is being tested, and also test one value beyond the limits in both directions. They also test the maximum reportable value from .num_*() and the maximum storable value of the Duration separately. - Separated out the tests for MAX and MIN, for clarity. - Added additional tests for addition and subtraction operations on Durations, ensuring that equivalent tests are performed against both operations, such as adding and subtracting zero, adding and subtracting one nanosecond, and others. - Added tests for greater-than and less-than comparison of two Durations, to ensure that internal representation of partial seconds is correctly ordered.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@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. |
@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! |
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.
In practice, we've found that keeping a little margin can be useful, see #1317 and subsequent discussion in #1382. |
@djc well colour me confused!
There's a section called "The fix" that explains this in detail.
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 The crux of it is what I mentioned here:
// 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 I hope that helps! ⭐ |
@danwilliams Thank you for investigating and all the effort you already put into it!
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 |
@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 I'm happy to update my PR to implement the "other" option, which is the one you mention - i.e. preserving use of 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 In terms of the required changes, I feel there are different levels I could go to:
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! ❤️ |
Sounds great, thanks! |
Thank you. It is only since 0.4.30 that chrono has a completely standalone I think the easiest route is to add |
- Added Panics and Errors sections where appropriate, as these are generally-expected and help draw attention to the fact that the standard (i.e. non-try) constructors can panic. The Errors section for the try constructors is common practice when returning None for overflow situations as well as for functions actually returning a Result. - Added an further explanation of the behaviour of the seconds() constructor. - Minor additional readability edits.
- Added a new Duration::try_milliseconds() function, to attempt to create a new milliseconds-based Duration, and return None if it fails, based on checking against MAX and MIN. Currently, failure can only happen for exactly one value, which is i64::MIN. - Updated Duration::milliseconds() to call try_milliseconds() and panic if None is returned. Although panicking in production code and especially library code is bad, this is in keeping with current Chrono behaviour. Note that this function is now no longer const. - Updated the Duration::milliseconds() documentation to make it clear that it now panics. - Added documentation to Duration::microseconds() and nanoseconds() to make it clear that they are infallible. - All tests now pass, including the one previously ignored.
This reverts commit 3417668. This has been reverted due to discussion on the PR, where the use of the -i64::MAX value was explained. The fix will be implemented in a different manner. chronotope#1385
- Added a description of internal storage and permissable range to the Duration type. - Adjusted formatting of `Duration` in a number of related function Rustdoc blocks, to aid readability. - Clarified comments for Duration::num_milliseconds().
@pitdicker gotcha! That makes sense, and I've gone ahead on that basis 🙂
I have done exactly that. I reverted my original fix, adjusted the tests to align with your explanation, and added
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 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? 🙂 |
d8d8065
to
151ed83
Compare
Thank you! I'll have time to look at it later today, and like your doc changes and the change to The tests seem a bit excessive, but I'll have to read them a bit better first 😄. |
@pitdicker No worries! I'm glad you like the documentation changes 👍
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?
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 |
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. |
@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 🙂 |
Thank you! Yes, we use rebase and merge with a small number of exceptions. |
151ed83
to
3ffcc18
Compare
@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? 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work (better than what I do 😆)!
I only have one request.
3ffcc18
to
3163fab
Compare
@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 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! @djc will have the final say.
- Adjusted MIN_MILLISECONDS, MIN_NANOSECONDS_FULL, and MIN_MICROSECONDS_FULL due to changes in Chrono 0.4.32 that were not previously noticed, which restrict the minimum value of milliseconds by 1, by changing the limit from i64::MIN to -i64::MAX. This was carried out under chronotope/chrono#1334. Notably, Chrono 0.4.32 does still allow creation of a Duration with a milliseconds value of i64::MIN, which is why this change was not noticed sooner. This will be corrected when Chrono 0.4.34 is released, with chronotope/chrono#1385. In the meantime, the correction of constant values here means that we are now compatible with both versions and also do not suffer from the current Chrono bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good modulo a few nits.
src/duration.rs
Outdated
// Technically we don't need to compare against MAX, as this function | ||
// accepts an i64, and MAX is aligned to i64::MAX milliseconds. However, | ||
// it is good practice to check it here in order to catch any future | ||
// changes that might be made. | ||
if d < MIN || d > MAX { | ||
return None; | ||
} | ||
Some(d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm. I see a failure to check against
MAX
in the code would be poor in caseMAX
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...sorry, I overlooked replying to the style of the
match
you have proposed. I believe a simpleif
is more idiomatic here, and thematch
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
src/duration.rs
Outdated
} | ||
|
||
/// Makes a new `Duration` with the given number of microseconds. | ||
/// | ||
/// The number of microseconds acceptable by this constructor is far less |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair...? I can't say I mind either way, but "far less" is accurate and not particularly opinionated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO in this context it is irrelevant how much less it is, so better to leave it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not remarking on the accuracy but on the relevancy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
3163fab
to
0024cee
Compare
Thank you for working on this! |
@pitdicker no worries! I've enjoyed getting involved ❤️ |
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
usingDuration::milliseconds()
, it is possible to create values using the full range of ani64
, and create aDuration
at the absolute limit ofi64::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 ani64
, 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 thani64::MIN
.Creating a new
Duration
usingDuration::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()
: UnaffectedDuration::minutes()
: UnaffectedDuration::hours()
: UnaffectedDuration::days()
: UnaffectedDuration::weeks()
: UnaffectedAdd
->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 throughDuration::min_value()
, which specifies:Meanwhile, creation through the various constructor functions follows two different approaches: those of a magnitude of seconds and above check
MIN
(andMAX
), whereas those of a magnitude of milliseconds and below check against the range of thei64
type.Seconds and above use:
Milliseconds and below use:
Perhaps the best illustration of this bug is that subtracting zero from a
Duration
ofi64::MIN
fails - but it fails at thechecked_sub()
stage, and not at the creation stage: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 thei64
type limiting the possible input. If the artificial constraint is required, then these should havetry_*()
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 thei64
-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
andMIN
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 ofi64
, after which the tests all pass.Previously:
After the implementation of the fix:
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 toMIN.nanos
- changing the modulo to usei64::MIN
instead of-i64::MAX
results in a modulus of192
instead of193
, which fixes the bug and allows the fulli64
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()
:And again in the definition for
Duration.num_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. usingabs()
) 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 adjustingMIN
toi64::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 thatDuration::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.