Minimum year changed in 0.4.32

[Lorak-mmk]

trafficstars

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]

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

[Lorak-mmk]

You're right, this is the commit that introduced the issue: https://github.com/chronotope/chrono/commit/b72752fd41f844b4dacb0974fe7826d8bd367be4

[Lorak-mmk]

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]

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.

[Lorak-mmk]

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]

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]

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]

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]

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]

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]

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.

[pitdicker]

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]

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]

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]

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