Skip to content

Commit 58dacbe

Browse files
authored
Fix TimeZone::get_possible_epoch_ns at date-time limits (#580)
The `is_valid_day_range()` check was based on an erroneous line in the Temporal spec, see tc39/proposal-temporal#3151. This PR removes that check, and adds a `get_possible_epoch_ns` unit test for the affected cases. For the unit test it was convenient to have CandidateEpochNanoseconds and EpochNanosecondsAndOffset implement PartialEq. Please let me know if that's not desired and I'll do it another way. The second commit is a cleanup, not necessary for the bug fix. Feel free to drop it if you prefer not to have it. While I was fixing the above, I noticed that in most places the `to_epoch_days` value is cast to i32 and then immediately back to i64, so I changed the return type to i64 and instead moved the cast to the place where i32 is actually needed.
1 parent f80164a commit 58dacbe

File tree

4 files changed

+72
-14
lines changed

4 files changed

+72
-14
lines changed

provider/src/provider.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,8 @@ pub struct IsoDateTime {
3030
}
3131

3232
impl IsoDateTime {
33-
fn to_epoch_days(self) -> i32 {
34-
// NOTE: cast to i32 is safe as IsoDate is in a valid range.
35-
utils::epoch_days_from_gregorian_date(self.year, self.month, self.day) as i32
33+
fn to_epoch_days(self) -> i64 {
34+
utils::epoch_days_from_gregorian_date(self.year, self.month, self.day)
3635
}
3736
/// `IsoTimeToEpochMs`
3837
///
@@ -49,7 +48,7 @@ impl IsoDateTime {
4948
/// Convert this datetime to nanoseconds since the Unix epoch
5049
pub fn as_nanoseconds(&self) -> EpochNanoseconds {
5150
let time_ms = self.time_to_epoch_ms();
52-
let epoch_ms = utils::epoch_days_to_epoch_ms(self.to_epoch_days() as i64, time_ms);
51+
let epoch_ms = utils::epoch_days_to_epoch_ms(self.to_epoch_days(), time_ms);
5352
EpochNanoseconds(
5453
epoch_ms as i128 * 1_000_000
5554
+ self.microsecond as i128 * 1_000
@@ -78,7 +77,7 @@ impl From<LocalTimeTypeRecord> for UtcOffsetSeconds {
7877
}
7978

8079
/// An EpochNanoseconds and a UTC offset
81-
#[derive(Copy, Clone, Debug)]
80+
#[derive(Copy, Clone, Debug, PartialEq)]
8281
pub struct EpochNanosecondsAndOffset {
8382
/// The resolved nanoseconds value
8483
pub ns: EpochNanoseconds,
@@ -135,7 +134,7 @@ pub struct GapEntryOffsets {
135134
}
136135

137136
/// The potential candidates for a given local datetime
138-
#[derive(Copy, Clone, Debug)]
137+
#[derive(Copy, Clone, Debug, PartialEq)]
139138
pub enum CandidateEpochNanoseconds {
140139
Zero(GapEntryOffsets),
141140
One(EpochNanosecondsAndOffset),

src/builtins/core/plain_date.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,10 @@ impl PlainDate {
308308
#[inline]
309309
#[must_use]
310310
fn days_until(&self, other: &Self) -> i32 {
311-
other.iso.to_epoch_days() - self.iso.to_epoch_days()
311+
// NOTE: cast to i32 is safe as both IsoDates must be in valid range.
312+
debug_assert!(self.iso.check_within_limits().is_ok());
313+
debug_assert!(other.iso.check_within_limits().is_ok());
314+
(other.iso.to_epoch_days() - self.iso.to_epoch_days()) as i32
312315
}
313316

314317
/// Returns this `PlainDate`'s ISO year value.

src/builtins/core/time_zone.rs

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -416,9 +416,7 @@ impl TimeZone {
416416
}
417417
// 3. Else,
418418
Self::IanaIdentifier(identifier) => {
419-
// a. Perform ? CheckISODaysRange(isoDateTime.[[ISODate]]).
420-
local_iso.date.is_valid_day_range()?;
421-
// b. Let possibleEpochNanoseconds be
419+
// a. Let possibleEpochNanoseconds be
422420
// GetNamedTimeZoneEpochNanoseconds(parseResult.[[Name]],
423421
// isoDateTime).
424422
provider.candidate_nanoseconds_for_local_epoch_nanoseconds(
@@ -625,4 +623,63 @@ mod tests {
625623
.time_zone_equals_with_provider(&kolkata, &*crate::builtins::TZ_PROVIDER)
626624
.unwrap());
627625
}
626+
627+
#[cfg(feature = "compiled_data")]
628+
fn test_possible_epoch_ns_at_limits_helper(
629+
time_zone_identifier: &str,
630+
offset_sec_at_min: i64,
631+
offset_sec_at_max: i64,
632+
) {
633+
use crate::iso::IsoDateTime;
634+
use crate::provider::{
635+
CandidateEpochNanoseconds, EpochNanosecondsAndOffset, UtcOffsetSeconds,
636+
};
637+
use crate::unix_time::EpochNanoseconds;
638+
use crate::{NS_MAX_INSTANT, NS_MIN_INSTANT};
639+
640+
let provider = &*crate::builtins::TZ_PROVIDER;
641+
642+
let time_zone = TimeZone::try_from_identifier_str(time_zone_identifier).unwrap();
643+
644+
let min = IsoDateTime::balance(-271821, 4, 20, 0, 0, offset_sec_at_min, 0, 0, 0);
645+
let min_result = time_zone.get_possible_epoch_ns_for(min, provider).unwrap();
646+
assert_eq!(
647+
min_result,
648+
CandidateEpochNanoseconds::One(EpochNanosecondsAndOffset {
649+
ns: EpochNanoseconds(NS_MIN_INSTANT),
650+
offset: UtcOffsetSeconds(offset_sec_at_min)
651+
})
652+
);
653+
654+
let max = IsoDateTime::balance(275760, 9, 13, 0, 0, offset_sec_at_max, 0, 0, 0);
655+
let max_result = time_zone.get_possible_epoch_ns_for(max, provider).unwrap();
656+
assert_eq!(
657+
max_result,
658+
CandidateEpochNanoseconds::One(EpochNanosecondsAndOffset {
659+
ns: EpochNanoseconds(NS_MAX_INSTANT),
660+
offset: UtcOffsetSeconds(offset_sec_at_max)
661+
})
662+
);
663+
664+
let too_early = IsoDateTime::balance(-271821, 4, 20, 0, 0, offset_sec_at_min, 0, 0, -1);
665+
assert!(time_zone
666+
.get_possible_epoch_ns_for(too_early, provider)
667+
.is_err());
668+
669+
let too_late = IsoDateTime::balance(275760, 9, 13, 0, 0, offset_sec_at_max, 0, 0, 1);
670+
assert!(time_zone
671+
.get_possible_epoch_ns_for(too_late, provider)
672+
.is_err());
673+
}
674+
675+
#[test]
676+
#[cfg(feature = "compiled_data")]
677+
fn test_possible_epoch_ns_at_limits() {
678+
test_possible_epoch_ns_at_limits_helper("UTC", 0, 0);
679+
test_possible_epoch_ns_at_limits_helper("+02:00", 7200, 7200);
680+
test_possible_epoch_ns_at_limits_helper("-07:00", -25200, -25200);
681+
test_possible_epoch_ns_at_limits_helper("Europe/Amsterdam", 1050, 7200);
682+
test_possible_epoch_ns_at_limits_helper("America/Vancouver", -29548, -25200);
683+
test_possible_epoch_ns_at_limits_helper("Australia/Sydney", 36292, 36000);
684+
}
628685
}

src/iso.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -362,9 +362,8 @@ impl IsoDate {
362362
///
363363
/// Equivalent to `IsoDateToEpochDays`
364364
#[inline]
365-
pub(crate) fn to_epoch_days(self) -> i32 {
366-
// NOTE: cast to i32 is safe as IsoDate is in a valid range.
367-
utils::epoch_days_from_gregorian_date(self.year, self.month, self.day) as i32
365+
pub(crate) fn to_epoch_days(self) -> i64 {
366+
utils::epoch_days_from_gregorian_date(self.year, self.month, self.day)
368367
}
369368

370369
/// Returns if the current `IsoDate` is valid.
@@ -903,7 +902,7 @@ fn utc_epoch_nanos(date: IsoDate, time: &IsoTime) -> EpochNanoseconds {
903902
#[inline]
904903
fn to_unchecked_epoch_nanoseconds(date: IsoDate, time: &IsoTime) -> i128 {
905904
let ms = time.to_epoch_ms();
906-
let epoch_ms = utils::epoch_days_to_epoch_ms(date.to_epoch_days() as i64, ms);
905+
let epoch_ms = utils::epoch_days_to_epoch_ms(date.to_epoch_days(), ms);
907906
epoch_ms as i128 * 1_000_000 + time.microsecond as i128 * 1_000 + time.nanosecond as i128
908907
}
909908

0 commit comments

Comments
 (0)