Skip to content
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

Receiver should be validated/processed before arguments #2462

Closed
gibson042 opened this issue Jan 6, 2023 · 2 comments · Fixed by #2478
Closed

Receiver should be validated/processed before arguments #2462

gibson042 opened this issue Jan 6, 2023 · 2 comments · Fixed by #2478
Assignees
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@gibson042
Copy link
Collaborator

Related to but distinct from #2289, observable interactions with arguments should take place in order (cf. #2377 (comment) ) after validation and processing of the receiver. This is violated by at least the with methods, which invoke PrepareTemporalFields on the first argument before the receiver (observable in code that e.g. defines getters on the receiver like (i => Object.defineProperties(Temporal.PlainYearMonth.from("2022-12"), { year: { get:()=>i++ } }).with({ get month(){ return i++; } }))(1), which currently returns 0002-01 rather than 0001-02).

For with methods in particular, I believe the general corrected steps would be

 1. Let _receiver_ be the *this* value.
 1. Perform ? RequireInternalSlot(_receiver_, [[InitializedTemporal<Type>]]).
 1. If Type(_temporalInstanceLike_) is not Object, throw a *TypeError* exception.
 1. Perform ? RejectObjectWithCalendarOrTimeZone(_temporalInstanceLike_).
+1. Set _options_ to ? GetOptionsObject(_options_).
 1. Let _calendar_ be _receiver_.[[Calendar]].
 1. Let _fieldNames_ be ? CalendarFields(_calendar_, « … »).
-1. Let _overrides_ be ? PrepareTemporalFields(_temporalInstanceLike_, _fieldNames_, ~partial~).
-1. Set _options_ to ? GetOptionsObject(_options_).
 1. Let _fields_ be ? PrepareTemporalFields(_receiver_, _fieldNames_, «»).
+1. Let _overrides_ be ? PrepareTemporalFields(_temporalInstanceLike_, _fieldNames_, ~partial~).
 1. Set _fields_ to ? CalendarMergeFields(_calendar_, _fields_, _overrides_).
 1. Set _fields_ to ? PrepareTemporalFields(_fields_, _fieldNames_, «»).
 1. Return ? Calendar<Type>FromFields(_calendar_, _fields_, _options_).
@ptomato ptomato added spec-text Specification text involved normative Would be a normative change to the proposal labels Jan 6, 2023
@ptomato ptomato added this to the Stage "3.5" milestone Jan 6, 2023
@ptomato ptomato self-assigned this Jan 16, 2023
@ptomato
Copy link
Collaborator

ptomato commented Jan 16, 2023

It could be debatable which order of operations best expresses the design principle, but ultimately I agree with the one you suggested above.

@ptomato
Copy link
Collaborator

ptomato commented Jan 16, 2023

I audited all of the methods with at least one argument. Aside from with, I found that PlainYearMonth's until and since methods also did PrepareTemporalFields on the argument before the receiver.

I also found that in PlainYearMonth's add and subtract it's possible we'd want to move steps 5–7 of AddDurationToOrSubtractDurationFromPlainYearMonth to the beginning. However, you could also interpret the existing steps as follows: first the receiver is validated in the caller (add or subtract), then temporalDurationLike is validated in steps 1–3, options is validated in step 4, and the actual calculation starts in step 5. That's more consistent with Richard's suggestion above, where the actual calculation starts with the call to CalendarFields. So I'm highlighting it as something to think about, but I don't think it needs to be changed.

ptomato added a commit that referenced this issue Jan 16, 2023
Changes to the order of observable operations in
- PlainDate.p.with
- PlainDateTime.p.with
- PlainMonthDay.p.with
- PlainTime.p.with
- PlainYearMonth.p.since
- PlainYearMonth.p.until
- PlainYearMonth.p.with
- ZonedDateTime.p.with

Swapping the PrepareTemporalFields calls in ZonedDateTime.p.with exposed
another unnecessary user call: the 'timeZone' field was observably read
from the receiver and from the object returned from mergeFields(), but
never used. This is fixed as well.

Closes: #2462
ptomato added a commit that referenced this issue Jan 20, 2023
Changes to the order of observable operations in
- PlainDate.p.with
- PlainDateTime.p.with
- PlainMonthDay.p.with
- PlainTime.p.with
- PlainYearMonth.p.since
- PlainYearMonth.p.until
- PlainYearMonth.p.with
- ZonedDateTime.p.with

Swapping the PrepareTemporalFields calls in ZonedDateTime.p.with exposed
another unnecessary user call: the 'timeZone' field was observably read
from the receiver and from the object returned from mergeFields(), but
never used. This is fixed as well.

Closes: #2462
ptomato added a commit that referenced this issue Feb 1, 2023
Changes to the order of observable operations in
- PlainDate.p.with
- PlainDateTime.p.with
- PlainMonthDay.p.with
- PlainTime.p.with
- PlainYearMonth.p.since
- PlainYearMonth.p.until
- PlainYearMonth.p.with
- ZonedDateTime.p.with

Swapping the PrepareTemporalFields calls in ZonedDateTime.p.with exposed
another unnecessary user call: the 'timeZone' field was observably read
from the receiver and from the object returned from mergeFields(), but
never used. This is fixed as well.

Closes: #2462
ptomato added a commit that referenced this issue Feb 1, 2023
Changes to the order of observable operations in
- PlainDate.p.with
- PlainDateTime.p.with
- PlainMonthDay.p.with
- PlainTime.p.with
- PlainYearMonth.p.since
- PlainYearMonth.p.until
- PlainYearMonth.p.with
- ZonedDateTime.p.with

Swapping the PrepareTemporalFields calls in ZonedDateTime.p.with exposed
another unnecessary user call: the 'timeZone' field was observably read
from the receiver and from the object returned from mergeFields(), but
never used. This is fixed as well.

Closes: #2462
Aditi-1400 pushed a commit to Aditi-1400/proposal-temporal that referenced this issue Mar 7, 2023
Changes to the order of observable operations in
- PlainDate.p.with
- PlainDateTime.p.with
- PlainMonthDay.p.with
- PlainTime.p.with
- PlainYearMonth.p.since
- PlainYearMonth.p.until
- PlainYearMonth.p.with
- ZonedDateTime.p.with

Swapping the PrepareTemporalFields calls in ZonedDateTime.p.with exposed
another unnecessary user call: the 'timeZone' field was observably read
from the receiver and from the object returned from mergeFields(), but
never used. This is fixed as well.

Closes: tc39#2462
justingrant pushed a commit that referenced this issue Apr 20, 2023
Changes to the order of observable operations in
- PlainDate.p.with
- PlainDateTime.p.with
- PlainMonthDay.p.with
- PlainTime.p.with
- PlainYearMonth.p.since
- PlainYearMonth.p.until
- PlainYearMonth.p.with
- ZonedDateTime.p.with

Swapping the PrepareTemporalFields calls in ZonedDateTime.p.with exposed
another unnecessary user call: the 'timeZone' field was observably read
from the receiver and from the object returned from mergeFields(), but
never used. This is fixed as well.

Closes: #2462

UPSTREAM_COMMIT=c9939fd078676c3fda40d06f3b8b5067c35dc124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants