chrono
chrono copied to clipboard
chronotope
DateTime.timestamp_nanos() on valid DateTime panics instead of returning a result/option
While it is documented behaviour, a valid DateTime should not panic while getting a timestamp from it.
Minimal repro
let naive = NaiveDateTime::from_timestamp_opt(456456456456, 0).unwrap(); //Does not panic
let datetime = DateTime::<Utc>::from_utc(naive, Utc)); //Still no panic
println!("{}", datetime.timestamp_nanos()); //This panics
Potential low-effort fix
pub fn timestamp_nanos(&self) -> Option<i64> {
let as_ns = self.timestamp().checked_mul(1_000_000_000);
match as_ns {
Some(ns) => Some(ns + i64::from(self.timestamp_subsec_nanos())),
None => None,
}
}
While testing this potential fix, I uncovered the following points where other unrelated operations on DateTimes call timestamp_nanos(), which suggests to me that this might of greater concern than something that can be chalked up as a low importance "formatting bug".
round.rs: 157
round.rs: 186
An alternate fix would be to add checked variants of timestamp_nanos() and timestamp_millis().
As for my use-case, I can easily work around the issue, but this is still a big problem for other use-cases.
To be honest, I don't get to understand your minimal repro codes. What is ndt in the code? Is it naive that you create by let naive = NaiveDateTime::from_timestamp_opt(456456456456, 0).unwrap();?
But while I checked for the codes of NaiveDateTime::timestamp_nanos, which DateTime::timestamp_nanos eventually calls, I found some comments left by previous developer.
# Panic
Note also that this does reduce the number of years that can be represented from ~584 Billion to ~584 years. The dates that can be represented as nanoseconds are between 1677-09-21T00:12:44.0 and 2262-04-11T23:47:16.854775804.
(If this is a problem, please file an issue to let me know what domain needs nanosecond precision over millennia, I'm curious.)
I guess your problem is of the same reason with what the comment mentions. The time created is so big that causes a multiply overflow. According to the comment, likely that who previously wrote the codes did consider about this overflow problem, and thought that it's nearly impossible to cause in programming field.
I think it is totally okay to make a pull request, using checked_mul() to avoid this overflow probelm. But I wonder if it is necessary to do so. A big time value that is big enough to cause this problem is hardly possible to be a valid value in the program. So it's okay to panic here, in my opinion, to point out that there's a potential bug that just creates a super big DateTime instance.
To be honest, I don't get to understand your minimal repro codes. What is ndt in the code? Is it naive that you create by let naive = NaiveDateTime::from_timestamp_opt(456456456456, 0).unwrap();?
derp, I missed that when reviewing before clicking "Create issue". Yes it is supposed to be naive instead of ndt. I'll edit the post
My use-case involves direct user input, where there is no bound on the input outside of the limits of i64, and where I need to be able to present something to the user, be it the timestamp or an error message, but not a panic (which also causes more things to not be part of the output).
Thankfully all the bits needed to reimplement this with checked multiplication are part of the public API. That way I have checked nanos (exact same code as above), micros, and millis timestamps implemented, and automatically fallback to a less precise one until I have no error.
Interestingly there is no timestamp_micros() method on DateTimes inbetween the nanos and millis versions.
In some way the comment is perfectly right about there being no reasonable need to generate nanoseconds timestamps for datetimes outside a ~584 years range
My use-case involves direct user input, where there is no bound on the input outside of the limits of
i64, and where I need to be able to present something to the user, be it the timestamp or an error message, but not a panic (which also causes more things to not be part of the output).
I understand your situation. But I would argue that there's another solution, that's the user program to have a check on user's input, instead of passing it directly into the function. My point is that the user program will have a better knowledge on the range of valid input, and thus can better handle the invalid input.
And it changes little to have timestamp_nanos() throw an exception instead of panic, since you always have to write
codes outside chrono crate to handle invalid user input.
