From 8caedc3890122a3d956e4f63ea5a3781ebcde041 Mon Sep 17 00:00:00 2001 From: Jacob Pratt Date: Mon, 27 Jan 2020 19:18:34 -0500 Subject: [PATCH] Fix behavior for `OffsetDateTime` The previous behavior was ambiguous, as it was unclear whether the provided offset was the one being assumed and being converting to UTC or vice versa. This ambiguity extended to within the code, so there were some bugs where the behavior was backwards. These have been fixed, as it's now clearly documented that the datetime stored in the struct is the UTC representation, not local. --- Cargo.toml | 2 +- src/offset_date_time.rs | 124 ++++++++++++++++++++++++------------- src/primitive_date_time.rs | 23 +++---- 3 files changed, 91 insertions(+), 58 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index dfbd72a28..8109ea4f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "time" -version = "0.2.4" +version = "0.2.5" authors = ["Jacob Pratt "] edition = "2018" repository = "https://github.com/time-rs/time" diff --git a/src/offset_date_time.rs b/src/offset_date_time.rs index 1b61bb054..040022e6e 100644 --- a/src/offset_date_time.rs +++ b/src/offset_date_time.rs @@ -13,8 +13,11 @@ use core::{ /// A [`PrimitiveDateTime`] with a [`UtcOffset`]. /// -/// For equality, comparisons, and hashing, calculations are performed using the -/// [Unix timestamp](https://en.wikipedia.org/wiki/Unix_time). +/// All comparisons are performed using the UTC time. +// Internally, an `OffsetDateTime` is a thin wrapper around a +// [`PrimitiveDateTime`] coupled with a [`UtcOffset`]. This offset is added to +// the date, time, or datetime as necessary for presentation or returning from a +// function. #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr( feature = "serde", @@ -26,7 +29,7 @@ use core::{ #[derive(Debug, Clone, Copy, Eq)] pub struct OffsetDateTime { /// The `PrimitiveDateTime`, which is _always_ UTC. - pub(crate) datetime: PrimitiveDateTime, + pub(crate) utc_datetime: PrimitiveDateTime, /// The `UtcOffset`, which will be added to the `PrimitiveDateTime` as necessary. pub(crate) offset: UtcOffset, } @@ -62,7 +65,10 @@ impl OffsetDateTime { /// ``` #[inline(always)] pub const fn to_offset(self, offset: UtcOffset) -> Self { - self.datetime.using_offset(offset) + Self { + utc_datetime: self.utc_datetime, + offset, + } } /// Midnight, 1 January, 1970 (UTC). @@ -78,7 +84,10 @@ impl OffsetDateTime { /// ``` #[inline(always)] pub const fn unix_epoch() -> Self { - PrimitiveDateTime::unix_epoch().using_offset(offset!(UTC)) + Self { + utc_datetime: PrimitiveDateTime::unix_epoch(), + offset: offset!(UTC), + } } /// Create an `OffsetDateTime` from the provided [Unix timestamp](https://en.wikipedia.org/wiki/Unix_time). @@ -91,7 +100,7 @@ impl OffsetDateTime { /// ); /// assert_eq!( /// OffsetDateTime::from_unix_timestamp(1_546_300_800), - /// date!(2019-01-1) + /// date!(2019-01-01) /// .midnight() /// .using_offset(offset!(UTC)), /// ); @@ -144,7 +153,7 @@ impl OffsetDateTime { /// ``` #[inline(always)] pub fn timestamp(self) -> i64 { - self.datetime.timestamp() - self.offset.as_seconds() as i64 + self.utc_datetime.timestamp() } /// Get the `Date` in the stored offset. @@ -160,15 +169,15 @@ impl OffsetDateTime { /// ); /// assert_eq!( /// date!(2019-01-01) - /// .midnight() + /// .with_time(time!(23:00:00)) /// .using_offset(offset!(-1)) /// .date(), - /// date!(2018-12-31), + /// date!(2019-01-01), /// ); /// ``` #[inline(always)] pub fn date(self) -> Date { - (self.datetime + self.offset.as_duration()).date() + (self.utc_datetime + self.offset.as_duration()).date() } /// Get the `Time` in the stored offset. @@ -187,12 +196,12 @@ impl OffsetDateTime { /// .midnight() /// .using_offset(offset!(-1)) /// .time(), - /// time!(23:00) + /// time!(0:00) /// ); /// ``` #[inline(always)] pub fn time(self) -> Time { - (self.datetime + self.offset.as_duration()).time() + (self.utc_datetime + self.offset.as_duration()).time() } /// Get the year of the date in the stored offset. @@ -247,7 +256,7 @@ impl OffsetDateTime { /// .with_time(time!(23:00)) /// .using_offset(offset!(+1)) /// .month(), - /// 1, + /// 12, /// ); /// ``` #[inline(always)] @@ -274,7 +283,7 @@ impl OffsetDateTime { /// .with_time(time!(23:00)) /// .using_offset(offset!(+1)) /// .day(), - /// 1, + /// 31, /// ); /// ``` #[inline(always)] @@ -301,7 +310,7 @@ impl OffsetDateTime { /// .with_time(time!(23:00)) /// .using_offset(offset!(+1)) /// .month_day(), - /// (1, 1), + /// (12, 31), /// ); /// ``` #[inline(always)] @@ -327,7 +336,7 @@ impl OffsetDateTime { /// .with_time(time!(23:00)) /// .using_offset(offset!(+1)) /// .ordinal(), - /// 1, + /// 365, /// ); /// ``` #[inline(always)] @@ -472,7 +481,7 @@ impl OffsetDateTime { /// .with_time(time!(23:59:59)) /// .using_offset(offset!(-2)) /// .hour(), - /// 21, + /// 23, /// ); /// ``` #[inline(always)] @@ -498,7 +507,7 @@ impl OffsetDateTime { /// .with_time(time!(23:59:59)) /// .using_offset(offset!(+0:30)) /// .minute(), - /// 29, + /// 59, /// ); /// ``` #[inline(always)] @@ -524,7 +533,7 @@ impl OffsetDateTime { /// .with_time(time!(23:59:59)) /// .using_offset(offset!(+0:00:30)) /// .second(), - /// 29, + /// 59, /// ); /// ``` #[inline(always)] @@ -661,9 +670,11 @@ impl OffsetDateTime { /// Given the items already parsed, attempt to create an `OffsetDateTime`. #[inline(always)] pub(crate) fn try_from_parsed_items(items: ParsedItems) -> ParseResult { + let offset = UtcOffset::try_from_parsed_items(items)?; + Ok(Self { - datetime: PrimitiveDateTime::try_from_parsed_items(items)?, - offset: UtcOffset::try_from_parsed_items(items)?, + utc_datetime: PrimitiveDateTime::try_from_parsed_items(items)? - offset.as_duration(), + offset, }) } } @@ -671,7 +682,7 @@ impl OffsetDateTime { impl PartialEq for OffsetDateTime { #[inline(always)] fn eq(&self, rhs: &Self) -> bool { - self.timestamp() == rhs.timestamp() + self.utc_datetime.eq(&rhs.utc_datetime) } } @@ -685,14 +696,17 @@ impl PartialOrd for OffsetDateTime { impl Ord for OffsetDateTime { #[inline(always)] fn cmp(&self, rhs: &Self) -> Ordering { - self.timestamp().cmp(&rhs.timestamp()) + self.utc_datetime.cmp(&(rhs.utc_datetime)) } } impl Hash for OffsetDateTime { #[inline(always)] fn hash(&self, hasher: &mut H) { - hasher.write_i64(self.timestamp()); + // We need to distinguish this from a `PrimitiveDateTime`, which would + // otherwise conflict. + hasher.write(b"OffsetDateTime"); + self.utc_datetime.hash(hasher); } } @@ -701,7 +715,7 @@ impl Add for OffsetDateTime { #[inline(always)] fn add(self, duration: Duration) -> Self::Output { - (self.datetime + duration).using_offset(self.offset) + (self.utc_datetime + duration).using_offset(self.offset) } } @@ -710,7 +724,7 @@ impl Add for OffsetDateTime { #[inline(always)] fn add(self, duration: StdDuration) -> Self::Output { - (self.datetime + duration).using_offset(self.offset) + (self.utc_datetime + duration).using_offset(self.offset) } } @@ -733,7 +747,7 @@ impl Sub for OffsetDateTime { #[inline(always)] fn sub(self, duration: Duration) -> Self::Output { - (self.datetime - duration).using_offset(self.offset) + (self.utc_datetime - duration).using_offset(self.offset) } } @@ -742,7 +756,7 @@ impl Sub for OffsetDateTime { #[inline(always)] fn sub(self, duration: StdDuration) -> Self::Output { - (self.datetime - duration).using_offset(self.offset) + (self.utc_datetime - duration).using_offset(self.offset) } } @@ -765,13 +779,14 @@ impl Sub for OffsetDateTime { #[inline(always)] fn sub(self, rhs: Self) -> Self::Output { - Duration::seconds(self.timestamp() - rhs.timestamp()) + self.utc_datetime - rhs.utc_datetime } } +// TODO A number of tests need to be updated once `assume_offset` is +// implemented. #[cfg(test)] #[rustfmt::skip::macros(date)] -#[allow(clippy::zero_prefixed_literal, clippy::result_unwrap_used)] mod test { use super::*; use crate::{date, prelude::*, time}; @@ -825,7 +840,7 @@ mod test { offset!(UTC), ); assert_eq!( - date!(2019-1-1) + date!(2019-01-01) .midnight() .using_offset(offset!(+1)) .offset(), @@ -840,7 +855,7 @@ mod test { PrimitiveDateTime::unix_epoch() .using_offset(offset!(-1)) .timestamp(), - 3_600, + 3600, ); } @@ -855,10 +870,10 @@ mod test { ); assert_eq!( date!(2019-01-01) - .midnight() + .with_time(time!(23:00:00)) .using_offset(offset!(-1)) .date(), - date!(2018-12-31), + date!(2019-01-01), ); } @@ -874,7 +889,8 @@ mod test { assert_eq!( date!(2019-01-01) .midnight() - .using_offset(offset!(-1)) + .using_offset(offset!(UTC)) + .to_offset(offset!(-1)) .time(), time!(23:00), ); @@ -920,7 +936,7 @@ mod test { .with_time(time!(23:00)) .using_offset(offset!(+1)) .month(), - 1, + 12, ); } @@ -938,7 +954,7 @@ mod test { .with_time(time!(23:00)) .using_offset(offset!(+1)) .day(), - 1, + 31, ); } @@ -956,7 +972,7 @@ mod test { .with_time(time!(23:00)) .using_offset(offset!(+1)) .month_day(), - (1, 1), + (12, 31), ); } @@ -974,7 +990,7 @@ mod test { .with_time(time!(23:00)) .using_offset(offset!(+1)) .ordinal(), - 1, + 365, ); } @@ -1050,7 +1066,7 @@ mod test { .with_time(time!(23:59:59)) .using_offset(UtcOffset::hours(-2)) .hour(), - 21, + 23, ); } @@ -1068,7 +1084,7 @@ mod test { .with_time(time!(23:59:59)) .using_offset(offset!(+0:30)) .minute(), - 29, + 59, ); } @@ -1086,7 +1102,7 @@ mod test { .with_time(time!(23:59:59)) .using_offset(offset!(+0:00:30)) .second(), - 29, + 59, ); } @@ -1201,6 +1217,12 @@ mod test { .with_time(time!(23:00)) .using_offset(offset!(-1)); assert_eq!(t1, t2); + + let t1 = date!(2019-01-01).midnight().using_offset(offset!(UTC)); + let t2 = date!(2019-01-01) + .with_time(time!(0:00:00.000_000_001)) + .using_offset(offset!(UTC)); + assert!(t2 > t1); } #[test] @@ -1226,6 +1248,24 @@ mod test { hasher.finish() } ); + + // Ensure that a `PrimitiveDateTime` and `OffsetDateTime` don't collide, + // even if the UTC time is the same. + assert_ne!( + { + let mut hasher = DefaultHasher::new(); + date!(2019-01-01).midnight().hash(&mut hasher); + hasher.finish() + }, + { + let mut hasher = DefaultHasher::new(); + date!(2019-01-01) + .midnight() + .using_offset(offset!(UTC)) + .hash(&mut hasher); + hasher.finish() + } + ); } #[test] diff --git a/src/primitive_date_time.rs b/src/primitive_date_time.rs index 90b110d64..d2807c246 100644 --- a/src/primitive_date_time.rs +++ b/src/primitive_date_time.rs @@ -387,8 +387,8 @@ impl PrimitiveDateTime { self.time().nanosecond() } - /// Create an `OffsetDateTime` from the existing `PrimitiveDateTime` and provided - /// `UtcOffset`. + /// Assuming that the existing `PrimitiveDateTime` represents a moment in + /// the provided `UtcOffset`, return an `OffsetDateTime`. /// /// ```rust /// # use time::{date, offset}; @@ -398,9 +398,10 @@ impl PrimitiveDateTime { /// ); /// ``` #[inline(always)] - pub const fn using_offset(self, offset: UtcOffset) -> OffsetDateTime { + pub fn using_offset(self, offset: UtcOffset) -> OffsetDateTime { OffsetDateTime { - datetime: self, + // Subtract the offset to get the time at UTC. + utc_datetime: self - offset.as_duration(), offset, } } @@ -668,17 +669,9 @@ impl Ord for PrimitiveDateTime { #[inline] fn cmp(&self, other: &Self) -> Ordering { match self.date.cmp(&other.date) { - Ordering::Equal => match self.time.hour.cmp(&other.time.hour) { - Ordering::Equal => match self.time.minute.cmp(&other.time.minute) { - Ordering::Equal => match self.time.second.cmp(&other.time.second) { - Ordering::Equal => self.time.nanosecond.cmp(&other.time.nanosecond), - other => other, - }, - other => other, - }, - other => other, - }, - other => other, + Ordering::Less => Ordering::Less, + Ordering::Equal => self.time.cmp(&other.time), + Ordering::Greater => Ordering::Greater, } } }