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.

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

Minimum year changed in 0.4.32 #1382

Closed
Lorak-mmk opened this issue Jan 24, 2024 · 15 comments
Closed

Minimum year changed in 0.4.32 #1382

Lorak-mmk opened this issue Jan 24, 2024 · 15 comments

Comments

@Lorak-mmk
Copy link

The following code works with chrono 0.4.31, but fails with 0.4.32:

fn main() {
    use chrono::NaiveDate;

    let min_naive_date: NaiveDate = NaiveDate::MIN;
    assert_eq!(
        min_naive_date,
        NaiveDate::from_ymd_opt(-262144, 1, 1).unwrap()
    );
}

The failure is because from_ymd_opt returns None.

@djc
Copy link
Member

djc commented Jan 24, 2024

Would you be able to bisect? I think it might be #1317 but would be good to be sure.

@Lorak-mmk
Copy link
Author

You're right, this is the commit that introduced the issue: b72752f

@Lorak-mmk
Copy link
Author

Judging from the commit and the fact that it changes the comments too, this is the intended change by this commit, not an error.
But this is a breaking change so it should be released as 0.5, not 0.4.32

@djc
Copy link
Member

djc commented Jan 24, 2024

But this is a breaking change so it should be released as 0.5, not 0.4.32

Honestly I'm not really sure this is obviously true? The API didn't change. The value did change somewhat, but I doubt there's a lot of code that is affected by the exact value of the minimum. I don't think I want to revert this based on a single report, but if it turns out to affect a bunch of other people I might reconsider.

@djc djc changed the title Behavior change in NaiveDate::from_ymd_opt in 0.4.32 Minimum year changed in 0.4.32 Jan 24, 2024
@Lorak-mmk
Copy link
Author

Lorak-mmk commented Jan 24, 2024

The API didn't change, but the behavior did. For example, if someone stored serialized NaiveDate::MIN in a database, they won't be able to retrieve it now.

Also, I think I don't understand something. Right now the comment says:
> The minimum possible `NaiveDate` (January 1, 262144 BCE).

So isn't NaiveDate::from_ymd_opt(-262144, 1, 1).unwrap() exactly this date? Previously the comment had a different date - January 1, 262145 BCE - and the code worked, so I assume that year -X passed to from_ymd_opt corresponds to year X+1 BCE - why?

Ignore the above question, it's probably because year 0 doesn't exist, right? So passing 0 means year -1 BCE?

@djc
Copy link
Member

djc commented Jan 24, 2024

The API didn't change, but the behavior did. For example, if someone stored serialized NaiveDate::MIN in a database, they won't be able to retrieve it now.

That's true -- and does make the case stronger for this being to big of a deal. Are you aware of actual applications or libraries that store NaiveDate::MIN in their ScyllaDB?

Maybe it would be viable to special case this in deserialization code, to silently change -262_144 to -262_143?

Ignore the above question, it's probably because year 0 doesn't exist, right? So passing 0 means year -1 BCE?

Not exactly sure what you're asking here. Year 0 is a well-defined value, but this change is about slightly shrinking the range of negative years, with the minimum supported year changing from -262_144 to -262_143.

@pitdicker
Copy link
Collaborator

pitdicker commented Jan 24, 2024

Ignore the above question, it's probably because year 0 doesn't exist, right? So passing 0 means year -1 BCE?

The reason to change NaiveDate::MIN was because a DateTime can be represented as a value in UTC or a value in the local timezone. When the value is close to DateTime::MIN or DateTime::MAX the implicit conversion between both representations could push the value out of range. So the change took care of many hidden panics by making it possible to represent some 'buffer values' in the type while not allowing their construction outside chrono.

The API didn't change, but the behavior did. For example, if someone stored serialized NaiveDate::MIN in a database, they won't be able to retrieve it now.

Depending on the value of an arbitrary defined constant from another crate is tricky. The year -262144 was clearly pretty implementation-specific. Like @djc I am curious, is this an actual problem or mostly theoretical and uncovered by a failing test?

@Lorak-mmk
Copy link
Author

The API didn't change, but the behavior did. For example, if someone stored serialized NaiveDate::MIN in a database, they won't be able to retrieve it now.

That's true -- and does make the case stronger for this being to big of a deal. Are you aware of actual applications or libraries that store NaiveDate::MIN in their ScyllaDB?

I'm not aware of any actual applications having this problem, so I guess we can just fix this on our end.

Maybe it would be viable to special case this in deserialization code, to silently change -262_144 to -262_143?

Ignore the above question, it's probably because year 0 doesn't exist, right? So passing 0 means year -1 BCE?

Not exactly sure what you're asking here. Year 0 is a well-defined value, but this change is about slightly shrinking the range of negative years, with the minimum supported year changing from -262_144 to -262_143.

I assumed that if I pass -1 as year to NaiveDate::from_ymd_opt it would mean year 1 BCE, but it seems it means year 2 BCE, right?
I'm asking because if passing -1 as year means year 1 BCE, then either the comment or implementation is wrong (because the comment says that minimum date is January 1, 262144 BCE, but NaiveDate::from_ymd_opt(-262144, 1, 1).unwrap() doesn't work).

@pitdicker
Copy link
Collaborator

As another thought: if your code was allowing values close to DateTime::MIN or DateTime::MAX, it was probably easy to craft inputs that would make it panic. Sorry for breaking thing though!

@pitdicker
Copy link
Collaborator

Chrono uses the Proleptic Gregorian calendar. The year 1 BCE is the year 0 in the Proleptic Gregorian calendar. And individual dates before the switch from the Julian to Gregorian calendar (which differs per country) is a bit of a mess 😄.

@djc
Copy link
Member

djc commented Jan 24, 2024

I'm not aware of any actual applications having this problem, so I guess we can just fix this on our end.

I think that would be a decent solution for now. Perhaps it would be better to avoid having your tests depend on the exact value of NaiveDate::MIN?

Again, if we see more widespread reports of this breaking things, I'm open to reverting this change but as it stands I don't think your test case by itself is rationale enough to forego the benefit of avoiding potentially tricky panic behavior.

@djc djc pinned this issue Jan 24, 2024
@pitdicker
Copy link
Collaborator

I'm asking because if passing -1 as year means year 1 BCE, then either the comment or implementation is wrong (because the comment says that minimum date is January 1, 262144 BCE, but NaiveDate::from_ymd_opt(-262144, 1, 1).unwrap() doesn't work).

That comment must have slipped through. Would you be willing to make a PR?

@Lorak-mmk
Copy link
Author

Lorak-mmk commented Jan 24, 2024

Chrono uses the Proleptic Gregorian calendar. The year 1 BCE is the year 0 in the Proleptic Gregorian calendar. And individual dates before the switch from the Julian to Gregorian calendar (which differs per country) is a bit of a mess 😄.

Ok, that makes sense, thanks for explanation.

I'm not aware of any actual applications having this problem, so I guess we can just fix this on our end.

I think that would be a decent solution for now. Perhaps it would be better to avoid having your tests depend on the exact value of NaiveDate::MIN?

Again, if we see more widespread reports of this breaking things, I'm open to reverting this change but as it stands I don't think your test case by itself is rationale enough to forego the benefit of avoiding potentially tricky panic behavior.

That's the plan, I'll change our test. I guess this can be closed.

I'm asking because if passing -1 as year means year 1 BCE, then either the comment or implementation is wrong (because the comment says that minimum date is January 1, 262144 BCE, but NaiveDate::from_ymd_opt(-262144, 1, 1).unwrap() doesn't work).

That comment must have slipped through. Would you be willing to make a PR?

Is the comment incorrect? From your comment about Proleptic Gregorian Calendar I assumed it was fine (because passing -262144 corresponds to 262145 BCE). If it is incorrect, it was also incorrect before the change - it said minimum date was January 1, 262145 BCE, and it was NaiveDate::from_ymd_opt(-262144, 1, 1).unwrap()

@jtmoon79
Copy link
Contributor

I'm not aware of any actual applications having this problem, so I guess we can just fix this on our end.

@Lorak-mmk it sounds like you had a use case explicitly dependent on DateTime::MIN particular value. Can you describe the use case? This may be useful information for the project maintainers.

@djc
Copy link
Member

djc commented Jan 25, 2024

@jtmoon79 the code from the OP suggests that this was a test case in their library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants