Make time zone parsing non-fallible (#5665)

Based on #5666 Zones that are added to TZDB in the future should parse as `Etc/Unknown`. This means every unrecognized string should parse as `Etc/Unknown`. This then means that the `time_zone_id` field can be non-optional, as we can always use `Etc/Unknown` as the absent value. The `metazone_id` field on the other hand need to be double optional: once for not-resolved, and once for no-metazone-for-zone. This change improves error cases in datetime formatting, i.e. `Some(None)` is a UTS-35 fallback, whereas `None` is a `MissingInputField`.
unicode-org · Oct 15, 2024 · 3a95556 · 3a95556
1 parent 23dc5b2
commit 3a95556
Show file tree

Hide file tree

Showing 52 changed files with 342 additions and 938 deletions.
diff --git a/components/datetime/src/format/neo.rs b/components/datetime/src/format/neo.rs
@@ -382,7 +382,7 @@ size_test!(
 /// // Further, the time zone we provide doesn't contain any offset into!
 /// // Missing data is filled in on a best-effort basis, and an error is signaled.
 /// assert_try_writeable_parts_eq!(
-///     names.with_pattern(&pattern).format(&CustomTimeZone::new_empty()),
+///     names.with_pattern(&pattern).format(&CustomTimeZone::unknown()),
 ///     "It is: {E} {M} {d} {y} {G} at {h}:{m}:{s} {a} {GMT+?}",
 ///     Err(DateTimeWriteError::MissingInputField("iso_weekday")),
 ///     [

diff --git a/components/datetime/src/input.rs b/components/datetime/src/input.rs
@@ -28,7 +28,7 @@ pub(crate) struct ExtractedInput {
     pub(crate) nanosecond: Option<NanoSecond>,
     pub(crate) offset: Option<UtcOffset>,
     pub(crate) time_zone_id: Option<TimeZoneBcp47Id>,
-    pub(crate) metazone_id: Option<MetazoneId>,
+    pub(crate) metazone_id: Option<Option<MetazoneId>>,
     pub(crate) zone_variant: Option<ZoneVariant>,
 }
 

diff --git a/components/datetime/src/neo_marker.rs b/components/datetime/src/neo_marker.rs
@@ -302,7 +302,7 @@
 //!
 //! // "uschi" - has metazone symbol data for generic_non_location_short
 //! let mut time_zone = "-0600".parse::<CustomTimeZone>().unwrap();
-//! time_zone.time_zone_id = mapper.as_borrowed().iana_to_bcp47("America/Chicago");
+//! time_zone.time_zone_id = mapper.as_borrowed().iana_to_bcp47("America/Chicago").unwrap();
 //! time_zone.maybe_calculate_metazone(&mzc, &datetime);
 //! assert_try_writeable_eq!(
 //!     tzf.format(&time_zone),
@@ -311,7 +311,7 @@
 //!
 //! // "ushnl" - has time zone override symbol data for generic_non_location_short
 //! let mut time_zone = "-1000".parse::<CustomTimeZone>().unwrap();
-//! time_zone.time_zone_id = Some(TimeZoneBcp47Id(tinystr!(8, "ushnl")));
+//! time_zone.time_zone_id = TimeZoneBcp47Id(tinystr!(8, "ushnl"));
 //! time_zone.maybe_calculate_metazone(&mzc, &datetime);
 //! assert_try_writeable_eq!(
 //!     tzf.format(&time_zone),
@@ -320,7 +320,7 @@
 //!
 //! // If we don't calculate the metazone, it falls back to generic location
 //! let mut time_zone = "-1000".parse::<CustomTimeZone>().unwrap();
-//! time_zone.time_zone_id = Some(TimeZoneBcp47Id(tinystr!(8, "ushnl")));
+//! time_zone.time_zone_id = TimeZoneBcp47Id(tinystr!(8, "ushnl"));
 //! assert_try_writeable_eq!(
 //!     tzf.format(&time_zone),
 //!     "Honolulu Time"
@@ -753,15 +753,15 @@ impl<C: Calendar, A: AsCalendar<Calendar = C>> NeoGetField<Option<TimeZoneBcp47I
 {
     #[inline]
     fn get_field(&self) -> Option<TimeZoneBcp47Id> {
-        self.zone.time_zone_id
+        Some(self.zone.time_zone_id)
     }
 }
 
-impl<C: Calendar, A: AsCalendar<Calendar = C>> NeoGetField<Option<MetazoneId>>
+impl<C: Calendar, A: AsCalendar<Calendar = C>> NeoGetField<Option<Option<MetazoneId>>>
     for CustomZonedDateTime<A>
 {
     #[inline]
-    fn get_field(&self) -> Option<MetazoneId> {
+    fn get_field(&self) -> Option<Option<MetazoneId>> {
         self.zone.metazone_id
     }
 }
@@ -785,13 +785,13 @@ impl NeoGetField<Option<UtcOffset>> for CustomTimeZone {
 impl NeoGetField<Option<TimeZoneBcp47Id>> for CustomTimeZone {
     #[inline]
     fn get_field(&self) -> Option<TimeZoneBcp47Id> {
-        self.time_zone_id
+        Some(self.time_zone_id)
     }
 }
 
-impl NeoGetField<Option<MetazoneId>> for CustomTimeZone {
+impl NeoGetField<Option<Option<MetazoneId>>> for CustomTimeZone {
     #[inline]
-    fn get_field(&self) -> Option<MetazoneId> {
+    fn get_field(&self) -> Option<Option<MetazoneId>> {
         self.metazone_id
     }
 }
@@ -927,7 +927,7 @@ impl From<NeverField> for Option<TimeZoneBcp47Id> {
     }
 }
 
-impl From<NeverField> for Option<MetazoneId> {
+impl From<NeverField> for Option<Option<MetazoneId>> {
     #[inline]
     fn from(_: NeverField) -> Self {
         None
@@ -1061,7 +1061,7 @@ pub trait ZoneMarkers: private::Sealed {
     /// Marker for resolving the time zone id input field.
     type TimeZoneIdInput: Into<Option<TimeZoneBcp47Id>>;
     /// Marker for resolving the time zone metazone input field.
-    type TimeZoneMetazoneInput: Into<Option<MetazoneId>>;
+    type TimeZoneMetazoneInput: Into<Option<Option<MetazoneId>>>;
     /// Marker for resolving the time zone variant input field.
     type TimeZoneVariantInput: Into<Option<ZoneVariant>>;
     /// Marker for loading core time zone data.
@@ -1549,7 +1549,7 @@ macro_rules! datetime_marker_helper {
         Option<TimeZoneBcp47Id>
     };
     (@input/timezone/metazone, yes) => {
-        Option<MetazoneId>
+        Option<Option<MetazoneId>>
     };
     (@input/timezone/variant, yes) => {
         Option<ZoneVariant>

diff --git a/components/datetime/src/time_zone.rs b/components/datetime/src/time_zone.rs
@@ -279,19 +279,19 @@ impl FormatTimeZone for GenericNonLocationFormat {
         let Some(time_zone_id) = input.time_zone_id else {
             return Ok(Err(FormatTimeZoneError::MissingInputField("time_zone_id")));
         };
-
         let Some(names) = (match self.0 {
             FieldLength::Wide => data_payloads.mz_generic_long.as_ref(),
             _ => data_payloads.mz_generic_short.as_ref(),
         }) else {
             return Ok(Err(FormatTimeZoneError::MissingZoneSymbols));
         };
 
-        let Some(name) = names
-            .overrides
-            .get(&time_zone_id)
-            .or_else(|| names.defaults.get(&metazone_id))
-        else {
+        let Some(name) = metazone_id.and_then(|mz| {
+            names
+                .overrides
+                .get(&time_zone_id)
+                .or_else(|| names.defaults.get(&mz))
+        }) else {
             return Ok(Err(FormatTimeZoneError::NameNotFound));
         };
 
@@ -323,19 +323,19 @@ impl FormatTimeZone for SpecificNonLocationFormat {
         let Some(time_zone_id) = input.time_zone_id else {
             return Ok(Err(FormatTimeZoneError::MissingInputField("time_zone_id")));
         };
-
         let Some(names) = (match self.0 {
             FieldLength::Wide => data_payloads.mz_specific_long.as_ref(),
             _ => data_payloads.mz_specific_short.as_ref(),
         }) else {
             return Ok(Err(FormatTimeZoneError::MissingZoneSymbols));
         };
 
-        let Some(name) = names
-            .overrides
-            .get_2d(&time_zone_id, &zone_variant)
-            .or_else(|| names.defaults.get_2d(&metazone_id, &zone_variant))
-        else {
+        let Some(name) = metazone_id.and_then(|mz| {
+            names
+                .overrides
+                .get_2d(&time_zone_id, &zone_variant)
+                .or_else(|| names.defaults.get_2d(&mz, &zone_variant))
+        }) else {
             return Ok(Err(FormatTimeZoneError::NameNotFound));
         };
 
@@ -454,9 +454,15 @@ impl FormatTimeZone for GenericLocationFormat {
             return Ok(Err(FormatTimeZoneError::MissingZoneSymbols));
         };
 
-        let Some(location) = locations.locations.get(&time_zone_id) else {
-            return Ok(Err(FormatTimeZoneError::NameNotFound));
-        };
+        let location = locations
+            .locations
+            .get(&time_zone_id)
+            .or_else(|| {
+                locations
+                    .locations
+                    .get(&TimeZoneBcp47Id(tinystr::tinystr!(8, "unk")))
+            })
+            .unwrap_or("Unknown");
 
         locations
             .pattern_generic
@@ -555,11 +561,12 @@ impl FormatTimeZone for GenericPartialLocationFormat {
         let Some(location) = locations.locations.get(&time_zone_id) else {
             return Ok(Err(FormatTimeZoneError::NameNotFound));
         };
-        let Some(non_location) = non_locations
-            .overrides
-            .get(&time_zone_id)
-            .or_else(|| non_locations.defaults.get(&metazone_id))
-        else {
+        let Some(non_location) = metazone_id.and_then(|mz| {
+            non_locations
+                .overrides
+                .get(&time_zone_id)
+                .or_else(|| non_locations.defaults.get(&mz))
+        }) else {
             return Ok(Err(FormatTimeZoneError::NameNotFound));
         };
 

diff --git a/components/datetime/tests/datetime.rs b/components/datetime/tests/datetime.rs
@@ -495,7 +495,7 @@ fn test_time_zone_format_offset_not_set_debug_assert_panic() {
         NeverCalendar,
     };
 
-    let time_zone = CustomTimeZone::try_from_str("America/Los_Angeles").unwrap();
+    let time_zone = CustomTimeZone::from_str("America/Los_Angeles");
     let tzf = TypedNeoFormatter::<NeverCalendar, _>::try_new(
         &locale!("en").into(),
         NeoTimeZoneOffsetMarker::with_length(NeoSkeletonLength::Medium),

diff --git a/components/datetime/tests/mock.rs b/components/datetime/tests/mock.rs
@@ -5,7 +5,7 @@
 //! Some useful parsing functions for tests.
 
 use icu_calendar::{DateTime, Gregorian};
-use icu_timezone::{CustomTimeZone, CustomZonedDateTime};
+use icu_timezone::CustomZonedDateTime;
 
 /// Temporary function for parsing a `DateTime<Gregorian>`
 ///
@@ -56,15 +56,7 @@ pub fn parse_gregorian_from_str(input: &str) -> DateTime<Gregorian> {
 ///         .expect("Failed to parse a zoned datetime.");
 /// ```
 pub fn parse_zoned_gregorian_from_str(input: &str) -> CustomZonedDateTime<Gregorian> {
-    let datetime_iso = CustomZonedDateTime::try_iso_from_str(input)
-        .or_else(|_| CustomZonedDateTime::try_iso_from_str(input.split('[').next().unwrap()))
-        .or_else(|_| {
-            DateTime::try_iso_from_str(input).map(|dt| CustomZonedDateTime {
-                date: dt.date,
-                time: dt.time,
-                zone: CustomTimeZone::new_empty(),
-            })
-        })
-        .unwrap();
-    datetime_iso.to_calendar(Gregorian)
+    CustomZonedDateTime::try_iso_from_str(input)
+        .unwrap()
+        .to_calendar(Gregorian)
 }
diff --git a/components/datetime/tests/patterns/tests/time_zones.json b/components/datetime/tests/patterns/tests/time_zones.json
@@ -1109,37 +1109,5 @@
         "expected": ["Unknown City Time"]
       }
     ]
-  },
-  {
-    "locale": "en",
-    "datetime": "2021-07-11T12:00:00.000",
-    "expectations": [
-      {
-        "patterns": [
-          "z",
-          "zz",
-          "zzz",
-          "zzzz",
-          "Z",
-          "ZZ",
-          "ZZZ",
-          "ZZZZ",
-          "ZZZZZ",
-          "O",
-          "OOOO"
-        ],
-        "configs": [],
-        "expected": ["{GMT+?}"]
-      },
-      {
-        "patterns": [
-          "v",
-          "vvvv",
-          "VVVV"
-        ],
-        "configs": [],
-        "expected": ["Unknown City Time"]
-      }
-    ]
   }
 ]
diff --git a/components/timezone/README.md b/components/timezone/README.md
diff --git a/components/timezone/src/error.rs b/components/timezone/src/error.rs
@@ -8,8 +8,3 @@ use displaydoc::Display;
 #[derive(Display, Debug, Copy, Clone, PartialEq)]
 #[allow(clippy::exhaustive_structs)]
 pub struct InvalidOffsetError;
-
-/// An error when the time zone was invalid or unknown.
-#[derive(Debug, Copy, Clone, PartialEq, Eq)]
-#[non_exhaustive]
-pub struct UnknownTimeZoneError;