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

Enforce FixedOffset is a multiple of 60 #1036

Closed

Conversation

pitdicker
Copy link
Collaborator

Offsets from UTC are defined in whole hours and minutes, never with seconds.
FixedOffset stores the offset from UTC in seconds, which makes sense because it allows for fast calculations.

I propose to enforce the value of FixedOffset is always a multiple of 60.
This prevents a number of existing or potential problems:

  • RFC 2822 and RFC 3339 don't allow seconds. Chrono currently can create invalid strings with offset formatted as hh::mm::ss.
  • The resulting string can not be parsed by chrono.
  • This also effects serialization with serde.
  • Leap seconds should always occur after the 59th second. Having an offset with seconds can place it after any other second, making a mess.

I mainly want to make this change because it prevents those problems.
Enforcing FixedOffset is always a multiple of 60 enables a nice possibility: it can fit inside an i16 if we shift the value 2 bits (which we can, 60 is divisable by 4). This creates room for niche optimization: <Option<DateTime<FixedOffset>>> now only requires 16 bytes instead of 20.

Could this effect users of chrono?
It is pretty much a rule that whenever you start checking something you didn't before, it catches some problems. Because Unix, Windows and the IANA timezone database don't allow seconds to define offsets, and because chrono can barely parse them (only with the %::z specifier), I don't expect any issues in real-world uses. But it may trip up some artificial test.

@pitdicker
Copy link
Collaborator Author

Haha, the CI pointed out a doctest that used FixedOffset::east_opt(23), which is not a great example.
Changed it to FixedOffset::east_opt(13 * 60 * 60) (Kiribati and others).

@pitdicker
Copy link
Collaborator Author

Hmmm. RFC3339 says:

NOTE: Following ISO 8601, numeric offsets represent only time zones that differ from UTC by an integral number of minutes. However, many historical time zones differ from UTC by a non-integral number of minutes. To represent such historical time stamps exactly, applications must convert them to a representable time zone.

😞

I will keep this PR open for a little longer for comments.

@djc
Copy link
Member

djc commented Apr 26, 2023

NOTE: Following ISO 8601, numeric offsets represent only time zones that differ from UTC by an integral number of minutes. However, many historical time zones differ from UTC by a non-integral number of minutes. To represent such historical time stamps exactly, applications must convert them to a representable time zone.

Yeah, I would say that, for chrono's goals, historical timestamps are definitely within scope and we should eat the performance trade-offs (unfortunately).

But, given that FixedOffset is merely just one type implementing Tz, we could come up with a different one that is similar in most respects but different in this one?

@pitdicker
Copy link
Collaborator Author

I am going to work a bit on formatting and parsing offsets. And there is more than one way to create a type with niche. That fits with another experiment. Another attempt in the next days 😄.

@pitdicker pitdicker closed this Apr 27, 2023
@pitdicker pitdicker deleted the fixedoffset_multiple_of_60 branch May 4, 2023 07:56
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

Successfully merging this pull request may close these issues.

2 participants