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.

Sign up for GitHub

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

Update ixdtf to handle unbound fraction length #6036

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Update ixdtf to handle unbound fraction length #6036

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9477c8e
Update fraction to handle unbounded fraction length
nekevss Jan 25, 2025
f86c37c
Add support for Fraction to icu_timezone
nekevss Jan 25, 2025
6192c3e
Update fraction according to feedback
nekevss Jan 27, 2025
73ea171
Change error to be more descriptive
nekevss Jan 27, 2025
f3bfb57
Update error to ExcessivePrecision and add Time::try_from_time_record
nekevss Jan 27, 2025
bf313b0
Merge branch 'main' into update-fraction-handling
nekevss Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions components/timezone/src/ixdtf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ pub enum ParseError {
InconsistentTimeZoneOffsets,
/// There was an invalid Offset.
InvalidOffsetError,
/// There was an invalid subsecond fraction.
InvalidFraction,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is overkill. I'd rather document that sub-nanoseconds are truncated. The formatting code only supports nanosecond precision anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, I'd prefer naming this

/// The string contains precision that is not representable by the output type.
ExcessivePrecision

InvalidFraction is what I'd expect for a string like "12:48:16.a0", which already produces Syntax.

/// The set of time zone fields was not expected for the given type.
/// For example, if a named time zone was present with offset-only parsing,
/// or an offset was present with named-time-zone-only parsing.
Expand Down Expand Up @@ -326,11 +328,14 @@ impl<'a> Intermediate<'a> {
};
let time_zone_id = mapper.iana_bytes_to_bcp47(iana_identifier);
let date = Date::<Iso>::try_new_iso(self.date.year, self.date.month, self.date.day)?;
let Some(nanosecond) = self.time.fraction.to_nanoseconds() else {
return Err(ParseError::InvalidFraction);
};
let time = Time::try_new(
self.time.hour,
self.time.minute,
self.time.second,
self.time.nanosecond,
nanosecond,
)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there'd only be one code path that parses the IXDTF struct into Time, and it should be Time::try_from_ixdtf_record. Please try to use that (also for Date ideally).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated Time with a try_from_time_record method to address this.

I'm not entirely clear what you had meant by also "for Date ideally." Unless the idea was to expose two methods: Time::try_from_time_record and Date::try_from_date_record on the referenced types. So if you were thinking of that or a bit of a different approach, let me know.

let offset = match time_zone_id.as_str() {
"utc" | "gmt" => Some(UtcOffset::zero()),
Expand Down Expand Up @@ -367,11 +372,14 @@ impl<'a> Intermediate<'a> {
},
};
let date = Date::<Iso>::try_new_iso(self.date.year, self.date.month, self.date.day)?;
let Some(nanosecond) = self.time.fraction.to_nanoseconds() else {
return Err(ParseError::InvalidFraction);
};
let time = Time::try_new(
self.time.hour,
self.time.minute,
self.time.second,
self.time.nanosecond,
nanosecond,
)?;
Ok(time_zone_id.with_offset(offset).at_time((date, time)))
}
Expand All @@ -389,11 +397,14 @@ impl<'a> Intermediate<'a> {
};
let time_zone_id = mapper.iana_bytes_to_bcp47(iana_identifier);
let date = Date::try_new_iso(self.date.year, self.date.month, self.date.day)?;
let Some(nanosecond) = self.time.fraction.to_nanoseconds() else {
return Err(ParseError::InvalidFraction);
};
let time = Time::try_new(
self.time.hour,
self.time.minute,
self.time.second,
self.time.nanosecond,
nanosecond,
)?;
let offset = UtcOffset::try_from_utc_offset_record(offset)?;
let zone_variant = match zone_offset_calculator
Expand Down Expand Up @@ -792,11 +803,14 @@ impl Time {

fn try_from_ixdtf_record(ixdtf_record: &IxdtfParseRecord) -> Result<Self, ParseError> {
let time_record = ixdtf_record.time.ok_or(ParseError::MissingFields)?;
let Some(nanosecond) = time_record.fraction.to_nanoseconds() else {
return Err(ParseError::InvalidFraction);
};
let time = Self::try_new(
time_record.hour,
time_record.minute,
time_record.second,
time_record.nanosecond,
nanosecond,
)?;
Ok(time)
}
Expand Down
12 changes: 6 additions & 6 deletions utils/ixdtf/README.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions utils/ixdtf/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ pub enum ParseError {
TimeSecond,
#[displaydoc("Invalid character while parsing fraction part value.")]
FractionPart,
#[displaydoc("Fraction part value exceeds a representable range.")]
InvalidFractionRange,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Invalid" is not very descriptive. This is an error, that already tells me that something is not valid.

Suggested change
InvalidFractionRange,
ExcessiveSecondPrecision,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was double checking this and the error is specific to a Duration hour fraction and minute fraction potentially exceeding the range of u64::MAX when calculated. Maybe DurationFractionExceededRange (this would then be moved down into the Duration errors).

#[displaydoc("Invalid character while parsing date separator.")]
DateSeparator,
#[displaydoc("Invalid character while parsing time separator.")]
Expand Down
12 changes: 6 additions & 6 deletions utils/ixdtf/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
//!
//! ```
//! use ixdtf::parsers::{
//! records::{Sign, TimeZoneRecord},
//! records::{Sign, TimeZoneRecord, Fraction},
//! IxdtfParser,
//! };
//!
Expand All @@ -42,7 +42,7 @@
//! assert_eq!(offset.hour, 5);
//! assert_eq!(offset.minute, 0);
//! assert_eq!(offset.second, 0);
//! assert_eq!(offset.nanosecond, 0);
//! assert_eq!(offset.fraction, Fraction { digits: 0, value: 0});
//! assert!(!tz_annotation.critical);
//! assert_eq!(
//! tz_annotation.tz,
Expand Down Expand Up @@ -74,7 +74,7 @@
//!
//! ```rust
//! use ixdtf::parsers::{
//! records::{Sign, TimeZoneRecord},
//! records::{Sign, TimeZoneRecord, Fraction},
//! IxdtfParser,
//! };
//!
Expand All @@ -96,7 +96,7 @@
//! assert_eq!(offset.hour, 0);
//! assert_eq!(offset.minute, 0);
//! assert_eq!(offset.second, 0);
//! assert_eq!(offset.nanosecond, 0);
//! assert_eq!(offset.fraction, Fraction { digits: 0, value: 0});
//! assert!(!tz_annotation.critical);
//! assert_eq!(
//! tz_annotation.tz,
Expand Down Expand Up @@ -135,7 +135,7 @@
//!
//! ```rust
//! use ixdtf::parsers::{
//! records::{Sign, TimeZoneRecord},
//! records::{Sign, TimeZoneRecord, Fraction},
//! IxdtfParser,
//! };
//!
Expand All @@ -152,7 +152,7 @@
//! assert_eq!(offset.hour, 0);
//! assert_eq!(offset.minute, 0);
//! assert_eq!(offset.second, 0);
//! assert_eq!(offset.nanosecond, 0);
//! assert_eq!(offset.fraction, Fraction { digits: 0, value: 0});
//! assert!(tz_annotation.critical);
//! assert_eq!(
//! tz_annotation.tz,
Expand Down
29 changes: 24 additions & 5 deletions utils/ixdtf/src/parsers/duration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{
is_month_designator, is_second_designator, is_sign, is_time_designator,
is_week_designator, is_year_designator,
},
records::{DateDurationRecord, DurationParseRecord, TimeDurationRecord},
records::{DateDurationRecord, DurationParseRecord, Fraction, TimeDurationRecord},
time::parse_fraction,
Cursor,
},
Expand Down Expand Up @@ -134,7 +134,7 @@ pub(crate) fn parse_time_duration(cursor: &mut Cursor) -> ParserResult<Option<Ti
TimeDurationDesignator,
);

let mut time: (u64, u64, u64, Option<u32>) = (0, 0, 0, None);
let mut time: (u64, u64, u64, Option<Fraction>) = (0, 0, 0, None);
let mut previous_unit = TimeUnit::None;
while cursor.check_or(false, |c| c.is_ascii_digit()) {
let mut value: u64 = 0;
Expand Down Expand Up @@ -194,20 +194,39 @@ pub(crate) fn parse_time_duration(cursor: &mut Cursor) -> ParserResult<Option<Ti
// Safety: Max fraction * 3600 is within u64 -> see test maximum_duration_fraction
TimeUnit::Hour => Ok(Some(TimeDurationRecord::Hours {
hours: time.0,
fraction: time.3.map(|f| 3600 * u64::from(f)).unwrap_or(0),
fraction: time
.3
.map(|f| adjust_fraction_for_unit(f, 3600))
.transpose()?
.unwrap_or_default(),
})),
// Safety: Max fraction * 60 is within u64 -> see test maximum_duration_fraction
TimeUnit::Minute => Ok(Some(TimeDurationRecord::Minutes {
hours: time.0,
minutes: time.1,
fraction: time.3.map(|f| 60 * u64::from(f)).unwrap_or(0),
fraction: time
.3
.map(|f| adjust_fraction_for_unit(f, 60))
.transpose()?
.unwrap_or_default(),
})),
TimeUnit::Second => Ok(Some(TimeDurationRecord::Seconds {
hours: time.0,
minutes: time.1,
seconds: time.2,
fraction: time.3.unwrap_or(0),
fraction: time.3.unwrap_or_default(),
})),
TimeUnit::None => Err(ParseError::abrupt_end("TimeDurationDesignator")),
}
}

fn adjust_fraction_for_unit(fraction: Fraction, unit: u64) -> ParserResult<Fraction> {
let value = fraction
.value
.checked_mul(unit)
.ok_or(ParseError::InvalidFractionRange)?;
Ok(Fraction {
digits: fraction.digits,
value,
})
}
22 changes: 11 additions & 11 deletions utils/ixdtf/src/parsers/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ impl<'a> IxdtfParser<'a> {
/// # Example
///
/// ```rust
/// use ixdtf::parsers::{IsoDurationParser, records::{Sign, DurationParseRecord, TimeDurationRecord}};
/// use ixdtf::parsers::{IsoDurationParser, records::{Sign, DurationParseRecord, Fraction, TimeDurationRecord}};
///
/// let duration_str = "P1Y2M1DT2H10M30S";
///
Expand All @@ -245,13 +245,13 @@ impl<'a> IxdtfParser<'a> {
/// let date_duration = result.date.unwrap();
///
/// let (hours, minutes, seconds, fraction) = match result.time {
/// // Hours variant is defined as { hours: u32, fraction: u64 }
/// // Hours variant is defined as { hours: u32, fraction: Fraction }
/// Some(TimeDurationRecord::Hours{ hours, fraction }) => (hours, 0, 0, fraction),
/// // Minutes variant is defined as { hours: u32, minutes: u32, fraction: u64 }
/// // Minutes variant is defined as { hours: u32, minutes: u32, fraction: Fraction }
/// Some(TimeDurationRecord::Minutes{ hours, minutes, fraction }) => (hours, minutes, 0, fraction),
/// // Seconds variant is defined as { hours: u32, minutes: u32, seconds: u32, fraction: u32 }
/// Some(TimeDurationRecord::Seconds{ hours, minutes, seconds, fraction }) => (hours, minutes, seconds, fraction as u64),
/// None => (0,0,0,0),
/// // Seconds variant is defined as { hours: u32, minutes: u32, seconds: u32, fraction: Fraction }
/// Some(TimeDurationRecord::Seconds{ hours, minutes, seconds, fraction }) => (hours, minutes, seconds, fraction),
/// None => (0,0,0, Fraction::default()),
/// };
///
/// assert_eq!(result.sign, Sign::Positive);
Expand All @@ -262,7 +262,7 @@ impl<'a> IxdtfParser<'a> {
/// assert_eq!(hours, 2);
/// assert_eq!(minutes, 10);
/// assert_eq!(seconds, 30);
/// assert_eq!(fraction, 0);
/// assert_eq!(fraction, Fraction { digits: 0, value: 0 });
/// ```
#[cfg(feature = "duration")]
#[derive(Debug)]
Expand Down Expand Up @@ -313,7 +313,7 @@ impl<'a> IsoDurationParser<'a> {
/// ## Parsing a time duration
///
/// ```rust
/// # use ixdtf::parsers::{IsoDurationParser, records::{DurationParseRecord, TimeDurationRecord }};
/// # use ixdtf::parsers::{IsoDurationParser, records::{DurationParseRecord, Fraction, TimeDurationRecord }};
/// let time_duration = "PT2H10M30S";
///
/// let result = IsoDurationParser::from_str(time_duration).parse().unwrap();
Expand All @@ -324,14 +324,14 @@ impl<'a> IsoDurationParser<'a> {
/// // Minutes variant is defined as { hours: u32, minutes: u32, fraction: u64 }
/// Some(TimeDurationRecord::Minutes{ hours, minutes, fraction }) => (hours, minutes, 0, fraction),
/// // Seconds variant is defined as { hours: u32, minutes: u32, seconds: u32, fraction: u32 }
/// Some(TimeDurationRecord::Seconds{ hours, minutes, seconds, fraction }) => (hours, minutes, seconds, fraction as u64),
/// None => (0,0,0,0),
/// Some(TimeDurationRecord::Seconds{ hours, minutes, seconds, fraction }) => (hours, minutes, seconds, fraction),
/// None => (0,0,0,Fraction::default()),
/// };
/// assert!(result.date.is_none());
/// assert_eq!(hours, 2);
/// assert_eq!(minutes, 10);
/// assert_eq!(seconds, 30);
/// assert_eq!(fraction, 0);
/// assert_eq!(fraction, Fraction { digits: 0, value: 0 });
/// ```
pub fn parse(&mut self) -> ParserResult<DurationParseRecord> {
duration::parse_duration(&mut self.cursor)
Expand Down
Loading
Loading