Review feedback and get_option changes

boa-dev · Oct 5, 2023 · 1d6bf18 · 1d6bf18
1 parent dc311e6
commit 1d6bf18
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 59 deletions.
diff --git a/boa_engine/src/builtins/temporal/calendar/mod.rs b/boa_engine/src/builtins/temporal/calendar/mod.rs
@@ -371,9 +371,8 @@ impl Calendar {
         };
 
         // 8. Let overflow be ? ToTemporalOverflow(options).
-        let overflow =
-            get_option::<ArithmeticOverflow>(&options, utf16!("overflow"), false, context)?
-                .unwrap_or(ArithmeticOverflow::Constrain);
+        let overflow = get_option(&options, utf16!("overflow"), context)?
+            .unwrap_or(ArithmeticOverflow::Constrain);
 
         // NOTE: implement the below on the calenar itself
         // 9. If calendar.[[Identifier]] is "iso8601", then
@@ -453,9 +452,8 @@ impl Calendar {
         };
 
         // 7. Let overflow be ? ToTemporalOverflow(options).
-        let overflow =
-            get_option::<ArithmeticOverflow>(&options, utf16!("overflow"), false, context)?
-                .unwrap_or(ArithmeticOverflow::Constrain);
+        let overflow = get_option::<ArithmeticOverflow>(&options, utf16!("overflow"), context)?
+            .unwrap_or(ArithmeticOverflow::Constrain);
 
         this_calendar.year_month_from_fields(&mut fields, overflow, context)
     }
@@ -530,9 +528,8 @@ impl Calendar {
         };
 
         // 8. Let overflow be ? ToTemporalOverflow(options).
-        let overflow =
-            get_option::<ArithmeticOverflow>(&options, utf16!("overflow"), false, context)?
-                .unwrap_or(ArithmeticOverflow::Constrain);
+        let overflow = get_option(&options, utf16!("overflow"), context)?
+            .unwrap_or(ArithmeticOverflow::Constrain);
 
         this_calendar.month_day_from_fields(&mut fields, overflow, context)
     }
@@ -569,9 +566,8 @@ impl Calendar {
         let options_obj = get_options_object(options)?;
 
         // 7. Let overflow be ? ToTemporalOverflow(options).
-        let overflow =
-            get_option::<ArithmeticOverflow>(&options_obj, utf16!("overflow"), false, context)?
-                .unwrap_or(ArithmeticOverflow::Constrain);
+        let overflow = get_option(&options_obj, utf16!("overflow"), context)?
+            .unwrap_or(ArithmeticOverflow::Constrain);
 
         // 8. Let balanceResult be ? BalanceTimeDuration(duration.[[Days]], duration.[[Hours]], duration.[[Minutes]], duration.[[Seconds]], duration.[[Milliseconds]], duration.[[Microseconds]], duration.[[Nanoseconds]], "day").
         duration.balance_time_duration(TemporalUnit::Day, None)?;
@@ -620,13 +616,10 @@ impl Calendar {
             Some(TemporalUnit::Day),
             None,
             context,
-        )?;
-
-        let Some(largest_unit) = largest_unit else {
-            return Err(JsNativeError::range()
-                .with_message("largestUnit cannot be undefined in this context.")
-                .into());
-        };
+        )?
+        .ok_or_else(|| {
+            JsNativeError::range().with_message("largestUnit cannot be undefined in this context.")
+        })?;
 
         this_calendar.date_until(&one, &two, largest_unit, context)
     }

diff --git a/boa_engine/src/builtins/temporal/date_equations.rs b/boa_engine/src/builtins/temporal/date_equations.rs
@@ -55,9 +55,16 @@ pub(crate) fn mathematical_in_leap_year(t: f64) -> i32 {
 
 pub(crate) fn epoch_time_to_month_in_year(t: f64) -> i32 {
     const DAYS: [i32; 11] = [30, 58, 89, 120, 150, 181, 212, 242, 272, 303, 333];
+    const LEAP_DAYS: [i32; 11] = [30, 59, 90, 121, 151, 182, 213, 242, 272, 303, 334];
+
+    let in_leap_year = mathematical_in_leap_year(t) == 1;
     let day = epoch_time_to_day_in_year(t);
 
-    let result = DAYS.binary_search(&day);
+    let result = if in_leap_year {
+        LEAP_DAYS.binary_search(&day)
+    } else {
+        DAYS.binary_search(&day)
+    };
 
     match result {
         Ok(i) | Err(i) => i as i32,

diff --git a/boa_engine/src/builtins/temporal/duration/mod.rs b/boa_engine/src/builtins/temporal/duration/mod.rs
@@ -610,10 +610,10 @@ impl Duration {
             }
         };
 
+        // NOTE: 6 & 7 unused in favor of `is_none()`.
         // 6. Let smallestUnitPresent be true.
-        let mut smallest_unit_present = true;
         // 7. Let largestUnitPresent be true.
-        let mut largest_unit_present = true;
+
         // 8. NOTE: The following steps read options and perform independent validation in alphabetical order
         //   (ToRelativeTemporalObject reads "relativeTo", ToTemporalRoundingIncrement reads "roundingIncrement" and ToTemporalRoundingMode reads "roundingMode").
 
@@ -635,9 +635,8 @@ impl Duration {
         let rounding_increment = get_temporal_rounding_increment(&round_to, context)?;
 
         // 12. Let roundingMode be ? ToTemporalRoundingMode(roundTo, "halfExpand").
-        let rounding_mode =
-            get_option::<RoundingMode>(&round_to, utf16!("roundingMode"), false, context)?
-                .unwrap_or(RoundingMode::HalfExpand);
+        let rounding_mode = get_option(&round_to, utf16!("roundingMode"), context)?
+            .unwrap_or(RoundingMode::HalfExpand);
 
         // 13. Let smallestUnit be ? GetTemporalUnit(roundTo, "smallestUnit", datetime, undefined).
         let smallest_unit = get_temporal_unit(
@@ -650,12 +649,20 @@ impl Duration {
             context,
         )?;
 
+        // NOTE: execute step 19 earlier before initial values are shadowed.
+        // 19. If smallestUnitPresent is false and largestUnitPresent is false, then
+        if smallest_unit.is_none() && largest_unit.is_none() {
+            // a. Throw a RangeError exception.
+            return Err(JsNativeError::range()
+                .with_message("smallestUnit or largestUnit must be present.")
+                .into());
+        }
+
         // 14. If smallestUnit is undefined, then
         let smallest_unit = if let Some(unit) = smallest_unit {
             unit
         } else {
             // a. Set smallestUnitPresent to false.
-            smallest_unit_present = false;
             // b. Set smallestUnit to "nanosecond".
             TemporalUnit::Nanosecond
         };
@@ -672,20 +679,11 @@ impl Duration {
             Some(u) => u,
             None => {
                 // a. Set largestUnitPresent to false.
-                largest_unit_present = false;
                 // b. Set largestUnit to defaultLargestUnit.
                 default_largest_unit
             }
         };
 
-        // 19. If smallestUnitPresent is false and largestUnitPresent is false, then
-        if !smallest_unit_present && !largest_unit_present {
-            // a. Throw a RangeError exception.
-            return Err(JsNativeError::range()
-                .with_message("smallestUnit or largestUnit must be present.")
-                .into());
-        }
-
         // 20. If LargerOfTwoTemporalUnits(largestUnit, smallestUnit) is not largestUnit, throw a RangeError exception.
         if core::cmp::max(largest_unit, smallest_unit) != largest_unit {
             return Err(JsNativeError::range()

diff --git a/boa_engine/src/builtins/temporal/instant/mod.rs b/boa_engine/src/builtins/temporal/instant/mod.rs
@@ -366,8 +366,7 @@ impl Instant {
 
         // 8. Let roundingMode be ? ToTemporalRoundingMode(roundTo, "halfExpand").
         let rounding_mode =
-            get_option::<RoundingMode>(&round_to, utf16!("roundingMode"), false, context)?
-                .unwrap_or_default();
+            get_option(&round_to, utf16!("roundingMode"), context)?.unwrap_or_default();
 
         // 9. Let smallestUnit be ? GetTemporalUnit(roundTo, "smallestUnit"), time, required).
         let smallest_unit = get_temporal_unit(
@@ -378,13 +377,8 @@ impl Instant {
             None,
             None,
             context,
-        )?;
-
-        let Some(smallest_unit) = smallest_unit else {
-            return Err(JsNativeError::range()
-                .with_message("smallestUnit cannot be undefined.")
-                .into());
-        };
+        )?
+        .ok_or_else(|| JsNativeError::range().with_message("smallestUnit cannot be undefined."))?;
 
         let maximum = match smallest_unit {
             // 10. If smallestUnit is "hour"), then

diff --git a/boa_engine/src/builtins/temporal/mod.rs b/boa_engine/src/builtins/temporal/mod.rs
@@ -525,10 +525,10 @@ pub(crate) fn get_diff_settings(
 
     // 4. Let roundingIncrement be ? ToTemporalRoundingIncrement(options).
     let rounding_increment = get_temporal_rounding_increment(options, context)?;
+
     // 5. Let roundingMode be ? ToTemporalRoundingMode(options, "trunc").
     let mut rounding_mode =
-        get_option::<RoundingMode>(options, utf16!("roundingMode"), false, context)?
-            .unwrap_or(RoundingMode::Trunc);
+        get_option(options, utf16!("roundingMode"), context)?.unwrap_or(RoundingMode::Trunc);
 
     // 6. If operation is since, then
     if !op {

diff --git a/boa_engine/src/builtins/temporal/options.rs b/boa_engine/src/builtins/temporal/options.rs
@@ -66,7 +66,7 @@ pub(crate) fn get_temporal_unit(
     let mut unit_values = unit_group.group();
     unit_values.extend(extra);
 
-    let unit = get_option::<TemporalUnit>(options, key, required, context)?.map_or(default, Some);
+    let unit = get_option(options, key, context)?.map_or(default, Some);
 
     if let Some(u) = &unit {
         if !unit_values.contains(u) {

diff --git a/boa_engine/src/builtins/temporal/plain_date/mod.rs b/boa_engine/src/builtins/temporal/plain_date/mod.rs
@@ -478,9 +478,8 @@ pub(crate) fn to_temporal_date(
             // c. If item has an [[InitializedTemporalDateTime]] internal slot, then
         } else if object.is_plain_date_time() {
             // i. Perform ? ToTemporalOverflow(options).
-            let _o =
-                get_option::<ArithmeticOverflow>(&options_obj, utf16!("overflow"), false, context)?
-                    .unwrap_or(ArithmeticOverflow::Constrain);
+            let _o = get_option(&options_obj, utf16!("overflow"), context)?
+                .unwrap_or(ArithmeticOverflow::Constrain);
 
             let obj = object.borrow();
             let date_time = obj
@@ -537,9 +536,8 @@ pub(crate) fn to_temporal_date(
             // 11. Set calendar to the ASCII-lowercase of calendar.
 
             // 12. Perform ? ToTemporalOverflow(options).
-            let _result =
-                get_option::<ArithmeticOverflow>(&options_obj, utf16!("overflow"), false, context)?
-                    .unwrap_or(ArithmeticOverflow::Constrain);
+            let _result = get_option(&options_obj, utf16!("overflow"), context)?
+                .unwrap_or(ArithmeticOverflow::Constrain);
 
             // 13. Return ? CreateTemporalDate(result.[[Year]], result.[[Month]], result.[[Day]], calendar).
             Ok(PlainDate {

diff --git a/boa_engine/src/builtins/temporal/tests.rs b/boa_engine/src/builtins/temporal/tests.rs
@@ -1,4 +1,4 @@
-use super::date_equations::epoch_time_to_month_in_year;
+use super::date_equations::{epoch_time_to_month_in_year, mathematical_in_leap_year};
 use crate::{js_string, run_test_actions, JsValue, TestAction};
 
 // Temporal Object tests.
@@ -37,10 +37,16 @@ fn now_object() {
 
 #[test]
 fn time_to_month() {
-    let milliseconds = [1696459917000_f64];
+    let oct_2023 = 1_696_459_917_000_f64;
+    let mar_1_2020 = 1_583_020_800_000_f64;
+    let feb_29_2020 = 1_582_934_400_000_f64;
+    let mar_1_2021 = 1_614_556_800_000_f64;
 
-    for test_epochs in milliseconds {
-        println!("{}", epoch_time_to_month_in_year(test_epochs));
-        assert_eq!(epoch_time_to_month_in_year(test_epochs), 9);
-    }
+    assert_eq!(epoch_time_to_month_in_year(oct_2023), 9);
+    assert_eq!(epoch_time_to_month_in_year(mar_1_2020), 2);
+    assert_eq!(mathematical_in_leap_year(mar_1_2020), 1);
+    assert_eq!(epoch_time_to_month_in_year(feb_29_2020), 1);
+    assert_eq!(mathematical_in_leap_year(feb_29_2020), 1);
+    assert_eq!(epoch_time_to_month_in_year(mar_1_2021), 2);
+    assert_eq!(mathematical_in_leap_year(mar_1_2021), 0);
 }