Eliminate duplication between Duration Records and Temporal.Duration instances #2943

ptomato · 2024-09-20T00:53:38Z

A series of refactors that does three things:

Change operations that took a long series of Duration components as arguments (i.e., years, months, weeks, etc.) to take a Record or a Temporal.Duration instance instead. (The exception is IsValidDuration.)
Make duration operations all explicitly use the "normalize - calculate - unnormalize" pattern, where we convert a Temporal.Duration instance into a Normalized Duration Record, do the calculations with the normalized form since that's how implementations calculate internally, and then convert back to a Temporal.Duration instance to expose to user code. This is a pattern that we already mostly used, but now that everything is refactored to take Records instead of individual Duration components, it can be expressed much more explicitly and concisely.
Get rid of Duration Records and Time Duration Records, instead using either Temporal.Duration instances or the normalized form.

Closes: #2308

Also introduce operations to take the sign of a Date Duration Record and of a Normalized Duration Record, which replace some uses of DurationSign where there wasn't a Duration instance handy to pass to it. See: #2308

…tion Record See: #2308

…te Duration Record See: #2308

Instead of picking a _sign_ of -1 or 1 and multiplying the components by it, create the Duration instance and then pass it through CreateNegatedTemporalDuration if the operation is ~since~. See: #2308

In the reference code, move AddISODate into calendar.mjs because it's only used once there (and replace the other calls of it with BalanceISODate.) This is a small step towards untangling the circular import in calendar.mjs. See: #2308

Similar to DifferenceTemporal___, instead of picking a _sign_ of -1 or 1 and multiplying the duration components by it, create the Duration instance and then pass it through CreateNegatedTemporalDuration if the operation is ~subtract~. This was already started in #2290, and brought into consistency everywhere in this commit. See: #2308

See: #2308

In order to avoid passing long lists of components, refactor Normalized Duration Records to have only two fields: a Date Duration Record and a Normalized Time Duration Record. This requires several changes throughout.

…normalize This is a pattern that we already mostly use, but now that everything is refactored to take Records instead of individual Duration components, it can be expressed much more concisely. See: #2308

…calc-unnormalize See: #2308

See: #2308

…rmalize See: #2308

…normalize See: #2308

…c-unnormalize This allows inlining AddDateTime into AddDurationTo...PlainDateTime. It's the only place AddDateTime was used. See: #2308

See: #2308

…lculate See: #2308

…culate See: #2308

…unnormalize See: #2308

…ulate-unnormalize See: #2308

In a few places, we did the operation of balancing the normalized time duration up to days, adding the days to the [[Days]] field of the date duration record, and discarding the time duration. Encapsulate this in its own AO. See: #2308

BalanceTimeDuration is now only used in UnnormalizeDuration. Inline it there. There is no longer any usage of Time Duration Records, so remove them. See: #2308

In a few places, we want to create a new Date Duration Record but with one or more fields adjusted, as if Date Duration Records had a with() method. This is easy to do with spreading and destructuring in JS, but is unwieldy in spec language. Add an AdjustDateDurationRecord abstract operation to do this more concisely. See: #2308

These are no longer necessary. We should either be using Normalized Duration Records (or their subcomponents, Date Duration Records and Normalized Time Duration Records) or Temporal.Duration instances. The records were still used in a few places: ToTemporalDurationRecord can be inlined into ToTemporalDuration since that is its only caller, and ParseTemporalDurationString can just return a Temporal.Duration instance directly. We keep ToTemporalDurationRecordRaw for the validStrings.js test.

gibson042 · 2024-09-20T14:48:52Z

spec/duration.html

-          1. Let _dateDuration_ be CreateDurationRecord(_duration_.[[Years]], _duration_.[[Months]], _duration_.[[Weeks]], _duration_.[[Days]] + _targetTime_.[[Days]], 0, 0, 0, 0, 0, 0).
+          1. Let _dateDuration_ be ? AdjustDateDurationRecord(_normalizedDuration_.[[Date]], _targetTime_.[[Days]]).
          1. Let _targetDate_ be ? CalendarDateAdd(_calendar_, _isoRelativeToDate_, _dateDuration_, *"constrain"*).


Does this new ? constitute a normative change? A surface reading suggests that an exception is now possible before CalendarDateAdd that was not possible before.

Oh, this should be ! (and the same for your other comment below.) I'll fix this in a follow up PR. (Proof: targetTime.[[Days]] cannot exceed the original value of duration.[[Days]] because its absolute value can only be equal or smaller, coming out of AddTime.)

gibson042 · 2024-09-20T14:50:47Z

spec/duration.html

-          1. Let _dateDuration_ be CreateDurationRecord(_duration_.[[Years]], _duration_.[[Months]], _duration_.[[Weeks]], _duration_.[[Days]] + _targetTime_.[[Days]], 0, 0, 0, 0, 0, 0).
+          1. Let _dateDuration_ be ? AdjustDateDurationRecord(_normalizedDuration_.[[Date]], _targetTime_.[[Days]]).
          1. Let _targetDate_ be ? CalendarDateAdd(_calendar_, _isoRelativeToDate_, _dateDuration_, *"constrain"*).


Same question here.

This was a mistake in #2943. _targetTime_.[[Days]] cannot exceed the original value of _duration_.[[Days]] here because its absolute value can only be equal or smaller coming out of AddTime.

This mistakenly became an assertion in PR #2943. By inspection it is easy to come up with a case where CreateTemporalDuration is fallible, and this is well covered in test262. Closes: #2992

<tc39/proposal-temporal#2943> should address the fixme note. Differential Revision: https://phabricator.services.mozilla.com/D223554

Here are a couple of stragglers from PR #2943.

Another couple of leftovers from PR #2943.

Here are a couple of stragglers from PR #2943.

Another couple of leftovers from PR #2943.

<tc39/proposal-temporal#2943> should address the fixme note. Differential Revision: https://phabricator.services.mozilla.com/D223554 UltraBlame original commit: de7fc134aa463dddcc4f75ff9ef2ba2b9b670a6f

<tc39/proposal-temporal#2943> should address the fixme note. Differential Revision: https://phabricator.services.mozilla.com/D223554

ptomato requested a review from gibson042 September 20, 2024 00:53

Ms2ger approved these changes Sep 20, 2024

View reviewed changes

ptomato added 25 commits September 20, 2024 11:48

Editorial: Refactor TemporalDurationToString to take a Duration instance

730e676

See: #2308

Editorial: Refactor DurationSign to take a Duration instance

a7ddf73

Also introduce operations to take the sign of a Date Duration Record and of a Normalized Duration Record, which replace some uses of DurationSign where there wasn't a Duration instance handy to pass to it. See: #2308

Editorial: Refactor UnbalanceDateDurationRelative to take a Date Dura…

04930dc

…tion Record See: #2308

Editorial: Refactor CombineDateAndNormalizedTimeDuration to take a Da…

a7c6db7

…te Duration Record See: #2308

Editorial: Refactor DifferenceTemporal___ methods to use CreateNegated

21508f5

Instead of picking a _sign_ of -1 or 1 and multiplying the components by it, create the Duration instance and then pass it through CreateNegatedTemporalDuration if the operation is ~since~. See: #2308

Editorial: Refactor AddDateTime to take a Date Duration Record

f05bf31

See: #2308

Editorial: Refactor Normalized Duration Record to have two fields

cb6b3b4

In order to avoid passing long lists of components, refactor Normalized Duration Records to have only two fields: a Date Duration Record and a Normalized Time Duration Record. This requires several changes throughout.

Editorial: Refactor Duration.p.add/subtract to normalize-calculate-un…

159ef2e

…normalize This is a pattern that we already mostly use, but now that everything is refactored to take Records instead of individual Duration components, it can be expressed much more concisely. See: #2308

Editorial: Refactor Duration.p.round/total/toString to use normalize-…

2b64158

…calc-unnormalize See: #2308

Editorial: Refactor Duration.compare to use normalize-calc-unnormalize

a85f9af

See: #2308

Editorial: Refactor Instant.p.add/subtract to use normalize-calc-unno…

3a5b8d2

…rmalize See: #2308

Editorial: Refactor PlainDate.p.add/subtract to use normalize-calc-un…

9d55d01

…normalize See: #2308

Editorial: Refactor PlainDateTime.p.add/subtract to use normalize-cal…

403234f

…c-unnormalize This allows inlining AddDateTime into AddDurationTo...PlainDateTime. It's the only place AddDateTime was used. See: #2308

Editorial: Refactor PlainTime.p.add/subtract to use normalize-calculate

6f37e7f

See: #2308

Editorial: Refactor PlainYearMonth.p.add/subtract to use normalize-ca…

2b4a8c8

…lculate See: #2308

Editorial: Refactor ZonedDateTime.p.add/subtract to use normalize-cal…

2613c40

…culate See: #2308

Editorial: Refactor Instant.p.since/until to use normalize-calculate-…

0065885

…unnormalize See: #2308

Editorial: Refactor ZonedDateTime.p.since/until to use normalize-calc…

6588998

…ulate-unnormalize See: #2308

Editorial: Add NormalizeDurationWithoutTime AO

e3a04d9

In a few places, we did the operation of balancing the normalized time duration up to days, adding the days to the [[Days]] field of the date duration record, and discarding the time duration. Encapsulate this in its own AO. See: #2308

Editorial: Remove BalanceTimeDuration AO and Time Duration Record

9ab7989

BalanceTimeDuration is now only used in UnnormalizeDuration. Inline it there. There is no longer any usage of Time Duration Records, so remove them. See: #2308

Editorial: Add use of ZeroDateDuration

2ade615

Editorial: Fix return type of DifferenceISODateTime

3333f8e

Ms2ger force-pushed the 2308-duration-records branch from f5eab0a to df118ba Compare September 20, 2024 09:55

Ms2ger force-pushed the 2308-duration-records branch from df118ba to 4cbf8c0 Compare September 20, 2024 10:02

Ms2ger merged commit 748f27f into main Sep 20, 2024
6 checks passed

Ms2ger deleted the 2308-duration-records branch September 20, 2024 10:54

gibson042 reviewed Sep 20, 2024

View reviewed changes

ptomato mentioned this pull request Oct 8, 2024

Consider renaming NormalizeDuration and UnnormalizeDuration #2953

Closed

ptomato added a commit that referenced this pull request Oct 8, 2024

Editorial: Fix AOs that no longer return a [[NormalizedDuration]] field

91ec749

Here are a couple of stragglers from PR #2943.

ptomato added a commit that referenced this pull request Oct 8, 2024

Editorial: Fix duration record field names

33228e7

Another couple of leftovers from PR #2943.

ptomato added a commit that referenced this pull request Oct 9, 2024

Editorial: Fix AOs that no longer return a [[NormalizedDuration]] field

56303c9

Here are a couple of stragglers from PR #2943.

ptomato added a commit that referenced this pull request Oct 9, 2024

Editorial: Fix duration record field names

6346638

Another couple of leftovers from PR #2943.

ptomato mentioned this pull request Oct 15, 2024

Temporal.Duration.prototype.toString: If-condition should use "and" instead of "or" #3009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate duplication between Duration Records and Temporal.Duration instances #2943

Eliminate duplication between Duration Records and Temporal.Duration instances #2943

ptomato commented Sep 20, 2024

gibson042 Sep 20, 2024

ptomato Sep 20, 2024

gibson042 Sep 20, 2024

Eliminate duplication between Duration Records and Temporal.Duration instances #2943

Eliminate duplication between Duration Records and Temporal.Duration instances #2943

Conversation

ptomato commented Sep 20, 2024

gibson042 Sep 20, 2024

Choose a reason for hiding this comment

ptomato Sep 20, 2024

Choose a reason for hiding this comment

gibson042 Sep 20, 2024

Choose a reason for hiding this comment