Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Timestamp::saturating_add misbehaves if the span contains calendar units #36

Closed
FeldrinH opened this issue Jul 23, 2024 · 1 comment
Closed

Timestamp::saturating_add misbehaves if the span contains calendar units #36

FeldrinH opened this issue Jul 23, 2024 · 1 comment
Labels
breaking change Issues that require a breaking change for resolution. bug Something isn't working

Comments

@FeldrinH
Copy link 

let now = Timestamp::now();
let span = 1.day();
println!("{}", now.saturating_add(span));

Expectation: Error, panic or some other indication that addition failed.

Actual result: 9999-12-30T22:00:00.999999999Z
@BurntSushi BurntSushi added the bug Something isn't working label Jul 23, 2024
@BurntSushi
Copy link
Owner

Thank you for catching this. This was a bad oversight on my part. I think I had written the saturating method routine before I smoothed out the error cases for checked arithmetic. This will indeed mean that Timestamp::saturating_add should return an error. I might make it panic in the interim before a jiff 0.2 release.

This also motivates #21, because with an "absolute" duration, we can provide infallible saturating arithmetic on Timestamp.

@BurntSushi BurntSushi added the breaking change Issues that require a breaking change for resolution. label Jul 23, 2024
BurntSushi added a commit that referenced this issue Jul 26, 2024
@BurntSushi
timestamp: make saturating arithmetic panic for non-uniform units
c34da2b 
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
@BurntSushi BurntSushi mentioned this issue Jul 26, 2024
Merged
BurntSushi added a commit that referenced this issue Jul 26, 2024
@BurntSushi
timestamp: make saturating arithmetic panic for non-uniform units
b918099 
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
BurntSushi added a commit that referenced this issue Jul 26, 2024
@BurntSushi
timestamp: make saturating arithmetic panic for non-uniform units
29febf0 
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
BurntSushi added a commit that referenced this issue Jan 24, 2025
@BurntSushi
timestamp: make saturating_add properly fallible
d200bf1 
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
BurntSushi added a commit that referenced this issue Jan 25, 2025
@BurntSushi
timestamp: make saturating_add properly fallible
6d0b2a6 
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
BurntSushi added a commit that referenced this issue Jan 27, 2025
@BurntSushi
timestamp: make saturating_add properly fallible
b89cdca 
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
BurntSushi added a commit that referenced this issue Jan 30, 2025
@BurntSushi
timestamp: make saturating_add properly fallible
8081ec7 
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
BurntSushi added a commit that referenced this issue Feb 1, 2025
@BurntSushi
timestamp: make saturating_add properly fallible
fee3738 
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
BurntSushi added a commit that referenced this issue Feb 2, 2025
@BurntSushi
timestamp: make saturating_add properly fallible
794e6e0 
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change Issues that require a breaking change for resolution. bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@BurntSushi @FeldrinH