Fixes for EpochNanoseconds and Offset parsing (#223)

This PR contains some fixes that was causing
`IsoDateTime::from_epoch_nanoseconds` to throw incorrectly along with
fixing the offset parsing that was being too forgiving and therefore not
throwing errors when it should.

Author: nekevss

src/builtins/core/datetime.rs

@@ -87,7 +87,7 @@ impl PlainDateTime {
         offset: i64,
         calendar: Calendar,
     ) -> TemporalResult<Self> {
-        let iso = IsoDateTime::from_epoch_nanos(&instant.as_i128(), offset)?;
+        let iso = IsoDateTime::from_epoch_nanos(instant.epoch_nanoseconds(), offset)?;
         Ok(Self { iso, calendar })
     }
 

src/builtins/core/instant.rs

@@ -52,7 +52,7 @@ impl Instant {
     /// Temporal-Proposal equivalent: `AddInstant`.
     pub(crate) fn add_to_instant(&self, duration: &TimeDuration) -> TemporalResult<Self> {
         let norm = NormalizedTimeDuration::from_time_duration(duration);
-        let result = self.epoch_nanoseconds() + norm.0;
+        let result = self.epoch_nanoseconds().0 + norm.0;
         Ok(Self::from(EpochNanoseconds::try_from(result)?))
     }
 
@@ -231,8 +231,8 @@ impl Instant {
 
     /// Returns the `epochNanoseconds` value for this `Instant`.
     #[must_use]
-    pub fn epoch_nanoseconds(&self) -> i128 {
-        self.as_i128()
+    pub fn epoch_nanoseconds(&self) -> &EpochNanoseconds {
+        &self.0
     }
 
     // TODO: May end up needing a provider API during impl
@@ -350,8 +350,8 @@ mod tests {
         let max_instant = Instant::try_new(max).unwrap();
         let min_instant = Instant::try_new(min).unwrap();
 
-        assert_eq!(max_instant.epoch_nanoseconds(), max);
-        assert_eq!(min_instant.epoch_nanoseconds(), min);
+        assert_eq!(max_instant.epoch_nanoseconds().0, max);
+        assert_eq!(min_instant.epoch_nanoseconds().0, min);
 
         let max_plus_one = NS_MAX_INSTANT + 1;
         let min_minus_one = NS_MIN_INSTANT - 1;

src/builtins/core/timezone.rs

@@ -141,7 +141,7 @@ impl TimeZone {
         provider: &impl TimeZoneProvider,
     ) -> TemporalResult<IsoDateTime> {
         let nanos = self.get_offset_nanos_for(instant.as_i128(), provider)?;
-        IsoDateTime::from_epoch_nanos(&instant.as_i128(), nanos.to_i64().unwrap_or(0))
+        IsoDateTime::from_epoch_nanos(instant.epoch_nanoseconds(), nanos.to_i64().unwrap_or(0))
     }
 
     /// Get the offset for this current `TimeZoneSlot`.

src/builtins/core/zoneddatetime.rs

@@ -192,7 +192,7 @@ impl ZonedDateTime {
             .get_iso_datetime_for(&self.instant, provider)?;
         // 5. Return ? RoundRelativeDuration(difference, ns2, dateTime, timeZone, calendar, largestUnit, roundingIncrement, smallestUnit, roundingMode).
         diff.round_relative_duration(
-            other.epoch_nanoseconds(),
+            other.epoch_nanoseconds().as_i128(),
             &PlainDateTime::new_unchecked(iso, self.calendar().clone()),
             Some((self.timezone(), provider)),
             resolved_options,
@@ -214,7 +214,7 @@ impl ZonedDateTime {
         // 3. Let endDateTime be GetISODateTimeFor(timeZone, ns2).
         let end = self.tz.get_iso_datetime_for(&other.instant, provider)?;
         // 4. If ns2 - ns1 < 0, let sign be -1; else let sign be 1.
-        let sign = if other.epoch_nanoseconds() - self.epoch_nanoseconds() < 0 {
+        let sign = if other.epoch_nanoseconds().as_i128() - self.epoch_nanoseconds().as_i128() < 0 {
             Sign::Negative
         } else {
             Sign::Positive
@@ -254,7 +254,7 @@ impl ZonedDateTime {
             )?;
             // d. Set timeDuration to TimeDurationFromEpochNanosecondsDifference(ns2, intermediateNs).
             time_duration = NormalizedTimeDuration::from_nanosecond_difference(
-                other.epoch_nanoseconds(),
+                other.epoch_nanoseconds().as_i128(),
                 intermediate_ns.0,
             )?;
             // e. Let timeSign be TimeDurationSign(timeDuration).
@@ -428,7 +428,7 @@ impl ZonedDateTime {
 
     /// Returns the `epochNanoseconds` value of this `ZonedDateTime`.
     #[must_use]
-    pub fn epoch_nanoseconds(&self) -> i128 {
+    pub fn epoch_nanoseconds(&self) -> &EpochNanoseconds {
         self.instant.epoch_nanoseconds()
     }
 
@@ -445,13 +445,21 @@ impl ZonedDateTime {
     /// Creates a new `ZonedDateTime` from the current `ZonedDateTime`
     /// combined with the provided `TimeZone`.
     pub fn with_timezone(&self, timezone: TimeZone) -> TemporalResult<Self> {
-        Self::try_new(self.epoch_nanoseconds(), self.calendar.clone(), timezone)
+        Self::try_new(
+            self.epoch_nanoseconds().as_i128(),
+            self.calendar.clone(),
+            timezone,
+        )
     }
 
     /// Creates a new `ZonedDateTime` from the current `ZonedDateTime`
     /// combined with the provided `Calendar`.
     pub fn with_calendar(&self, calendar: Calendar) -> TemporalResult<Self> {
-        Self::try_new(self.epoch_nanoseconds(), calendar, self.tz.clone())
+        Self::try_new(
+            self.epoch_nanoseconds().as_i128(),
+            calendar,
+            self.tz.clone(),
+        )
     }
 
     /// Compares one `ZonedDateTime` to another `ZonedDateTime` using their
@@ -487,8 +495,11 @@ impl ZonedDateTime {
         // 10. Else,
         // a. Assert: direction is previous.
         // b. Let transition be GetNamedTimeZonePreviousTransition(timeZone, zonedDateTime.[[EpochNanoseconds]]).
-        let transition =
-            provider.get_named_tz_transition(identifier, self.epoch_nanoseconds(), direction)?;
+        let transition = provider.get_named_tz_transition(
+            identifier,
+            self.epoch_nanoseconds().as_i128(),
+            direction,
+        )?;
 
         // 11. If transition is null, return null.
         // 12. Return ! CreateTemporalZonedDateTime(transition, timeZone, zonedDateTime.[[Calendar]]).
@@ -606,7 +617,7 @@ impl ZonedDateTime {
     pub fn offset_with_provider(&self, provider: &impl TimeZoneProvider) -> TemporalResult<String> {
         let offset = self
             .tz
-            .get_offset_nanos_for(self.epoch_nanoseconds(), provider)?;
+            .get_offset_nanos_for(self.epoch_nanoseconds().as_i128(), provider)?;
         Ok(nanoseconds_to_formattable_offset(offset).to_string())
     }
 
@@ -616,7 +627,7 @@ impl ZonedDateTime {
     ) -> TemporalResult<i64> {
         let offset = self
             .tz
-            .get_offset_nanos_for(self.epoch_nanoseconds(), provider)?;
+            .get_offset_nanos_for(self.epoch_nanoseconds().as_i128(), provider)?;
         Ok(offset as i64)
     }
 }
@@ -1139,6 +1150,7 @@ mod tests {
         options::{DifferenceSettings, Disambiguation, OffsetDisambiguation, TemporalUnit},
         partial::{PartialDate, PartialTime, PartialZonedDateTime},
         primitive::FiniteF64,
+        time::EpochNanoseconds,
         tzdb::FsTzdbProvider,
         Calendar, MonthCode, TimeZone,
     };
@@ -1245,10 +1257,13 @@ mod tests {
         )
         .unwrap();
 
-        assert_eq!(start_of_day.epoch_nanoseconds(), -1601753400000000000);
+        assert_eq!(
+            start_of_day.epoch_nanoseconds(),
+            &EpochNanoseconds(-1601753400000000000)
+        );
         assert_eq!(
             midnight_disambiguated.epoch_nanoseconds(),
-            -1601751600000000000
+            &EpochNanoseconds(-1601751600000000000)
         );
         let diff = start_of_day
             .instant

src/epoch_nanoseconds.rs

@@ -38,6 +38,13 @@ impl TryFrom<f64> for EpochNanoseconds {
     }
 }
 
+// Potential TODO: Build out primitive arthmetic methods if needed.
+impl EpochNanoseconds {
+    pub fn as_i128(&self) -> i128 {
+        self.0
+    }
+}
+
 /// Utility for determining if the nanos are within a valid range.
 #[inline]
 #[must_use]

src/iso.rs

@@ -44,7 +44,7 @@ use crate::{
     utils, TemporalResult, TemporalUnwrap, NS_PER_DAY,
 };
 use icu_calendar::{Date as IcuDate, Iso};
-use num_traits::{cast::FromPrimitive, AsPrimitive, Euclid, ToPrimitive};
+use num_traits::{cast::FromPrimitive, AsPrimitive, Euclid};
 
 /// `IsoDateTime` is the record of the `IsoDate` and `IsoTime` internal slots.
 #[non_exhaustive]
@@ -77,19 +77,19 @@ impl IsoDateTime {
     // TODO: Move away from offset use of f64
     /// Creates an `IsoDateTime` from a `BigInt` of epochNanoseconds.
     #[allow(clippy::neg_cmp_op_on_partial_ord)]
-    pub(crate) fn from_epoch_nanos(nanos: &i128, offset: i64) -> TemporalResult<Self> {
+    pub(crate) fn from_epoch_nanos(
+        epoch_nanoseconds: &EpochNanoseconds,
+        offset: i64,
+    ) -> TemporalResult<Self> {
         // Skip the assert as nanos should be validated by Instant.
         // TODO: Determine whether value needs to be validated as integral.
         // Get the component ISO parts
-        let mathematical_nanos = nanos.to_i64().ok_or_else(|| {
-            TemporalError::range().with_message("nanos was not within a valid range.")
-        })?;
 
         // 2. Let remainderNs be epochNanoseconds modulo 10^6.
-        let remainder_nanos = mathematical_nanos.rem_euclid(1_000_000);
+        let remainder_nanos = epoch_nanoseconds.0.rem_euclid(1_000_000);
 
         // 3. Let epochMilliseconds be 𝔽((epochNanoseconds - remainderNs) / 10^6).
-        let epoch_millis = (mathematical_nanos - remainder_nanos) / 1_000_000;
+        let epoch_millis = (epoch_nanoseconds.0 - remainder_nanos).div_euclid(1_000_000) as i64;
 
         let (year, month, day) = utils::ymd_from_epoch_milliseconds(epoch_millis);
 
@@ -103,11 +103,11 @@ impl IsoDateTime {
         let millis = epoch_millis.rem_euclid(1000);
 
         // 11. Let microsecond be floor(remainderNs / 1000).
-        let micros = remainder_nanos.div_euclid(1000);
+        let micros = remainder_nanos.div_euclid(1_000) as i64;
         // 12. Assert: microsecond < 1000.
         temporal_assert!(micros < 1000);
         // 13. Let nanosecond be remainderNs modulo 1000.
-        let nanos = remainder_nanos.rem_euclid(1000);
+        let nanos = remainder_nanos.rem_euclid(1000) as i64;
 
         Ok(Self::balance(
             year,

src/parsers/timezone.rs

@@ -66,6 +66,10 @@ pub(crate) fn parse_offset(chars: &mut Peekable<Chars<'_>>) -> TemporalResult<Op
     // First offset portion
     let hours = parse_digit_pair(chars)?;
 
+    if !(0..24).contains(&hours) {
+        return Err(TemporalError::range().with_message("Invalid offset hour value."));
+    }
+
     let sep = chars.peek().is_some_and(|ch| *ch == ':');
     if sep {
         let _ = chars.next();
@@ -79,7 +83,54 @@ pub(crate) fn parse_offset(chars: &mut Peekable<Chars<'_>>) -> TemporalResult<Op
         None => 0,
     };
 
-    Ok(Some((hours * 60 + minutes) * sign))
+    if !(0..60).contains(&minutes) {
+        return Err(TemporalError::range().with_message("Invalid offset hour value."));
+    }
+
+    let result = Some((hours * 60 + minutes) * sign);
+
+    // We continue parsing for correctness, but we only care about
+    // minute precision
+
+    let next_peek = chars.peek();
+    match next_peek {
+        Some(&':') if sep => _ = chars.next(),
+        Some(&':') => {
+            return Err(TemporalError::range().with_message("offset separators do not align."))
+        }
+        Some(_) => _ = parse_digit_pair(chars),
+        None => return Ok(result),
+    }
+
+    let potential_fraction = chars.next();
+    match potential_fraction {
+        Some(ch) if ch == '.' || ch == ',' => {
+            if !chars.peek().is_some_and(|ch| ch.is_ascii_digit()) {
+                return Err(
+                    TemporalError::range().with_message("fraction separator must have digit after")
+                );
+            }
+        }
+        Some(_) => return Err(TemporalError::range().with_message("Invalid offset")),
+        None => return Ok(result),
+    }
+
+    for _ in 0..9 {
+        let digit_or_end = chars.next().map(|ch| ch.is_ascii_digit());
+        match digit_or_end {
+            Some(true) => {}
+            Some(false) => {
+                return Err(TemporalError::range().with_message("Not a valid fractional second"))
+            }
+            None => break,
+        }
+    }
+
+    if chars.peek().is_some() {
+        return Err(TemporalError::range().with_message("Invalid offset"));
+    }
+
+    Ok(result)
 }
 
 fn parse_digit_pair(chars: &mut Peekable<Chars<'_>>) -> TemporalResult<i16> {
@@ -103,7 +154,15 @@ fn parse_digit_pair(chars: &mut Peekable<Chars<'_>>) -> TemporalResult<i16> {
     let tens = (first.to_digit(10).expect("validated") * 10) as i16;
     let ones = second.to_digit(10).expect("validated") as i16;
 
-    Ok(tens + ones)
+    let result = tens + ones;
+
+    if !(0..=59).contains(&result) {
+        return Err(
+            TemporalError::range().with_message("digit pair not in a valid range of [0..59]")
+        );
+    }
+
+    Ok(result)
 }
 
 fn parse_iana_component(chars: &mut Peekable<Chars<'_>>) -> bool {

temporal_capi/src/instant.rs

@@ -102,7 +102,7 @@ pub mod ffi {
         }
 
         pub fn epoch_nanoseconds(&self) -> I128Nanoseconds {
-            let ns = self.0.epoch_nanoseconds();
+            let ns = self.0.epoch_nanoseconds().as_i128();
             let is_neg = ns < 0;
             let ns = ns.unsigned_abs();