-
Notifications
You must be signed in to change notification settings - Fork 520
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
DateTime.timestamp_nanos() on valid DateTime panics instead of returning a result/option #586
Comments
To be honest, I don't get to understand your minimal repro codes. What is But while I checked for the codes of
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 |
derp, I missed that when reviewing before clicking "Create issue". Yes it is supposed to be My use-case involves direct user input, where there is no bound on the input outside of the limits of 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. 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 |
I understand your situation. But I would argue that there's another solution, that's the user program to have a check And it changes little to have |
I'm going to look into this. |
@Milo123459 Finally got to test this patch, and it works perfectly for me |
Great! I'll fix that PR in a moment |
This seems to be addressed by #1275. |
I think this can be closed then |
While it is documented behaviour, a valid DateTime should not panic while getting a timestamp from it.
Minimal repro
Potential low-effort fix
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()
andtimestamp_millis()
.As for my use-case, I can easily work around the issue, but this is still a big problem for other use-cases.
The text was updated successfully, but these errors were encountered: