WIP solution to #3310, extra nudge#3318
Conversation
…itionalShift undo more scenarios that would error prior
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3318 +/- ##
==========================================
- Coverage 98.39% 98.37% -0.02%
==========================================
Files 22 22
Lines 10763 10774 +11
Branches 1866 1869 +3
==========================================
+ Hits 10590 10599 +9
- Misses 161 163 +2
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ptomato
left a comment
There was a problem hiding this comment.
Thanks very much for taking a look at this Adam!
I've been working on this myself and I've come to some similar conclusions: we definitely need to handle additionalShift in the days and weeks cases, and like you I suspect we don't need the extra case when sign === 1.
I do find the -2 result kind of suspicious though. At least, it seems weird to start with a duration of exactly -1 day, round it to the nearest day, and get -2 days. That doesn't happen with normal DST changes, e.g.
relativeTo = Temporal.ZonedDateTime.from('2026-03-08T03:00-07:00[America/Vancouver]');
duration = Temporal.Duration.from({ hours: -1 })
duration.round({ smallestUnit: 'hours', relativeTo })
// -1 heven though the wall clock difference is 2 hours
relativeTo.toPlainDateTime().until(relativeTo.add({ hours: -1 }).toPlainDateTime())
// -2 hBut then again, hours are time units and we do explicitly treat those differently than calendar units in ZonedDateTime arithmetic. So maybe it's justified. I don't know.
I've been using a bot to search for solutions that avoid the assertions while still getting a -1 result. It found one, at least one that passes all the tests I've been able to throw at it, but I've been hesitant to post it since I don't yet feel confident about whether it's correct. It adds a new if-clause inside the sign === -1 case and before the endEpochNs ≤ destEpochNs ≤ startEpochNs test, and if startEpochNs = endEpochNs it will basically compute an extended endEpochNs for the denominator, but not overwrite the actual nudge window's startEpochNs and endEpochNs. Seems vaguely iffy to me, like maybe the bot is overfitting to the test. I guess the part I don't understand is why rounding modes and total don't get messed up when we increase the size of the denominator.
In any case, I just updated the test262 PR (tc39/test262#5044) with all of the failing cases I've found over the past few days. Some of these I found by taking a solution that the bot claimed was correct and running it through the snapshot tests, to discover that there were still cases that would fail the assertions. But do note that I wrote the tests assuming the -1 result is correct and not the -2 result.
|
Remind me what the original intent of the "nudge window" is? I often find that it's easier to resolve problems like this by going back to the first principles that caused us to create concepts and algorithms in the first place, and then to see if our new idea matches the original intent. |
|
Adam might have a better answer to this, but as I understand it the nudge window should be a pair of values bracketing the value to be rounded, and they should be exactly one rounding increment apart. Depending on the rounding mode, we round either to the left edge or the right edge of the nudge window. It gets simpler to visualize if we pretend to be rounding plain numbers and not durations: for example the number to be rounded is 10.5, to the nearest increment of 3, the nudge window would be from 9 to 12. |
I was thinking same thing as @justingrant #3310 (comment):
For relative-duration math, we have the notion of a "nudge window" that contains the epoch-nanoseconds of the "destination" (relativeTo+duration). I visualize infinite windows emanating from relativeTo, as
relativeTo + duration * nand the goal is to find the one that bounds the destination.With the the test case from #3310 in mind, I prefer to first think of a case where the duration has some extra time units that push it within the nudge window rather that residing exactly on the start:
Now for the clean
day: -1case:Conclusion
This -2 funny business seems "correct" according to the original algorithm, but feels rather unintuitive on its face.
Maybe the -2 feels weird but is more consistent with other operations, so overall better. Need to play around with this more.