Preserve `-00:00` offset by pitdicker · Pull Request #1042 · chronotope/chrono

pitdicker · 2023-04-28T19:30:54Z

RFC 3339 and RFC 2822 make a distinction between an offset of +00:00 and -00:00.

RFC 2822:

The form "+0000" SHOULD be used to indicate a time zone at Universal Time. Though "-0000" also indicates Universal Time, it is used to indicate that the time was generated on a system that may be in a local time zone other than Universal Time and therefore indicates that the date-time contains no information about the local time zone.

From RFC3339:

If the time in UTC is known, but the offset to local time is unknown, this can be represented with an offset of "-00:00". This differs semantically from an offset of "Z" or "+00:00", which imply that UTC is the preferred reference point for the specified time.

Currently chrono drops the distinction, but I would like to preserve it.

I noticed format/scan.rs was already written with the functionality of this RFCs in mind, but did not go all the way. It now follows the specs better and can sometimes give better parser errors.
Modifying Parsed to preserve this information is a bit messy as all the methods are public. Adding a public constant NO_OFFSET_INFO seemed the cleanest solution. Parsing doesn't support more than two digits for the offset hours, limiting the values that are passed to Parsed::set_offset to 99 * 3600 + 59 * 60 = 359940. The value of NO_OFFSET_INFO is way outside that range, so no chance for conflicts.
FixedOffset can be modified in a reasonably clean way to encode -0: shift the value 2 bits left and encode this special state as an OFFSET_UNKNOWN flag in the rightmost bits. Advantage of this encoding is that it only takes a single shift right to get the offset in seconds, so this should not measurably impact performance.
The same shifting trick enables using NonZeroI32, as long as we set one of the rightmost bits to something non-null with the OFFSET_NORMAL flag. This gives FixedOffset a niche. Option<DateTime<FixedOffset>> is now the same size as DateTime<FixedOffset>: 16 bytes instead of 20.
For comparisons and hashing the flags are ignored. This way -00:00 is treated as equal to +00:00, which is usually what is expected. Also this way we remain backwards compatible.
FixedOffset::no_offset_info returns whether the offset is -00:00.
FixedOffset::OFFSET_UNKNOWN can be used to initialize an offset to -00:00.

src/format/parse.rs

djc · 2023-04-28T20:00:15Z

Currently chrono drops the distinction, but I would like to preserve it.

Do you have an actual use case for this?

pitdicker · 2023-04-28T20:04:42Z

Ai, why do you have to ask that 🤣 ?
As a hobby I am making my own feed reader, and I would like to know the time an entry was published by the author in his own timezone. If the timestamp indicates it knows the time in UTC but the relation to the local time is unknown, I find that interesting. But currently chrono offers no easy way to know.

pitdicker · 2023-04-28T20:06:04Z

As a better reason: we would follow the RFCs more closely, and be able to round-trip these timestamps.

djc · 2023-04-28T20:11:39Z

In general, I would concur we want to follow the RFCs closely. But at the same time, "you find it interesting" to know this doesn't seem like a strong justification for adding ~90 lines of code on net.

pitdicker · 2023-04-28T20:38:18Z

That is fair.

Honestly the changes felt small, so i did some counting:

17 lines for testing the size of common types in chrono.
30 would also be there if we wanted to use the part of encoding an offset in an NonZeroI32 to get the niche optimization.
that leaves ca 45 netto for the feature and documentation

pitdicker · 2023-04-28T20:41:57Z

src/format/parse.rs

        ("Tue, 20 Jan 2015 17:35:20 -0890", Err(OUT_OF_RANGE)), // bad offset
        ("6 Jun 1944 04:00:00Z", Err(INVALID)),            // bad offset (zulu not allowed)
-        ("Tue, 20 Jan 2015 17:35:20 HAS", Err(NOT_ENOUGH)), // bad named time zone
+        ("Tue, 20 Jan 2015 17:35:20 HAS", Err(INVALID)),   // bad named time zone


We would now report the correct error here.

pitdicker · 2023-04-28T20:42:12Z

src/format/parse.rs

+        ("Tue, 20 Jan 2015 17:35:20 k", Ok("Tue, 20 Jan 2015 17:35:20 -0000")),
        // named single-letter timezone "J" is specifically not valid
-        ("Tue, 20 Jan 2015 17:35:20 J", Err(NOT_ENOUGH)),
+        ("Tue, 20 Jan 2015 17:35:20 J", Err(INVALID)),


jtmoon79

I'm very compelled by this standards-following PR. I thought something wasn't quite right about how the various RFC and +00:00 and -00:00 were being handled but I couldn't put my finger on it (and it seemed like a difficult bramblebush to crawl through).

Thanks much @pitdicker , this is a great change!

src/format/parse.rs

jtmoon79 · 2023-04-29T22:11:10Z

src/format/parse.rs

        ("Tue, 20 Jan 2015 17:35:20 -0890", Err(OUT_OF_RANGE)), // bad offset
        ("6 Jun 1944 04:00:00Z", Err(INVALID)),            // bad offset (zulu not allowed)
-        ("Tue, 20 Jan 2015 17:35:20 HAS", Err(NOT_ENOUGH)), // bad named time zone
+        ("Tue, 20 Jan 2015 17:35:20 HAS", Err(INVALID)),   // bad named time zone


Can you add extra asserts

("20 Jan 2015 17:35:20 +0000", Ok("Tue, 20 Jan 2015 17:35:20 +0000")), ("20 Jan 2015 17:35:20 -0001", Ok("Tue, 20 Jan 2015 17:35:20 -0001")), ("Tue, 20 Jan 2015 17:35:20 -9900", Err(OUT_OF_RANGE)), // bad offset

(adjusted to whatever the correct value is)

src/format/scan.rs

jtmoon79 · 2023-04-29T22:26:56Z

src/lib.rs

+
+#[test]
+#[allow(deprecated)]
+fn test_type_sizes() {


I'm curious if this has been tested on an x86 (32 bit) platform?

IME FreeBSD x86 and OpenBSD in-general is always little weird about sizings of things (often subtle differences from Linux anyway).

If I get around to it, I'll try to compile this PR on a FreeBSD 13 x86 VM.

jtmoon79 · 2023-04-29T22:30:16Z

src/offset/fixed.rs

+    //    of +0:00 and -0:00.
+    // Advantage of this encoding is that it only takes a single shift right to get offset in
+    // seconds.
+    local_minus_utc: NonZeroI32,


Can you make this code comment a docstring?
In the docstring, would you tell the user what helper functions they should call to extract various infos? Can you provide those helper functions? Can you have this PR use those bit-shift helper functions?
Even if it's only chrono developers that need to know, (even if those docstrings aren't actually published) IMO it's still worthwhile to write it down in "official-seeming" manner.

Otherwise I might assume I have to do the bit shifts and masks myself... and those are programming approaches I would like everyone to move on from. 😬

On the other hand ...

honestly, I'd prefer to have discrete struct members for each piece of information embedded here. I'm not persuaded the runtime efficiency-savings is worth the mental overhead.

Also, I'm not sure, but I'm concerned it might be a problem on less-common Big Endian platforms (I don't recall where and when Rust gives guarantees around bit endianness).

(IIUC, combining various information into one NonZeroI32 member is only done for a little less memory usage at runtime?)

honestly, I'd prefer to have discrete struct members for each piece of information embedded here. I'm not persuaded the runtime efficiency-savings is worth the mental overhead.

This is somewhat comparable to NaiveDate, which fits a year, ordinal, leap-year flag and weekday flags in an i32.

I agree that using individual struct members is usually better. But chrono's DateTime<FixedOffset> is a pretty fundamental type in the rust ecosystem. It should not increase in size without very good reason.

Also, I'm not sure, but I'm concerned it might be a problem on less-common Big Endian platforms (I don't recall where and when Rust gives guarantees around bit endianness).

In safe Rust, we do not have to worry about this. As soons as we start transmuting it becomes messy.

Can you make this code comment a docstring?
In the docstring, would you tell the user what helper functions they should call to extract various infos? Can you provide those helper functions? Can you have this PR use those bit-shift helper functions?
Even if it's only chrono developers that need to know, (even if those docstrings aren't actually published) IMO it's still worthwhile to write it down in "official-seeming" manner.

I would like to keep the implementation details private.
But we definitely have helper functions which this PR uses: local_minus_utc() and no_offset_info().

I'll mention them in a comment:

// Use `local_minus_utc()` to get the offset in seconds, and `no_offset_info()` to inspect the // `OFFSET_UNKNOWN` flag.

src/offset/fixed.rs

jtmoon79 · 2023-04-29T22:38:26Z

RFC 3339 and RFC 2822 make a distinction between an offset of +00:00 and -00:00.

RFC 2822:

The form "+0000" SHOULD be used to indicate a time zone at Universal Time. Though "-0000" also indicates Universal Time, it is used to indicate that the time was generated on a system that may be in a local time zone other than Universal Time and therefore indicates that the date-time contains no information about the local time zone.

From RFC3339:

If the time in UTC is known, but the offset to local time is unknown, this can be represented with an offset of "-00:00". This differs semantically from an offset of "Z" or "+00:00", which imply that UTC is the preferred reference point for the specified time.

Currently chrono drops the distinction, but I would like to preserve it.
* I noticed `format/scan.rs` was already written with the functionality of this RFCs in mind, but did not go all the way. It now follows the specs better and can sometimes give better parser errors.

* Modifying `Parsed` to preserve this information is a bit messy as all the methods are public. Adding a public constant `NO_OFFSET_INFO` seemed the cleanest solution. Parsing doesn't support more than two digits for the offset hours, limiting the values that are passed to `Parsed::set_offset` to `99 * 3600 + 59 * 60 = 359940`. The value of `NO_OFFSET_INFO` is way outside that range, so no chance for conflicts.

* `FixedOffset` can be modified in a reasonably clean way to encode `-0`: shift the value 2 bits left and encode this special state as an `OFFSET_UNKNOWN` flag in the rightmost bits. Advantage of this encoding is that it only takes a single shift right to get the offset in seconds, so this should not measurably impact performance.

* The same shifting trick enables using `NonZeroI32`, as long as we set one of the rightmost bits to something non-null with the `OFFSET_NORMAL` flag. This gives `FixedOffset` a niche. `Option<DateTime<FixedOffset>>` is now the same size as `DateTime<FixedOffset>`: 16 bytes instead of 20.

* For comparisons and hashing the flags are ignored. This way `-00:00` is treated as equal to `+00:00`, which is usually what is expected. Also this way we remain backwards compatible.

* `FixedOffset::no_offset_info` returns whether the offset is `-00:00`.

* `FixedOffset::OFFSET_UNKNOWN` can be used to initialize an offset to `-00:00`.

Can you find an appropriate docstring to place this information (especiially the quoted RFC stuff)? This is a great explanation of little finicky area that I suspect quite a few users will run into.
You might find it appropriate to split up the info and place within various user-facing functions. Whatever you think is good. So long as this subtlety is explained to the user.

esheppa · 2023-04-30T13:30:08Z

Interestingly, this was a common case that was causing failures in tests/dateutils.rs::verify_against_date_command_format_local, if this is merged, we may be able to ensure the offsets match as well, rather than just the date/time values.

pitdicker · 2023-05-04T09:12:01Z

Can you find an appropriate docstring to place this information (especially the quoted RFC stuff)? This is a great explanation of little finicky area that I suspect quite a few users will run into.
You might find it appropriate to split up the info and place within various user-facing functions. Whatever you think is good. So long as this subtlety is explained to the user.

Good idea, I have written an introduction for the FixedOffset type, and included a small reference in the docstring of the Parsed::offset field.

pitdicker · 2023-05-04T09:28:51Z

Interestingly, this was a common case that was causing failures in tests/dateutils.rs::verify_against_date_command_format_local, if this is merged, we may be able to ensure the offsets match as well, rather than just the date/time values.

Great if this would help with an existing test 😄.

src/lib.rs

src/datetime/tests.rs

jtmoon79

Approve still, with suggestion.

src/offset/fixed.rs

codecov · 2024-02-04T16:27:15Z

Codecov Report

Attention: Patch coverage is 91.95402% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 92.16%. Comparing base (e292d9b) to head (cc18784).

Files	Patch %	Lines
src/offset/fixed.rs	79.31%	6 Missing ⚠️
src/format/parsed.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1042   +/-   ##
=======================================
  Coverage   92.16%   92.16%           
=======================================
  Files          40       40           
  Lines       18052    18101   +49     
=======================================
+ Hits        16637    16683   +46     
- Misses       1415     1418    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pitdicker force-pushed the no_offset_info branch from e044499 to e42b18a Compare April 28, 2023 19:35

pitdicker commented Apr 28, 2023

View reviewed changes

src/format/parse.rs Outdated Show resolved Hide resolved

pitdicker force-pushed the no_offset_info branch from e42b18a to d357688 Compare April 28, 2023 19:39

pitdicker changed the title ~~No offset info~~ Preserve -00:00 offset Apr 28, 2023

pitdicker force-pushed the no_offset_info branch from d357688 to d2d5956 Compare April 28, 2023 19:49

pitdicker commented Apr 28, 2023

View reviewed changes

jtmoon79 suggested changes Apr 29, 2023

View reviewed changes

pitdicker mentioned this pull request May 2, 2023

exploration with the timezone trait #830

Closed

pitdicker force-pushed the no_offset_info branch 3 times, most recently from c31316b to e070c8a Compare May 4, 2023 09:15

pitdicker force-pushed the no_offset_info branch 3 times, most recently from 60c0f59 to f6f0495 Compare May 10, 2023 11:54

pitdicker force-pushed the no_offset_info branch 3 times, most recently from f59ed7b to d82ca0e Compare May 17, 2023 15:21

djc reviewed May 19, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/datetime/tests.rs Show resolved Hide resolved

pitdicker force-pushed the no_offset_info branch 2 times, most recently from fd30238 to 5ecc449 Compare May 19, 2023 15:07

jtmoon79 approved these changes May 24, 2023

View reviewed changes

src/offset/fixed.rs Outdated Show resolved Hide resolved

pitdicker force-pushed the no_offset_info branch from 53af4d5 to 9b8d91c Compare May 24, 2023 12:44

djc added the waiting-on-review label May 26, 2023

pitdicker force-pushed the no_offset_info branch from 9b8d91c to af9a12e Compare June 1, 2023 09:04

pitdicker force-pushed the no_offset_info branch 3 times, most recently from a289068 to af2da5a Compare June 12, 2023 12:43

pitdicker force-pushed the no_offset_info branch 6 times, most recently from 7582a3f to efe03c9 Compare July 1, 2023 18:52

pitdicker force-pushed the no_offset_info branch from efe03c9 to 68a7c7a Compare July 20, 2023 05:17

pitdicker force-pushed the no_offset_info branch from 68a7c7a to bdd511e Compare July 27, 2023 17:48

pitdicker force-pushed the no_offset_info branch from bdd511e to 0f13ac6 Compare February 4, 2024 16:23

pitdicker mentioned this pull request Feb 4, 2024

Remove partial support for handling -0000 offset #1411

Merged

pitdicker force-pushed the no_offset_info branch from 0f13ac6 to d1cbfea Compare February 7, 2024 13:07

pitdicker force-pushed the no_offset_info branch from d1cbfea to 9f0ee28 Compare February 28, 2024 13:22

pitdicker added 4 commits February 28, 2024 14:23

Use local_minus_utc() to get offset

801a362

Support encoding -00:00 in FixedOffset

bcf9057

Support formatting an offset of -00:00

739db32

Some more documentation for FixedOffset

2dbffae

pitdicker mentioned this pull request Feb 28, 2024

Parsed fixes and documentation #1439

Merged

pitdicker added 3 commits February 28, 2024 14:37

Support encoding -00:00 in Parsed::offset

e9796fe

Support parsing -00:00

431adde

Parse invalid single-letter timezones as -00:00

cc18784

pitdicker force-pushed the no_offset_info branch from 9f0ee28 to cc18784 Compare February 28, 2024 13:38

pitdicker marked this pull request as draft February 28, 2024 15:42

Conversation

pitdicker commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

djc commented Apr 28, 2023

Uh oh!

pitdicker commented Apr 28, 2023

Uh oh!

pitdicker commented Apr 28, 2023

Uh oh!

djc commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitdicker commented Apr 28, 2023

Uh oh!

pitdicker Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

pitdicker Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

jtmoon79 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtmoon79 Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtmoon79 Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtmoon79 Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitdicker May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitdicker May 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtmoon79 commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esheppa commented Apr 30, 2023

Uh oh!

pitdicker commented May 4, 2023

Uh oh!

pitdicker commented May 4, 2023

Uh oh!

Uh oh!

Uh oh!

jtmoon79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitdicker commented Apr 28, 2023 •

edited

Loading

djc commented Apr 28, 2023 •

edited

Loading

jtmoon79 left a comment •

edited

Loading

jtmoon79 Apr 29, 2023 •

edited

Loading

jtmoon79 Apr 29, 2023 •

edited

Loading

pitdicker May 4, 2023 •

edited

Loading

jtmoon79 commented Apr 29, 2023 •

edited

Loading

codecov bot commented Feb 4, 2024 •

edited

Loading