-
Notifications
You must be signed in to change notification settings - Fork 29
Update to new ComputeNudgeWindow spec text #636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,8 @@ use crate::{ | |
| primitive::{DoubleDouble, FiniteF64}, | ||
| provider::TimeZoneProvider, | ||
| rounding::IncrementRounder, | ||
| Calendar, TemporalError, TemporalResult, TemporalUnwrap, NS_PER_DAY, NS_PER_DAY_NONZERO, | ||
| temporal_assert, Calendar, TemporalError, TemporalResult, TemporalUnwrap, NS_PER_DAY, | ||
| NS_PER_DAY_NONZERO, | ||
| }; | ||
|
|
||
| use super::{DateDuration, Duration, Sign}; | ||
|
|
@@ -395,18 +396,26 @@ struct NudgeRecord { | |
| expanded: bool, | ||
| } | ||
|
|
||
| struct NudgeWindow { | ||
| r1: i128, | ||
| r2: i128, | ||
| start_epoch_ns: EpochNanoseconds, | ||
| end_epoch_ns: EpochNanoseconds, | ||
| start_duration: DateDuration, | ||
| end_duration: DateDuration, | ||
| } | ||
|
|
||
| impl InternalDurationRecord { | ||
| // TODO: Add assertion into impl. | ||
| // TODO: Add unit tests specifically for nudge_calendar_unit if possible. | ||
| fn nudge_calendar_unit( | ||
| /// <https://tc39.es/proposal-temporal/#sec-temporal-computenudgewindow> | ||
| fn compute_nudge_window( | ||
| &self, | ||
| sign: Sign, | ||
| origin_epoch_ns: EpochNanoseconds, | ||
| dest_epoch_ns: i128, | ||
| dt: &PlainDateTime, | ||
| time_zone: Option<(&TimeZone, &(impl TimeZoneProvider + ?Sized))>, // ??? | ||
| options: ResolvedRoundingOptions, | ||
| ) -> TemporalResult<NudgeRecord> { | ||
| additional_shift: bool, | ||
| ) -> TemporalResult<NudgeWindow> { | ||
| // NOTE: r2 may never be used...need to test. | ||
| let (r1, r2, start_duration, end_duration) = match options.smallest_unit { | ||
| // 1. If unit is "year", then | ||
|
|
@@ -417,11 +426,18 @@ impl InternalDurationRecord { | |
| options.increment.as_extended_increment(), | ||
| )? | ||
| .round(RoundingMode::Trunc); | ||
| // b. Let r1 be years. | ||
| let r1 = years; | ||
| // c. Let r2 be years + increment × sign. | ||
| let r2 = years | ||
| + i128::from(options.increment.get()) * i128::from(sign.as_sign_multiplier()); | ||
| let increment_x_sign = | ||
| i128::from(options.increment.get()) * i128::from(sign.as_sign_multiplier()); | ||
| // b. If additionalShift is false, then | ||
| let r1 = if !additional_shift { | ||
| // i. Let r1 be years. | ||
| years | ||
| } else { | ||
| // i. Let r1 be years + increment × sign. | ||
| years + increment_x_sign | ||
| }; | ||
| // c. Let r2 be r1 + increment × sign. | ||
| let r2 = r1 + increment_x_sign; | ||
| // d. Let startDuration be ? CreateNormalizedDurationRecord(r1, 0, 0, 0, ZeroTimeDuration()). | ||
| // e. Let endDuration be ? CreateNormalizedDurationRecord(r2, 0, 0, 0, ZeroTimeDuration()). | ||
| ( | ||
|
|
@@ -449,11 +465,18 @@ impl InternalDurationRecord { | |
| options.increment.as_extended_increment(), | ||
| )? | ||
| .round(RoundingMode::Trunc); | ||
| // b. Let r1 be months. | ||
| let r1 = months; | ||
| // c. Let r2 be months + increment × sign. | ||
| let r2 = months | ||
| + i128::from(options.increment.get()) * i128::from(sign.as_sign_multiplier()); | ||
| let increment_x_sign = | ||
| i128::from(options.increment.get()) * i128::from(sign.as_sign_multiplier()); | ||
| // b. If additionalShift is false, then | ||
| let r1 = if !additional_shift { | ||
| // i. Let r1 be months. | ||
| months | ||
| } else { | ||
| // i. Let r1 be months + increment × sign. | ||
| months + increment_x_sign | ||
| }; | ||
| // c. Let r2 be r1 + increment × sign. | ||
| let r2 = r1 + increment_x_sign; | ||
| // d. Let startDuration be ? CreateNormalizedDurationRecord(duration.[[Years]], r1, 0, 0, ZeroTimeDuration()). | ||
| // e. Let endDuration be ? CreateNormalizedDurationRecord(duration.[[Years]], r2, 0, 0, ZeroTimeDuration()). | ||
| ( | ||
|
|
@@ -602,6 +625,14 @@ impl InternalDurationRecord { | |
| } | ||
| }; | ||
|
|
||
| // 5. Assert: If sign is 1, r1 ≥ 0 and r1 < r2. | ||
| // 6. Assert: If sign is -1, r1 ≤ 0 and r1 > r2. | ||
| // n.b. sign == 1 means nonnegative | ||
| crate::temporal_assert!( | ||
| (sign != Sign::Negative && r1 >= 0 && r1 < r2) | ||
| || (sign == Sign::Negative && r1 <= 0 && r1 > r2) | ||
| ); | ||
|
|
||
| let start_epoch_ns = if r1 == 0 { | ||
| origin_epoch_ns | ||
| } else { | ||
|
|
@@ -646,36 +677,132 @@ impl InternalDurationRecord { | |
| end.as_nanoseconds() | ||
| }; | ||
|
|
||
| // TODO: look into handling asserts | ||
| // 13. If sign is 1, then | ||
| // a. Assert: startEpochNs ≤ destEpochNs ≤ endEpochNs. | ||
| // 14. Else, | ||
| // a. Assert: endEpochNs ≤ destEpochNs ≤ startEpochNs. | ||
| // 15. Assert: startEpochNs ≠ endEpochNs. | ||
| Ok(NudgeWindow { | ||
| r1, | ||
| r2, | ||
| start_epoch_ns, | ||
| end_epoch_ns, | ||
| start_duration, | ||
| end_duration, | ||
| }) | ||
| } | ||
| // TODO: Add assertion into impl. | ||
| // TODO: Add unit tests specifically for nudge_calendar_unit if possible. | ||
| fn nudge_calendar_unit( | ||
| &self, | ||
| sign: Sign, | ||
| origin_epoch_ns: EpochNanoseconds, | ||
| dest_epoch_ns: i128, | ||
| dt: &PlainDateTime, | ||
| time_zone: Option<(&TimeZone, &(impl TimeZoneProvider + ?Sized))>, // ??? | ||
| options: ResolvedRoundingOptions, | ||
| ) -> TemporalResult<NudgeRecord> { | ||
| let dest_epoch_ns = EpochNanoseconds(dest_epoch_ns); | ||
|
|
||
| // 1. Let didExpandCalendarUnit be false. | ||
| let mut did_expand_calendar_unit = false; | ||
|
|
||
| // 2. Let nudgeWindow be ? ComputeNudgeWindow(sign, duration, originEpochNs, isoDateTime, timeZone, calendar, increment, unit, false). | ||
| let mut nudge_window = | ||
| self.compute_nudge_window(sign, origin_epoch_ns, dt, time_zone, options, false)?; | ||
|
|
||
| // 3. Let startEpochNs be nudgeWindow.[[StartEpochNs]]. | ||
| // 4. Let endEpochNs be nudgeWindow.[[EndEpochNs]]. | ||
| // (implicitly used) | ||
|
|
||
| // 5. If sign is 1, then | ||
| if sign != Sign::Negative { | ||
| // a. If startEpochNs ≤ destEpochNs ≤ endEpochNs is false, then | ||
| if !(nudge_window.start_epoch_ns <= dest_epoch_ns | ||
| && dest_epoch_ns <= nudge_window.end_epoch_ns) | ||
| { | ||
| // i. Set nudgeWindow to ? ComputeNudgeWindow(sign, duration, originEpochNs, isoDateTime, timeZone, calendar, increment, unit, true). | ||
| nudge_window = | ||
| self.compute_nudge_window(sign, origin_epoch_ns, dt, time_zone, options, true)?; | ||
| // ii. Assert: nudgeWindow.[[StartEpochNs]] ≤ destEpochNs ≤ nudgeWindow.[[EndEpochNs]]. | ||
| temporal_assert!( | ||
| nudge_window.start_epoch_ns <= dest_epoch_ns | ||
| && dest_epoch_ns <= nudge_window.end_epoch_ns | ||
| ); | ||
| // iii. Set didExpandCalendarUnit to true. | ||
| did_expand_calendar_unit = true; | ||
| } | ||
| } else { | ||
| // a. If endEpochNs ≤ destEpochNs ≤ startEpochNs is false, then | ||
| if !(nudge_window.end_epoch_ns <= dest_epoch_ns | ||
| && dest_epoch_ns <= nudge_window.start_epoch_ns) | ||
| { | ||
| // i. Set nudgeWindow to ? ComputeNudgeWindow(sign, duration, originEpochNs, isoDateTime, timeZone, calendar, increment, unit, true). | ||
| nudge_window = | ||
| self.compute_nudge_window(sign, origin_epoch_ns, dt, time_zone, options, true)?; | ||
| // ii. Assert: nudgeWindow.[[EndEpochNs]] ≤ destEpochNs ≤ nudgeWindow.[[StartEpochNs]]. | ||
| temporal_assert!( | ||
| nudge_window.end_epoch_ns <= dest_epoch_ns | ||
| && dest_epoch_ns <= nudge_window.start_epoch_ns | ||
| ); | ||
| // iii. Set didExpandCalendarUnit to true. | ||
| did_expand_calendar_unit = true; | ||
| } | ||
| } | ||
|
|
||
| // 7. Let r1 be nudgeWindow.[[R1]]. | ||
| // 8. Let r2 be nudgeWindow.[[R2]]. | ||
| // 9. Set startEpochNs to nudgeWindow.[[StartEpochNs]]. | ||
| // 10. Set endEpochNs to nudgeWindow.[[StartEpochNs]]. | ||
| // 11. Let startDuration be nudgeWindow.[[StartDuration]]. | ||
| // 12. Let endDuration be nudgeWindow.[[EndDuration]]. | ||
|
|
||
| let NudgeWindow { | ||
| r1, | ||
| r2, | ||
| start_epoch_ns, | ||
| end_epoch_ns, | ||
| start_duration, | ||
| end_duration, | ||
| } = nudge_window; | ||
|
|
||
| // 13. Assert: startEpochNs ≠ endEpochNs. | ||
| temporal_assert!(start_epoch_ns != end_epoch_ns); | ||
|
|
||
| // TODO: Don't use f64 below ... | ||
| // NOTE(nekevss): Step 12..13 could be problematic...need tests | ||
| // and verify, or completely change the approach involved. | ||
| // TODO(nekevss): Validate that the `f64` casts here are valid in all scenarios | ||
| // 16. Let progress be (destEpochNs - startEpochNs) / (endEpochNs - startEpochNs). | ||
| // 17. Let total be r1 + progress × increment × sign. | ||
| let progress = | ||
| (dest_epoch_ns - start_epoch_ns.0) as f64 / (end_epoch_ns.0 - start_epoch_ns.0) as f64; | ||
| // 14. Let progress be (destEpochNs - startEpochNs) / (endEpochNs - startEpochNs). | ||
| // 15. Let total be r1 + progress × increment × sign. | ||
| let progress = (dest_epoch_ns.0 - start_epoch_ns.0) as f64 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: this actually may be a candidate for using the |
||
| / (end_epoch_ns.0 - start_epoch_ns.0) as f64; | ||
| let total = r1 as f64 | ||
| + progress * options.increment.get() as f64 * f64::from(sign.as_sign_multiplier()); | ||
|
|
||
| // 14. NOTE: The above two steps cannot be implemented directly using floating-point arithmetic. | ||
| // 16. NOTE: The above two steps cannot be implemented directly using floating-point arithmetic. | ||
| // This division can be implemented as if constructing Normalized Time Duration Records for the denominator | ||
| // and numerator of total and performing one division operation with a floating-point result. | ||
| // 15. Let roundedUnit be ApplyUnsignedRoundingMode(total, r1, r2, unsignedRoundingMode). | ||
| let rounded_unit = | ||
| IncrementRounder::from_signed_num(total, options.increment.as_extended_increment())? | ||
| .round(options.rounding_mode); | ||
|
|
||
| // 16. If roundedUnit - total < 0, let roundedSign be -1; else let roundedSign be 1. | ||
| // 19. Return Duration Nudge Result Record { [[Duration]]: resultDuration, [[Total]]: total, [[NudgedEpochNs]]: nudgedEpochNs, [[DidExpandCalendarUnit]]: didExpandCalendarUnit }. | ||
| // 17. If roundedSign = sign, then | ||
| if rounded_unit == r2 { | ||
| // 17. Assert: 0 ≤ progress ≤ 1. | ||
| temporal_assert!((0. ..=1.).contains(&progress)); | ||
| // 18. If sign < 0, let isNegative be negative; else let isNegative be positive. | ||
| // (used implicitly) | ||
|
|
||
| // 19. Let unsignedRoundingMode be GetUnsignedRoundingMode(roundingMode, isNegative). | ||
| // n.b. get_unsigned_round_mode takes is_positive, but it actually cares about nonnegative | ||
| let unsigned_rounding_mode = options | ||
| .rounding_mode | ||
| .get_unsigned_round_mode(sign != Sign::Negative); | ||
|
|
||
| // 20. If progress = 1, then | ||
| let rounded_unit = if progress == 1. { | ||
| // a. Let roundedUnit be abs(r2). | ||
| r2.abs() | ||
| } else { | ||
| // a. Assert: abs(r1) ≤ abs(total) < abs(r2). | ||
| temporal_assert!(r1.abs() as f64 <= total.abs() && total.abs() < r2.abs() as f64); | ||
| // b. Let roundedUnit be ApplyUnsignedRoundingMode(abs(total), abs(r1), abs(r2), unsignedRoundingMode). | ||
| // TODO: what happens to r2 here? | ||
| unsigned_rounding_mode.apply(total.abs(), r1.abs(), r2.abs()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, huh. Yeah, it'd probably be worth taking a second pass at rounding given some of the changes to the specification. I'd really prefer to stay away from f64 if possible, but it might not be with all the changes that have been made to the spec since this was first written. |
||
| }; | ||
|
|
||
| // 22. If roundedUnit is abs(r2), then | ||
| if rounded_unit == r2.abs() { | ||
| // a. Let didExpandCalendarUnit be true. | ||
| // b. Let resultDuration be endDuration. | ||
| // c. Let nudgedEpochNs be endEpochNs. | ||
|
|
@@ -687,14 +814,13 @@ impl InternalDurationRecord { | |
| }) | ||
| // 18. Else, | ||
| } else { | ||
| // a. Let didExpandCalendarUnit be false. | ||
| // b. Let resultDuration be startDuration. | ||
| // c. Let nudgedEpochNs be startEpochNs. | ||
| Ok(NudgeRecord { | ||
| normalized: InternalDurationRecord::new(start_duration, TimeDuration::default())?, | ||
| total: Some(FiniteF64::try_from(total)?), | ||
| nudge_epoch_ns: start_epoch_ns.0, | ||
| expanded: false, | ||
| expanded: did_expand_calendar_unit, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -847,6 +847,47 @@ impl RoundingMode { | |
| } | ||
| } | ||
|
|
||
| impl UnsignedRoundingMode { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: It would be nice to have some unit tests for this. |
||
| /// <https://tc39.es/proposal-temporal/#sec-applyunsignedroundingmode> | ||
| pub(crate) fn apply(self, x: f64, r1: i128, r2: i128) -> i128 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: I think at the point that we are passing x as a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't, because r1 and r2 are the actual returned values, and that would potentially change the produced mathematical value. On the other hand, the only thing that matters about x is which side of r1 + r2 / 2 it is. That's easier to get right. We should probably use a Fraction type.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah shoot, true.
Yeah, that's what IncrementRounder was relying on. |
||
| // 1. If x = r1, return r1. | ||
| if r1 as f64 == x { | ||
| return r1; | ||
| } | ||
| // 4. If unsignedRoundingMode is zero, return r1. | ||
| if self == UnsignedRoundingMode::Zero { | ||
| return r1; | ||
| } else if self == UnsignedRoundingMode::Infinity { | ||
| return r2; | ||
| } | ||
| // 6. Let d1 be x – r1. | ||
| // 7. Let d2 be r2 – x. | ||
| let d1 = x - r1 as f64; | ||
| let d2 = r2 as f64 - x; | ||
| if d1 < d2 { | ||
| return r1; | ||
| } else if d1 > d2 { | ||
| return r2; | ||
| } | ||
| match self { | ||
| UnsignedRoundingMode::HalfZero => r1, | ||
| UnsignedRoundingMode::HalfInfinity => r2, | ||
| // HalfEven | ||
| _ => { | ||
| // 14. Let cardinality be (r1 / (r2 – r1)) modulo 2. | ||
| let diff = r2 - r1; | ||
| let cardinality = (r1 as f64 / diff as f64) % 2.; | ||
| // 15. If cardinality = 0, return r1. | ||
| if cardinality == 0. { | ||
| r1 | ||
| } else { | ||
| r2 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl FromStr for RoundingMode { | ||
| type Err = TemporalError; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, spec bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tc39/proposal-temporal#3191 - should have been caught in review, but anba caught it a bit later