Incorrect handling of DST changeover for PrimitiveDateTime

[korrat]

When using assume_timezone on a PrimitiveDateTime close to the forward DST changeover, I get an unexpected result for the offset in the end. In my opinion, time-tz does not yet handle the timezone correctly in this situation.

The following test demonstrates the issue. I've also included a test for the backward changeover, but I think the behaviour there is acceptable.

mod time_tz {
    use time::macros::datetime;
    use time_tz::timezones::db::CET;
    use time_tz::PrimitiveDateTimeExt as _;

    #[test]
    fn handles_forward_changeover() {
        assert_eq!(
            datetime!(2022-03-27 01:30).assume_timezone(CET),
            datetime!(2022-03-27 01:30 +01:00)
        );
    }

    #[test]
    fn handles_backward_changeover() {
        // During backward changeover, the hour between 02:00 and 03:00 occurs twice, so either answer is correct
        /* assert_eq!(
            datetime!(2022-10-30 02:30).assume_timezone(CET),
            datetime!(2022-10-30 02:30 +02:00)
        ); */
        assert_eq!(
            datetime!(2022-10-30 02:30).assume_timezone(CET),
            datetime!(2022-10-30 02:30 +01:00)
        );
    }
}

On 2022-03-27, the date of DST changeover in CET, 01:30 in CET is 00:30 in UTC, but 01:30 in UTC is 03:30 in CET due to DST changeover.

My expectation of assume_timezone was that it behaves similar to assume_offset on PrimitiveDateTime, which considers the PrimitiveDateTime to be in the provided offset.

The exhibited behaviour is correct when treating the PrimitiveDateTime as UTC, but I'd argue that this would not be very useful. If this is indeed the expected behaviour, the documentation should highlight this fact.

I think the issue comes down to this line in assume_timezone:

let offset = tz.get_offset_utc(&self.assume_utc());

---

For completeness, I've included the behaviour of chrono-tz. Note that the second test panics, since the local time 02:30 is ambiguous during backward DST changeover in CET.

mod chrono {
    use chrono::NaiveDate;
    use chrono::NaiveDateTime;
    use chrono::NaiveTime;
    use chrono::TimeZone;
    use chrono_tz::CET;

    #[test]
    fn handles_forward_changeover() {
        let local = CET
            .from_local_datetime(&NaiveDateTime::new(
                NaiveDate::from_ymd(2022, 3, 27),
                NaiveTime::from_hms(01, 30, 00),
            ))
            .unwrap();

        assert_eq!(local, CET.ymd(2022, 3, 27).and_hms(01, 30, 00));
    }

    #[test]
    #[should_panic]
    fn handles_backward_changeover() {
        let local = CET
            .from_local_datetime(&NaiveDateTime::new(
                NaiveDate::from_ymd(2022, 10, 30),
                NaiveTime::from_hms(02, 30, 00),
            ))
            .unwrap();

        assert_eq!(local, CET.ymd(2022, 10, 30).and_hms(02, 30, 00));
        assert_eq!(local, CET.ymd(2022, 10, 30).and_hms(02, 30, 00));
    }
}

[Yuri6037]

Well it does not behave like assume_offset. It assumes the PrimitiveDateTime is in UTC. That system has not been designed to work on non-UTC date times. You should first convert all date times to UTC.

EDIT: Regarding documentation maybe I should make it clear but it's indeed UTC based. The PrimitiveDateTime you give to assume_timezone must be UTC otherwise the function behavior is unexpected. I thought the name of get_offset_utc in the Timezone trait made that clear. Maybe the function name assume_timezone is also not clear.

Regarding the actual issue of assuming the PrimitiveDateTime is already in the proper offset, this would have to be a new API.

EDIT2: For completeness I think we should both methods but I have no idea about the API names. I will open issues towards that goal.

[Yuri6037]

@korrat I've just pushed a new version 0.7.0 in the fix-assume-timezone branch (not yet in crates.io). That version includes a breaking change from 0.6 which uses the default offset for the given timezone in assume_timezone. This works with your test case and should work on any PrimitiveDateTime which is proper for the current timezone.

Does this new version 0.7 in master behave like you'd expect?

Regarding chrono-tz, unfortunately I can't achieve that much details with a PrimitiveDateTime because a PrimitiveDateTime is not convertible to a unix timestamp, so it's not possible to perform binary search directly on the PrimitiveDateTime. To provide ambiguity checking, a new API function would have to be provided but on OffsetDateTime which can be converted to a unix timestamp.

[korrat]

Thanks, @Yuri6037. While version 0.7 indeed behaves as I'd expect for that one test case, it fails two other test cases now:

• for a local date time of 02:30, which is invalid in CET, the result is 2022-03-27T02:30+01:00.
• for a local date time of 03:30, which is after DST changeover, the result is 2022-03-27T03:30+01:00, but should be 2022-03-27T03:30+02:00

For naming, you might want to change the name of the method to something like assume_timezone_local, to prevent silent changes for people relying on the UTC offset behaviour now.

---

As for chrono-tz, it seems that NaiveDateTime::timestamp simply assumes UTC offset (https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDateTime.html#method.timestamp) and chrono-tz adjusts the effective timestamps through some adjustment in FixedTimespanSet::local_span. I don't understand why that works yet.

[Yuri6037]

Thanks, @Yuri6037. While version 0.7 indeed behaves as I'd expect for that one test case, it fails two other test cases now:

• for a local date time of 02:30, which is invalid in CET, the result is 2022-03-27T02:30+01:00.
• for a local date time of 03:30, which is after DST changeover, the result is 2022-03-27T03:30+01:00, but should be 2022-03-27T03:30+02:00

For naming, you might want to change the name of the method to something like assume_timezone_local, to prevent silent changes for people relying on the UTC offset behaviour now.

As for chrono-tz, it seems that NaiveDateTime::timestamp simply assumes UTC offset (https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDateTime.html#method.timestamp) and chrono-tz adjusts the effective timestamps through some adjustment in FixedTimespanSet::local_span. I don't understand why that works yet.

Thank you, I'll see what I can do. About the naming, it won't be a problem since this will be published under a new 1.0 release which should be completely independent from 0.6 (which is already issuing deprecation warning).

[Yuri6037]

@korrat I've now included all necessary changes and also, for the sake of completeness, I've added ambiguity checking.

All test cases you gave me are now passing.

EDIT: Please check the new result in the crates.io pre-release version 1.0.0-alpha-1. Docs have also been updated. Please review the changes in the associated PR #8

[korrat]

Can confirm, all cases I can think of are handled nicely. Thanks a lot for being so quick to fix this!

[Yuri6037]

The new version with all these fixes is now released under 1.0.0 on crates.io.