Add additional checks for AlternateTime

[x-hgg-x]

Thanks for contributing to chrono!

Please consider adding a test to ensure your bug fix/feature will not break in the future.

This PR adds additional checks for the AlternateTime struct in the tz-info module, not existing in the initial implementation (see #677 (comment)).

[esheppa]

Thanks for this @x-hgg-x - I'll need a bit of time to understand this better, however one thing I noticed was:

/// Check DST transition rules consistency, which ensures that the DST start and end time are always in the same order.

Is this empirically always the case in the IANA tzdb? From memory while testing I'd come across cases where there were regular transitions that switched orders, although potentially these checks only apply to the rules that are used once all the hard-coded transition times are exhausted?

[x-hgg-x]

Yes all the rules in the IANA database pass the check. In the case that a new version of the database adds an invalid rule, the tzdb crate will not compile since the check is done at compile time in tz-rs, so I will be notified.
I think this is unlikely to happen since in this case, the resulting rule is ill-defined and not covered by the POSIX specification.

For information, if the order of the DST start and end time is switched between consecutive years, the current behavior of glibc and musl is to have a new transition at the new year on UTC (not on local time like the other transitions defined by the rule):

export TZ="STD5DST,J87,M3.5.0"

date --date=@1609459199 +"%F %T %:z %Z"
date --date=@1609459199 +"%F %T %:z %Z" --utc
echo
date --date=@1609459200 +"%F %T %:z %Z"
date --date=@1609459200 +"%F %T %:z %Z" --utc

Output:

2020-12-31 18:59:59 -05:00 STD
2020-12-31 23:59:59 +00:00 UTC

2020-12-31 20:00:00 -04:00 DST
2021-01-01 00:00:00 +00:00 UTC

[x-hgg-x]

these checks only apply to the rules that are used once all the hard-coded transition times are exhausted

Yes the check only applies to the extra rule after the last transition, defined by a POSIX TZ string.

[esheppa]

Yes all the rules in the IANA database pass the check. In the case that a new version of the database adds an invalid rule, the tzdb crate will not compile since the check is done at compile time in tz-rs, so I will be notified.

With regards to this - would you mind adding a test that parses all the available files in the systems timezone database? That way it runs in the CI here, meaning we would be notified here if the validation is causing an issue. (Separately, I'll schedule the CI to run at least once a day on main which would probably be useful as well)

[x-hgg-x]

would you mind adding a test that parses all the available files in the systems timezone database

Done in e4ebfe4.

[djc]

If we're "statically" validating that the tzdb zones pass some particular test, do we still need run-time verification? Seems like overkill to me.

[esheppa]

One potential benefit of the runtime validation is that we could then make more assumptions about the structure of the files and therefore change fallible functions to be non-fallible, provided the validation has already passed, which might simplify some of the downstream code.

[x-hgg-x]

If we're "statically" validating that the tzdb zones pass some particular test, do we still need run-time verification? Seems like overkill to me.

I don't think there is any compile-time validation of the tzdb zones at the moment in the chrono repository.

[esheppa]

Perhaps we can have a feature (enabled by default) that does the runtime validation? Then for users who need better performance they could disable this (with the caveat that dependencies may cause it to be engaged anyway). We already do some runtime validation, so that could potentially be later gated by that feature as well. Eventually some of these validation rules could potentially be shared with parse-zoneinfo as well (while it operates on a different input file format, the parsed structures may be able to be harmonized)

[x-hgg-x]

A quick benchmark on my PC for the test_read_all_tz_files() test shows that the check_dst_transition_rules_consistency() validation check represents less than 1% of the runtime of the TimeZone::from_tz_data() function, so I think it is ok even if we don't put the check behind a feature.