Skip to content

Commit

Permalink
Fix bug introduced by EpochNanoseconds + adjust tests to catch bett…
Browse files Browse the repository at this point in the history
…er (#128)

This fixes a bug that was introduced by #116.

General gist is that the in the proposal [`5.5.4 ISODateTimeWithinLimits
( isoDateTime
)`](https://tc39.es/proposal-temporal/#sec-temporal-isodatetimewithinlimits)
there is a call to `GetUTCEpochNanoseconds`. However, this value is not
checked to be valid, and instead of constraining to the
NS_MAX_INSTANT/NS_MIN_INSTANT +/- NS_PER_DAY, respectively, we were
constraining to NS_MAX_INSTANT/NS_MIN_INSTANT.

There was a limit test on datetime, but we were only asserting the
negative case, not the positive case, so the regression was not caught.

So adjusted to handle the potentially invalid value, adjusted the
preexisting datetime test, and added a date limit test based off the
test262 limit test.

EDIT: Potentially relevant context to #119
  • Loading branch information
nekevss authored Dec 8, 2024
1 parent f7fa900 commit 016bc31
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 24 deletions.
33 changes: 33 additions & 0 deletions src/components/date.rs
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,39 @@ mod tests {

use super::*;

#[test]
fn new_date_limits() {
let err = PlainDate::try_new(-271_821, 4, 18, Calendar::default());
assert!(err.is_err());
let err = PlainDate::try_new(275_760, 9, 14, Calendar::default());
assert!(err.is_err());
let ok = PlainDate::try_new(-271_821, 4, 19, Calendar::default()).unwrap();
assert_eq!(
ok,
PlainDate {
iso: IsoDate {
year: -271_821,
month: 4,
day: 19,
},
calendar: Calendar::default(),
}
);

let ok = PlainDate::try_new(275_760, 9, 13, Calendar::default()).unwrap();
assert_eq!(
ok,
PlainDate {
iso: IsoDate {
year: 275760,
month: 9,
day: 13,
},
calendar: Calendar::default(),
}
);
}

#[test]
fn simple_date_add() {
let base = PlainDate::from_str("1976-11-18").unwrap();
Expand Down
56 changes: 39 additions & 17 deletions src/components/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -724,20 +724,20 @@ impl FromStr for PlainDateTime {

#[cfg(test)]
mod tests {
use core::str::FromStr;

use tinystr::{tinystr, TinyAsciiStr};

use crate::{
components::{
calendar::Calendar, duration::DateDuration, Duration, PartialDate, PartialDateTime,
PartialTime, PlainDateTime,
},
iso::{IsoDate, IsoDateTime, IsoTime},
options::{
DifferenceSettings, RoundingIncrement, RoundingOptions, TemporalRoundingMode,
TemporalUnit,
},
primitive::FiniteF64,
TemporalResult,
};

fn assert_datetime(
Expand All @@ -756,28 +756,50 @@ mod tests {
assert_eq!(dt.nanosecond(), fields.9);
}

fn pdt_from_date(year: i32, month: i32, day: i32) -> TemporalResult<PlainDateTime> {
PlainDateTime::try_new(year, month, day, 0, 0, 0, 0, 0, 0, Calendar::default())
}

#[test]
#[allow(clippy::float_cmp)]
fn plain_date_time_limits() {
// This test is primarily to assert that the `expect` in the epoch methods is
// valid, i.e., a valid instant is within the range of an f64.
let negative_limit = PlainDateTime::try_new(
-271_821,
4,
19,
0,
0,
0,
0,
0,
0,
Calendar::from_str("iso8601").unwrap(),
);
let positive_limit =
PlainDateTime::try_new(275_760, 9, 14, 0, 0, 0, 0, 0, 0, Calendar::default());

let negative_limit = pdt_from_date(-271_821, 4, 19);
assert!(negative_limit.is_err());
let positive_limit = pdt_from_date(275_760, 9, 14);
assert!(positive_limit.is_err());
let within_negative_limit = pdt_from_date(-271_821, 4, 20);
assert_eq!(
within_negative_limit,
Ok(PlainDateTime {
iso: IsoDateTime {
date: IsoDate {
year: -271_821,
month: 4,
day: 20,
},
time: IsoTime::default(),
},
calendar: Calendar::default(),
})
);

let within_positive_limit = pdt_from_date(275_760, 9, 13);
assert_eq!(
within_positive_limit,
Ok(PlainDateTime {
iso: IsoDateTime {
date: IsoDate {
year: 275_760,
month: 9,
day: 13,
},
time: IsoTime::default(),
},
calendar: Calendar::default(),
})
);
}

#[test]
Expand Down
19 changes: 12 additions & 7 deletions src/iso.rs
Original file line number Diff line number Diff line change
Expand Up @@ -885,36 +885,41 @@ impl IsoTime {

// ==== `IsoDateTime` specific utility functions ====

const MAX_EPOCH_DAYS: i32 = 10i32.pow(8) + 1;

#[inline]
/// Utility function to determine if a `DateTime`'s components create a `DateTime` within valid limits
fn iso_dt_within_valid_limits(date: IsoDate, time: &IsoTime) -> bool {
if iso_date_to_epoch_days(date.year, (date.month - 1).into(), date.day.into()).abs()
> 100_000_001
> MAX_EPOCH_DAYS
{
return false;
}
let Ok(ns) = utc_epoch_nanos(date, time) else {
return false;
};

let ns = to_unchecked_epoch_nanoseconds(date, time);
let max = crate::NS_MAX_INSTANT + i128::from(NS_PER_DAY);
let min = crate::NS_MIN_INSTANT - i128::from(NS_PER_DAY);

min <= ns.0 && max >= ns.0
min <= ns && max >= ns
}

#[inline]
/// Utility function to convert a `IsoDate` and `IsoTime` values into epoch nanoseconds
fn utc_epoch_nanos(date: IsoDate, time: &IsoTime) -> TemporalResult<EpochNanoseconds> {
let epoch_nanos = to_unchecked_epoch_nanoseconds(date, time);
EpochNanoseconds::try_from(epoch_nanos)
}

#[inline]
fn to_unchecked_epoch_nanoseconds(date: IsoDate, time: &IsoTime) -> i128 {
let ms = time.to_epoch_ms();
let epoch_ms = utils::epoch_days_to_epoch_ms(date.to_epoch_days(), ms);

let epoch_nanos = epoch_ms.mul_add(
1_000_000f64,
f64::from(time.microsecond).mul_add(1_000f64, f64::from(time.nanosecond)),
);

EpochNanoseconds::try_from(epoch_nanos)
epoch_nanos as i128
}

// ==== `IsoDate` specific utiltiy functions ====
Expand Down

0 comments on commit 016bc31

Please sign in to comment.