Timezone-aware datetime comparisons ignore the timezone

[lovasoa]

In the current version of chrono, timezone-aware datetime comparisons ignore the timezone.

https://github.com/chronotope/chrono/blob/d754c5c7dda975869d7672de0cf370e376f27305/src/datetime/mod.rs#L1167-L1171

(the timezone should be compared here too)

This means that

DateTime::parse_from_rfc3339("2016-10-23T12:45:37+02:00") == DateTime::parse_from_rfc3339("2016-10-23T12:45:37+07:00") 

even though these represent completely different times.

It just took me 2 hours to realise that the bug was not in my code but in chrono itself

[pitdicker]

Yes, I was somewhat surprised by this also, see https://github.com/chronotope/chrono/pull/1088#pullrequestreview-1448084460.

In many cases it makes sense that DateTime's can be compared/ordered independent of the timezone, but in some cases you also want to ensure they have the same offset from UTC or even the same timezone.

In any case this is not a bug, it is a design choice. Changing it would break the code of others.

We can work on improving the documentation here.

[djc]

We should definitely change this in 0.5 IMO, even if it's just removing the PartialEq implementation (or, more likely, restricting it to instances that have the same Tz type, with the timezone values also being included in the comparison).

[lovasoa]

Oh, I didn't expect this to be intentional ! Are there cases in which it makes sense for two datetimes that have the same hour, but in different timezones to be considered equal? I'm struggling to imagine an application in which this behavior would be desired.

I think few people read the docs of the PartialEq implementation, because it's generally obvious. So changing the documentation wouldn't really solve the problem. At least in my case, I spent two hours searching for a bug in my code, because I couldn't imagine two different dates being equal in PartialEq.

[djc]

I agree that it's a surprising choice, and not one that I would have made (but it was made before I got involved with this library). But due to compatibility, we can't easily change it now.

[pitdicker]

Are there cases in which it makes sense for two datetimes that have the same hour, but in different timezones to be considered equal? I'm struggling to imagine an application in which this behavior would be desired.

Maybe you already realize this, but just in case: the datetime field that is compared is in UTC, independent of the timezone.

Values compare equal when they describe the same moment in time. And it doesn't matter for the comparison if a type includes the full timezone name, only an offset, is normalized to UTC, or some other format. I.e. 2023-08-03 18:00 +02:00 compares equal to 2023-08-03 16:00 UTC.

I don't have strong opinion on if this is something that should change.

[pitdicker]

@lovasoa Do you already have a workaround?

What would be the comparison you need? Should besides the timestamp the offset be equal, the type, of both?

[lovasoa]

In my case, the problem occurred in the tests, allowing the tests to pass even when the code was returning completely incoherent timezones. Currently my only workaround is to be careful and to keep in mind that the tests won't catch this kind of errors.

I think the most basic thing, just testing that everything is exactly equal (and of the same type), would make the most sense.

I would not mind the equality code trying to be smart about timezones, though. But just ignoring them completely is a real footgun.

[msdrigg]

This occurred in my code where I assumed cross-tz date time <= would compare the actual instant. This caused some problems, but it was resolved by a ".with_timezone(&Utc)" on both sides of the comparison.

[pitdicker]

@msdrigg Maybe you want to double-check? Because chrono does compare the actual instant, all values are converted to UTC internally.

[msdrigg]

Ahh... well my bad. I saw

    fn partial_cmp(&self, other: &DateTime<Tz2>) -> Option<Ordering> {
        self.datetime.partial_cmp(&other.datetime)
    }

and searched for "datetime comparison issue chrono" and assumed it was a problem as soon as I found this issue. I'll go find the bug in my own code now...