Make TimeZoneInfo more strict #5700

sffc · 2024-10-17T02:12:09Z

Instead of TimeZoneInfo having optional fields, it now makes the fields required via the type system, which prevents callers from passing in a time zone with missing fields to a formatter that needs more of the fields.

Manishearth

I'm a bit worried about the UX of this but timezones are gnarly anyway.

robertbastian

reviewed the timezone crate so far

robertbastian · 2024-10-17T16:34:45Z

components/timezone/src/ixdtf.rs

@@ -36,6 +34,8 @@ pub enum ParseError {
    InvalidOffsetError,
    /// There was an invalid IANA identifier provided.
    InvalidIanaIdentifier,


I don't think we ever want to raise InvalidIanaIdentifier, it might already be dead

robertbastian · 2024-10-17T16:36:55Z

components/timezone/src/ixdtf.rs

+        let offset = UtcOffset::try_from_utc_offset_record(offset)?;
+        let iso = DateTime::<Iso>::try_new_iso_datetime(
+            date.year,
+            date.month,
+            date.day,
+            time.hour,
+            time.minute,
+            time.second,
+        )?;


Nit: UtcOffset can construct itself from OffsetRecord, but DateTime cannot construct itself from DateRecord. Seems inconsistent.

Also, maybe create a Date and a Time instead of a DateTime, as that's what we need.

it's because DateTime is in a different crate and we can't export from-ixdtf-record functions across crate boundaries because ixdtf is less stable than icu_calendar.

robertbastian · 2024-10-17T16:39:17Z

components/timezone/src/ixdtf.rs

            IxdtfParseRecord {
                offset: Some(offset),
                tz: None,
+                date: Some(date),
+                time: Some(time),
                ..
-            } => Self::try_from_utc_offset_record(offset),
+            } => (offset, date, time),
+            // [-0800]
            IxdtfParseRecord {
                offset: None,
-                tz: Some(time_zone_annotation),
-                date,
-                time,
+                tz:
+                    Some(TimeZoneAnnotation {
+                        tz: TimeZoneRecord::Offset(offset),
+                        ..
+                    }),
+                date: Some(date),
+                time: Some(time),
                ..
-            } => Self::try_from_time_zone_record(&time_zone_annotation.tz, None, date, time),
+            } => (offset, date, time),


these cases can be collapsed

no? they cover different cases which are in comments

robertbastian · 2024-10-17T16:44:28Z

components/timezone/src/ixdtf.rs

-                            },
-                        );
+        let IxdtfParseRecord {
+            offset: None,


phew, not sure I want to make too much information an error. Imagine if you want to start sending more details across the network to change time zone format, if this errors you have to upgrade client and server at exactly the same time.

I think being strict about the expected fields is important. But, we can continue to iterate on exactly the shape of these functions and the strictness. In the mean time, I added more comments.

Imagine if you're calling an API that only guarantees that it sends the offset, but doesn't guarantee that it only sends the offset.

Perhaps that use case should be another constructor / parse function that sets the time zone to unk if it is not present. It seems bad to throw away the info, though.

There's currently no constructor here that does that.

Yes, it should be an error. The constructor you choose asserts the shape of the string.

Can we please not make this block the PR.

No this is important. Look at serde json, it does not complain when an object contains a key it doesn't know, it only complains when it's missing a key.

ok can I make it try_strict_from_str and then try_from_str is the loose one, as it is now

There are 4 functions which I think cover all of the use cases. This is how I have named them for now:

try_offset_only_iso_from_str (strict, returns UtcOffset)

try_location_only_iso_from_str (strict, returns TimeZoneInfo<AtTime>)

try_loose_iso_from_str (loose, returns TimeZoneInfo<AtTime>)

try_iso_from_str (strict, returns TimeZoneInfo<Full>)

There is only one "loose" function because it is only possible for a loose function to return TimeZoneInfo<AtTime>, not TimeZoneInfo<Full>.

I do not see a use case for a "loose" function returning UtcOffset because if you want things loose, just get a TimeZoneInfo<AtTime> which is more general and fully featured. The whole point, as we discussed today, of parsing to a UtcOffset is to assert that you do not have a location.

robertbastian · 2024-10-17T16:45:44Z

components/timezone/src/ixdtf.rs

+}
+
+impl TimeZoneInfo<models::Full> {
+    fn try_full_from_ixdtf_record(ixdtf_record: &IxdtfParseRecord) -> Result<Self, ParseError> {


nit: keep constructors for the same type together

Right now all the TimeZoneInfo constructors are together and the CustomZonedDateTime constructors are together.

robertbastian · 2024-10-17T16:50:18Z

components/timezone/src/lib.rs

@@ -90,6 +90,7 @@ extern crate alloc;
 mod error;
 mod ids;
 pub mod provider;
+pub mod scaffold;


#[doc(hidden)]?

But it is public, not just ICU4X-internal, and I really do not like #[doc(hidden)]. I'm happy to slap unstable warnings on the module and everything inside of it, similar to the provider modules.

robertbastian · 2024-10-17T16:50:59Z

components/timezone/src/lib.rs

 pub use time_zone::TimeZoneInfo;
+pub use time_zone::TimeZoneModel;


when does a user need access to this trait? I don't think we need it reexported.

It shows up in a public API signature and I do not like to not export things that show up publicly. I'm happy to export it from the scaffold module..

robertbastian · 2024-10-17T16:51:26Z

components/timezone/src/time_zone.rs

    /// This can be set in order to get correct historical time zone names.
    /// If it's not set, the most recent data for the time zone will be used.


not anymore

robertbastian · 2024-10-17T16:53:57Z

components/timezone/src/time_zone.rs

+    }
+
+    /// Sets a local time on this time zone.
+    pub const fn with_local_time(


Suggested change

pub const fn with_local_time(

pub const fn at_time(

robertbastian · 2024-10-17T16:54:47Z

components/timezone/src/time_zone.rs

+
+impl TimeZoneInfo<models::Full> {
+    /// Creates a new [`TimeZoneInfo`] for the UTC time zone
+    /// with a reference point at the UNIX Epoch.


mmh kinda don't want to document this. The reference point for UTC is completely irrelevant, because its metazone doesn't change.

I'll delete this function since it has only 1 call site.

I think it's still useful to have this as a public constructor

No actually I think this is a terrible function. For example, someone might make a TimeZoneInfo::utc_full as the only non-string constructor of TimeZoneInfo<Full> and then try to overwrite fields on it.

This function clearly is not that useful because there is only 1 call site and it is a weird call site in a match statement. There are no call sites in normal code.

sffc · 2024-10-18T02:21:30Z

Ok I think this is ready now. Only thing I didn't do 100% from your feedback is the full deduplication of code in ixdtf.rs, but it is better than it was before.

robertbastian · 2024-10-29T14:42:04Z

#5533

sffc added 6 commits October 16, 2024 19:00

Initial commit of strict TimeZoneInfo

438c7e5

Fixes and tests in the timezone crate

c8c89fa

Most tests passing

f76fba8

Fix!

876e476

Fix expectation of newly added marker

51cdfef

More illustrative test, and add specific combo marker

311444d

sffc marked this pull request as ready for review October 17, 2024 02:12

sffc requested review from Manishearth, robertbastian, zbraniecki and a team as code owners October 17, 2024 02:12

sffc removed request for a team and zbraniecki October 17, 2024 02:12

Manishearth previously approved these changes Oct 17, 2024

View reviewed changes

robertbastian reviewed Oct 17, 2024

View reviewed changes

More comments about strictness parsing behavior

e370305

sffc dismissed Manishearth’s stale review via e370305 October 17, 2024 21:00

sffc added 13 commits October 17, 2024 14:38

Merge branch 'main' into strict-tzinfo-2

e792014

Update ixdtf.rs

45f28a2

Update simple_test.rs

b57cca7

Update neo_marker.rs

9336175

diplomat-gen

35807ec

fmt

32bc147

allowlist scaffold

a8c10fe

rm InvalidIanaIdentifier

2834a83

Re-do time zone types and parsing strictness

e5126c1

Make things build

3c0357c

Fix docs tests

c0fdea2

Make loose looser

3059722

Should be Pacific/Honolulu

24d1802

fmt

ea3ce98

robertbastian previously approved these changes Oct 18, 2024

View reviewed changes

sffc added 5 commits October 17, 2024 19:08

at_time

1a1c040

Delete obsolete comment

e05f53c

rm utc_full

e03782c

diplomat-coverage

346443a

fix datetime.cpp

9883920

sffc dismissed robertbastian’s stale review via 9883920 October 18, 2024 02:17

docs links

be305c5

sffc requested a review from robertbastian October 18, 2024 02:21

robertbastian approved these changes Oct 18, 2024

View reviewed changes

sffc merged commit 5ad31bf into unicode-org:main Oct 18, 2024
28 checks passed

sffc deleted the strict-tzinfo-2 branch October 18, 2024 03:26

sffc mentioned this pull request Oct 18, 2024

Consider not using Etc/Unknown time zone #5653

Closed

		pub use time_zone::TimeZoneInfo;
		pub use time_zone::TimeZoneModel;

		/// This can be set in order to get correct historical time zone names.
		/// If it's not set, the most recent data for the time zone will be used.

Make TimeZoneInfo more strict #5700

Make TimeZoneInfo more strict #5700

Uh oh!

Conversation

sffc commented Oct 17, 2024

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian Oct 17, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Oct 18, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc commented Oct 18, 2024

Uh oh!

Uh oh!

robertbastian commented Oct 29, 2024

robertbastian Oct 17, 2024 •

edited

Loading

sffc Oct 18, 2024 •

edited

Loading