Ensure Of is always valid

pitdicker · pitdicker · commit 83b24c94c250 · 2023-05-15T15:29:38.000+02:00
diff --git a/src/naive/date.rs b/src/naive/date.rs
@@ -244,8 +244,8 @@ impl NaiveDate {
     }
     /// Makes a new `NaiveDate` from year and packed ordinal-flags, with a verification.
     fn from_of(year: i32, of: Of) -> Option<NaiveDate> {
-        if (MIN_YEAR..=MAX_YEAR).contains(&year) && of.valid() {
-            let Of(of) = of;
+        if (MIN_YEAR..=MAX_YEAR).contains(&year) {
+            let of = of.inner();
             Some(NaiveDate { ymdf: (year << 13) | (of as DateImpl) })
         } else {
             None
@@ -254,7 +254,7 @@ impl NaiveDate {
 
     /// Makes a new `NaiveDate` from year and packed month-day-flags, with a verification.
     fn from_mdf(year: i32, mdf: Mdf) -> Option<NaiveDate> {
-        NaiveDate::from_of(year, mdf.to_of())
+        NaiveDate::from_of(year, mdf.to_of()?)
     }
 
     /// Makes a new `NaiveDate` from the [calendar date](#calendar-date)
@@ -925,28 +925,24 @@ impl NaiveDate {
     /// Returns the packed ordinal-flags.
     #[inline]
     const fn of(&self) -> Of {
-        Of((self.ymdf & 0b1_1111_1111_1111) as u32)
+        Of::from_date_impl(self.ymdf)
     }
 
     /// Makes a new `NaiveDate` with the packed month-day-flags changed.
     ///
     /// Returns `None` when the resulting `NaiveDate` would be invalid.
     #[inline]
     fn with_mdf(&self, mdf: Mdf) -> Option<NaiveDate> {
-        self.with_of(mdf.to_of())
+        Some(self.with_of(mdf.to_of()?))
     }
 
     /// Makes a new `NaiveDate` with the packed ordinal-flags changed.
     ///
     /// Returns `None` when the resulting `NaiveDate` would be invalid.
+    /// Does not check if the year flags match the year.
     #[inline]
-    fn with_of(&self, of: Of) -> Option<NaiveDate> {
-        if of.valid() {
-            let Of(of) = of;
-            Some(NaiveDate { ymdf: (self.ymdf & !0b1_1111_1111_1111) | of as DateImpl })
-        } else {
-            None
-        }
+    const fn with_of(&self, of: Of) -> NaiveDate {
+        NaiveDate { ymdf: (self.ymdf & !0b1_1111_1111_1111) | of.inner() as DateImpl }
     }
 
     /// Makes a new `NaiveDate` for the next calendar date.
@@ -975,7 +971,10 @@ impl NaiveDate {
     #[inline]
     #[must_use]
     pub fn succ_opt(&self) -> Option<NaiveDate> {
-        self.with_of(self.of().succ()).or_else(|| NaiveDate::from_ymd_opt(self.year() + 1, 1, 1))
+        match self.of().succ() {
+            Some(of) => Some(self.with_of(of)),
+            None => NaiveDate::from_ymd_opt(self.year() + 1, 1, 1),
+        }
     }
 
     /// Makes a new `NaiveDate` for the previous calendar date.
@@ -1004,7 +1003,10 @@ impl NaiveDate {
     #[inline]
     #[must_use]
     pub fn pred_opt(&self) -> Option<NaiveDate> {
-        self.with_of(self.of().pred()).or_else(|| NaiveDate::from_ymd_opt(self.year() - 1, 12, 31))
+        match self.of().pred() {
+            Some(of) => Some(self.with_of(of)),
+            None => NaiveDate::from_ymd_opt(self.year() - 1, 12, 31),
+        }
     }
 
     /// Adds the `days` part of given `Duration` to the current date.
@@ -1623,7 +1625,7 @@ impl Datelike for NaiveDate {
     /// ```
     #[inline]
     fn with_ordinal(&self, ordinal: u32) -> Option<NaiveDate> {
-        self.with_of(self.of().with_ordinal(ordinal)?)
+        self.of().with_ordinal(ordinal).map(|of| self.with_of(of))
     }
 
     /// Makes a new `NaiveDate` with the day of year (starting from 0) changed.
@@ -1648,7 +1650,7 @@ impl Datelike for NaiveDate {
     #[inline]
     fn with_ordinal0(&self, ordinal0: u32) -> Option<NaiveDate> {
         let ordinal = ordinal0.checked_add(1)?;
-        self.with_of(self.of().with_ordinal(ordinal)?)
+        self.with_ordinal(ordinal)
     }
 }
 
diff --git a/src/naive/internals.rs b/src/naive/internals.rs
@@ -18,7 +18,7 @@
 use crate::Weekday;
 use core::{fmt, i32};
 
-/// The internal date representation. This also includes the packed `Mdf` value.
+/// The internal date representation: `year << 13 | Of`
 pub(super) type DateImpl = i32;
 
 pub(super) const MAX_YEAR: DateImpl = i32::MAX >> 13;
@@ -172,8 +172,9 @@ impl fmt::Debug for YearFlags {
     }
 }
 
+// OL: (ordinal << 1) | leap year flag
 pub(super) const MIN_OL: u32 = 1 << 1;
-pub(super) const MAX_OL: u32 = 366 << 1; // larger than the non-leap last day `(365 << 1) | 1`
+pub(super) const MAX_OL: u32 = 366 << 1; // `(366 << 1) | 1` would be day 366 in a non-leap year
 pub(super) const MAX_MDL: u32 = (12 << 6) | (31 << 1) | 1;
 
 const XX: i8 = -128;
@@ -265,58 +266,70 @@ const OL_TO_MDL: &[u8; MAX_OL as usize + 1] = &[
 ///
 /// The whole bits except for the least 3 bits are referred as `Ol` (ordinal and leap flag),
 /// which is an index to the `OL_TO_MDL` lookup table.
+///
+/// The methods implemented on `Of` always return a valid value.
 #[derive(PartialEq, PartialOrd, Copy, Clone)]
-pub(super) struct Of(pub(crate) u32);
+pub(super) struct Of(u32);
 
 impl Of {
     #[inline]
     pub(super) const fn new(ordinal: u32, YearFlags(flags): YearFlags) -> Option<Of> {
-        match ordinal <= 366 {
-            true => Some(Of((ordinal << 4) | flags as u32)),
-            false => None,
-        }
+        let of = Of((ordinal << 4) | flags as u32);
+        of.validate()
+    }
+
+    pub(super) const fn from_date_impl(date_impl: DateImpl) -> Of {
+        // We assume the value in the `DateImpl` is valid.
+        Of((date_impl & 0b1_1111_1111_1111) as u32)
     }
 
     #[inline]
-    pub(super) const fn from_mdf(Mdf(mdf): Mdf) -> Of {
+    pub(super) const fn from_mdf(Mdf(mdf): Mdf) -> Option<Of> {
         let mdl = mdf >> 3;
-        if mdl <= MAX_MDL {
-            // Array is indexed from `[1..=MAX_MDL]`, with a `0` index having a meaningless value.
-            let v = MDL_TO_OL[mdl as usize];
-            Of(mdf.wrapping_sub((v as i32 as u32 & 0x3ff) << 3))
-        } else {
-            // Panicking here would be reasonable, but we are just going on with a safe value.
-            Of(0)
+        if mdl > MAX_MDL {
+            // Panicking on out-of-bounds indexing would be reasonable, but just return `None`.
+            return None;
         }
+        // Array is indexed from `[1..=MAX_MDL]`, with a `0` index having a meaningless value.
+        let v = MDL_TO_OL[mdl as usize];
+        let of = Of(mdf.wrapping_sub((v as i32 as u32 & 0x3ff) << 3));
+        of.validate()
     }
 
     #[inline]
-    pub(super) const fn valid(&self) -> bool {
-        let Of(of) = *self;
-        let ol = of >> 3;
-        ol >= MIN_OL && ol <= MAX_OL
+    pub(super) const fn inner(&self) -> u32 {
+        self.0
     }
 
+    /// Returns `(ordinal << 1) | leap-year-flag`.
     #[inline]
-    pub(super) const fn ordinal(&self) -> u32 {
-        let Of(of) = *self;
-        of >> 4
+    const fn ol(&self) -> u32 {
+        self.0 >> 3
     }
 
     #[inline]
-    pub(super) const fn with_ordinal(&self, ordinal: u32) -> Option<Of> {
-        if ordinal > 366 {
-            return None;
+    const fn validate(self) -> Option<Of> {
+        let ol = self.ol();
+        match ol >= MIN_OL && ol <= MAX_OL {
+            true => Some(self),
+            false => None,
         }
+    }
 
-        let Of(of) = *self;
-        Some(Of((of & 0b1111) | (ordinal << 4)))
+    #[inline]
+    pub(super) const fn ordinal(&self) -> u32 {
+        self.0 >> 4
+    }
+
+    #[inline]
+    pub(super) const fn with_ordinal(&self, ordinal: u32) -> Option<Of> {
+        let of = Of((ordinal << 4) | (self.0 & 0b1111));
+        of.validate()
     }
 
     #[inline]
     pub(super) const fn flags(&self) -> YearFlags {
-        let Of(of) = *self;
-        YearFlags((of & 0b1111) as u8)
+        YearFlags((self.0 & 0b1111) as u8)
     }
 
     #[inline]
@@ -339,16 +352,20 @@ impl Of {
         Mdf::from_of(*self)
     }
 
+    /// Returns an `Of` with the next day, or `None` if this is the last day of the year.
     #[inline]
-    pub(super) const fn succ(&self) -> Of {
-        let Of(of) = *self;
-        Of(of + (1 << 4))
+    pub(super) const fn succ(&self) -> Option<Of> {
+        let of = Of(self.0 + (1 << 4));
+        of.validate()
     }
 
+    /// Returns an `Of` with the previous day, or `None` if this is the first day of the year.
     #[inline]
-    pub(super) const fn pred(&self) -> Of {
-        let Of(of) = *self;
-        Of(of - (1 << 4))
+    pub(super) const fn pred(&self) -> Option<Of> {
+        if self.ordinal() == 1 {
+            return None;
+        }
+        Some(Of(self.0 - (1 << 4)))
     }
 }
 
@@ -370,13 +387,17 @@ impl fmt::Debug for Of {
 /// The whole bits except for the least 3 bits are referred as `Mdl`
 /// (month, day of month and leap flag),
 /// which is an index to the `MDL_TO_OL` lookup table.
+///
+/// The methods implemented on `Mdf` do not always return a valid value.
+/// Dates than can't exist, like February 30, can still be represented.
+/// Use `Mdl::valid` to check whether the date is valid.
 #[derive(PartialEq, PartialOrd, Copy, Clone)]
 pub(super) struct Mdf(u32);
 
 impl Mdf {
     #[inline]
     pub(super) const fn new(month: u32, day: u32, YearFlags(flags): YearFlags) -> Option<Mdf> {
-        match month <= 12 && day <= 31 {
+        match month >= 1 && month <= 12 && day >= 1 && day <= 31 {
             true => Some(Mdf((month << 9) | (day << 4) | flags as u32)),
             false => None,
         }
@@ -447,7 +468,7 @@ impl Mdf {
 
     #[cfg_attr(feature = "cargo-clippy", allow(clippy::wrong_self_convention))]
     #[inline]
-    pub(super) const fn to_of(&self) -> Of {
+    pub(super) const fn to_of(&self) -> Option<Of> {
         Of::from_mdf(*self)
     }
 }
@@ -542,7 +563,7 @@ mod tests {
                 };
 
                 assert!(
-                    of.valid() == expected,
+                    of.validate().is_some() == expected,
                     "ordinal {} = {:?} should be {} for dominical year {:?}",
                     ordinal,
                     of,
@@ -662,8 +683,7 @@ mod tests {
     fn test_of_fields() {
         for &flags in FLAGS.iter() {
             for ordinal in range_inclusive(1u32, 366) {
-                let of = Of::new(ordinal, flags).unwrap();
-                if of.valid() {
+                if let Some(of) = Of::new(ordinal, flags) {
                     assert_eq!(of.ordinal(), ordinal);
                 }
             }
@@ -676,14 +696,9 @@ mod tests {
             let of = Of::new(ordinal, flags).unwrap();
 
             for ordinal in range_inclusive(0u32, 1024) {
-                let of = match of.with_ordinal(ordinal) {
-                    Some(of) => of,
-                    None if ordinal > 366 => continue,
-                    None => panic!("failed to create Of with ordinal {}", ordinal),
-                };
-
-                assert_eq!(of.valid(), Of::new(ordinal, flags).unwrap().valid());
-                if of.valid() {
+                let of = of.with_ordinal(ordinal);
+                assert_eq!(of, Of::new(ordinal, flags));
+                if let Some(of) = of {
                     assert_eq!(of.ordinal(), ordinal);
                 }
             }
@@ -808,25 +823,25 @@ mod tests {
     #[test]
     fn test_of_to_mdf() {
         for i in range_inclusive(0u32, 8192) {
-            let of = Of(i);
-            assert_eq!(of.valid(), of.to_mdf().valid());
+            if let Some(of) = Of(i).validate() {
+                assert!(of.to_mdf().valid());
+            }
         }
     }
 
     #[test]
     fn test_mdf_to_of() {
         for i in range_inclusive(0u32, 8192) {
             let mdf = Mdf(i);
-            assert_eq!(mdf.valid(), mdf.to_of().valid());
+            assert_eq!(mdf.valid(), mdf.to_of().is_some());
         }
     }
 
     #[test]
     fn test_of_to_mdf_to_of() {
         for i in range_inclusive(0u32, 8192) {
-            let of = Of(i);
-            if of.valid() {
-                assert_eq!(of, of.to_mdf().to_of());
+            if let Some(of) = Of(i).validate() {
+                assert_eq!(of, of.to_mdf().to_of().unwrap());
             }
         }
     }
@@ -836,11 +851,40 @@ mod tests {
         for i in range_inclusive(0u32, 8192) {
             let mdf = Mdf(i);
             if mdf.valid() {
-                assert_eq!(mdf, mdf.to_of().to_mdf());
+                assert_eq!(mdf, mdf.to_of().unwrap().to_mdf());
             }
         }
     }
 
+    #[test]
+    fn test_invalid_returns_none() {
+        let regular_year = YearFlags::from_year(2023);
+        let leap_year = YearFlags::from_year(2024);
+        assert!(Of::new(0, regular_year).is_none());
+        assert!(Of::new(366, regular_year).is_none());
+        assert!(Of::new(366, leap_year).is_some());
+        assert!(Of::new(367, regular_year).is_none());
+
+        assert!(Mdf::new(0, 1, regular_year).is_none());
+        assert!(Mdf::new(13, 1, regular_year).is_none());
+        assert!(Mdf::new(1, 0, regular_year).is_none());
+        assert!(Mdf::new(1, 32, regular_year).is_none());
+        assert!(Mdf::new(2, 31, regular_year).is_some());
+
+        assert!(Of::from_mdf(Mdf::new(2, 30, regular_year).unwrap()).is_none());
+        assert!(Of::from_mdf(Mdf::new(2, 30, leap_year).unwrap()).is_none());
+        assert!(Of::from_mdf(Mdf::new(2, 29, regular_year).unwrap()).is_none());
+        assert!(Of::from_mdf(Mdf::new(2, 29, leap_year).unwrap()).is_some());
+        assert!(Of::from_mdf(Mdf::new(2, 28, regular_year).unwrap()).is_some());
+
+        assert!(Of::new(365, regular_year).unwrap().succ().is_none());
+        assert!(Of::new(365, leap_year).unwrap().succ().is_some());
+        assert!(Of::new(366, leap_year).unwrap().succ().is_none());
+
+        assert!(Of::new(1, regular_year).unwrap().pred().is_none());
+        assert!(Of::new(1, leap_year).unwrap().pred().is_none());
+    }
+
     #[test]
     fn test_weekday_from_u32_mod7() {
         for i in 0..=1000 {
