Normative: Validate receiver fields before fields of any param objects #2478

ptomato · 2023-01-16T23:36:04Z

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 · 2023-01-16T23:38:58Z

spec/plaintime.html

-        1. Let _partialTime_ be ? ToTemporalTimeRecord(_temporalTimeLike_, ~partial~).
        1. Set _options_ to ? GetOptionsObject(_options_).
        1. Let _overflow_ be ? ToTemporalOverflow(_options_).
+        1. Let _partialTime_ be ? ToTemporalTimeRecord(_temporalTimeLike_, ~partial~).


It's debatable whether we'd want to make this change to PlainTime, which doesn't use PrepareTemporalFields, but it seems most consistent with the form suggested by Richard in #2462 (comment)

👍
But if this is awkward and options never affects the interpretation of the first argument, then it would also be coherent to move every Set _options_ to ? GetOptionsObject(_options_). to follow full consumption (with constituent validation) of the first argument by PrepareTemporalFields/ToTemporalTimeRecord/etc.—although that is not my preference because it's not clear to me in that case whether GetOptionsObject should precede or follow integration of receiver and argument data in e.g. CalendarMergeFields.

codecov · 2023-01-16T23:40:02Z

Codecov Report

Merging #2478 (f35fc58) into main (189e689) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head f35fc58 differs from pull request most recent head f25466a. Consider uploading reports for the commit f25466a to get more accurate results

@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
- Coverage   94.97%   94.95%   -0.03%     
==========================================
  Files          20       20              
  Lines       10907    10817      -90     
  Branches     1996     1957      -39     
==========================================
- Hits        10359    10271      -88     
  Misses        487      487              
+ Partials       61       59       -2

Impacted Files	Coverage Δ
polyfill/lib/ecmascript.mjs	`98.29% <100.00%> (-0.01%)`	⬇️
polyfill/lib/plaindate.mjs	`99.66% <100.00%> (-0.01%)`	⬇️
polyfill/lib/plaindatetime.mjs	`97.23% <100.00%> (ø)`
polyfill/lib/plainmonthday.mjs	`97.67% <100.00%> (ø)`
polyfill/lib/plaintime.mjs	`98.26% <100.00%> (ø)`
polyfill/lib/plainyearmonth.mjs	`98.36% <100.00%> (-0.01%)`	⬇️
polyfill/lib/zoneddatetime.mjs	`99.82% <100.00%> (-0.01%)`	⬇️
polyfill/runtest262.mjs	`100.00% <0.00%> (+10.00%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

The with() methods, as well as PlainYearMonth's since() and until(), were adjusted to read the receiver's fields before the fields of any objects provided as arguments. This change is observable, so affects several tests that test the observed order of operations. Normative PR: tc39/proposal-temporal#2478

ptomato · 2023-01-16T23:43:06Z

Test262 updates are in tc39/test262#3769

polyfill/lib/plaindate.mjs

polyfill/lib/plaindatetime.mjs

polyfill/lib/plainmonthday.mjs

gibson042 · 2023-01-17T17:40:56Z

spec/plaintime.html

-        1. Let _partialTime_ be ? ToTemporalTimeRecord(_temporalTimeLike_, ~partial~).
        1. Set _options_ to ? GetOptionsObject(_options_).
        1. Let _overflow_ be ? ToTemporalOverflow(_options_).
+        1. Let _partialTime_ be ? ToTemporalTimeRecord(_temporalTimeLike_, ~partial~).


👍
But if this is awkward and options never affects the interpretation of the first argument, then it would also be coherent to move every Set _options_ to ? GetOptionsObject(_options_). to follow full consumption (with constituent validation) of the first argument by PrepareTemporalFields/ToTemporalTimeRecord/etc.—although that is not my preference because it's not clear to me in that case whether GetOptionsObject should precede or follow integration of receiver and argument data in e.g. CalendarMergeFields.

polyfill/lib/plainyearmonth.mjs

gibson042 · 2023-01-17T18:01:58Z

spec/plainyearmonth.html

        1. Let _thisFields_ be ? PrepareTemporalFields(_yearMonth_, _fieldNames_, «»).
        1. Perform ! CreateDataPropertyOrThrow(_thisFields_, *"day"*, *1*<sub>𝔽</sub>).
        1. Let _thisDate_ be ? CalendarDateFromFields(_calendar_, _thisFields_).
+        1. Let _otherFields_ be ? PrepareTemporalFields(_other_, _fieldNames_, «»).
+        1. Perform ! CreateDataPropertyOrThrow(_otherFields_, *"day"*, *1*<sub>𝔽</sub>).
+        1. Let _otherDate_ be ? CalendarDateFromFields(_calendar_, _otherFields_).
        1. Let _untilOptions_ be OrdinaryObjectCreate(*null*).
        1. Perform ? CopyDataProperties(_untilOptions_, _settings_.[[Options]], « »).


It is curious that this step can throw; is that a bug in failure to properly consume options earlier on?

I don't think it's a bug - we have to perform CopyDataProperties on options at some point, so that we can pass an options object to CalendarDateUntil with a modified largestUnit property. options is a user-provided object so it may have accessor properties.

Right, I'm saying that the bug is consuming it so deep in the spec algorithms rather than in initial processing—and in particular in performing CopyDataProperties after GetDifferenceSettings has read several properties on its own.

I could see this going either way - it'd be good to eliminate the duplicate reads, but on the other hand we'd introduce an inconsistency where we'd copy the options object in the since/until methods of PlainDate, PlainDateTime, PlainYearMonth, and ZonedDateTime; and not copy it in the since/until methods of PlainTime and Instant.

Wouldn't options be copied in all since/until methods?

Only the ones where a modified options object needs to be passed on to calendar.dateUntil(). Otherwise, we just read only the needed properties in alphabetical order.

Well, either that serves as explanation of the inconsistency or we'd copy options in all since/until methods even though some technically don't need it.

Do you have a concrete proposal for this? If we can manage to write that additional change before TC39's review deadline, I wouldn't be opposed to including it here. Otherwise I think it can be done as part of #2289 or left as is.

Concretely, I'm proposing 1. Perform ? CopyDataProperties(_optionsCopy_, _options_, « »). in every DifferenceTemporal<Type> operation immediately after validating other and use of optionsCopy rather than options in every subsequent step. But I'm also comfortable with a compromise in which that change is omitted in DifferenceTemporalInstant and DifferenceTemporalPlainTime, ideally replaced with a NOTE step explaining its omission.

I agree that it is logically part of the fixes for #2289, but will still try to put up a narrow PR since it has been specifically identified (and in fact already has a comment: #2289 (comment) ).

polyfill/lib/zoneddatetime.mjs

spec/zoneddatetime.html

ptomato · 2023-01-17T19:19:20Z

Draft until presented at the next TC39 plenary meeting.

In ZonedDateTime with(), the 'timeZone' field was observably read from the receiver and from the object returned from mergeFields(), but never used. This is not only an unnecessary user call, it becomes a bug when we remove the ZonedDateTime.p.timeZone property in order to replace it with .timeZoneId and .getTimeZone(). Also part of #2478

The with() methods, as well as PlainYearMonth's since() and until(), were adjusted to read the receiver's fields before the fields of any objects provided as arguments. This change is observable, so affects several tests that test the observed order of operations. Normative PR: tc39/proposal-temporal#2478

In ZonedDateTime with(), the 'timeZone' field was observably read from the receiver and from the object returned from mergeFields(), but never used. This is an unnecessary user call.

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 · 2023-02-01T18:44:55Z

This change achieved consensus at the 2023-01-31 TC39 plenary meeting. Test262 PR has been merged already.

ptomato requested review from Ms2ger, gibson042 and Aditi-1400 January 16, 2023 23:36

ptomato commented Jan 16, 2023

View reviewed changes

ptomato mentioned this pull request Jan 16, 2023

Temporal: Adjustments to order-of-operations tests tc39/test262#3769

Merged

gibson042 approved these changes Jan 17, 2023

View reviewed changes

ptomato marked this pull request as draft January 17, 2023 19:19

gibson042 approved these changes Jan 18, 2023

View reviewed changes

ptomato force-pushed the 2462-process-receiver-first branch from ad0283a to f35fc58 Compare January 20, 2023 02:10

ptomato added 2 commits February 1, 2023 10:36

Normative: Don't read 'timeZone' property in ZonedDateTime.p.with

354ce1f

In ZonedDateTime with(), the 'timeZone' field was observably read from the receiver and from the object returned from mergeFields(), but never used. This is an unnecessary user call.

ptomato force-pushed the 2462-process-receiver-first branch from f35fc58 to 7aaab13 Compare February 1, 2023 18:43

ptomato marked this pull request as ready for review February 1, 2023 18:43

Update test262

f25466a

ptomato merged commit 98607ce into main Feb 1, 2023

ptomato deleted the 2462-process-receiver-first branch February 1, 2023 18:50

This was referenced Jul 12, 2023

Normative: don't coerce non-string primitives to strings, esp. time zones, offsets, and calendars #2574

Merged

Normative: Audit of user-visible operations in Temporal #2519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Validate receiver fields before fields of any param objects #2478

Normative: Validate receiver fields before fields of any param objects #2478

ptomato commented Jan 16, 2023

ptomato Jan 16, 2023

gibson042 Jan 17, 2023

codecov bot commented Jan 16, 2023 •

edited

Loading

ptomato commented Jan 16, 2023

gibson042 Jan 17, 2023

gibson042 Jan 17, 2023

ptomato Jan 17, 2023

gibson042 Jan 17, 2023 •

edited

Loading

ptomato Jan 18, 2023

gibson042 Jan 18, 2023

ptomato Jan 18, 2023

gibson042 Jan 18, 2023

ptomato Jan 19, 2023

gibson042 Jan 19, 2023 •

edited

Loading

ptomato commented Jan 17, 2023

ptomato commented Feb 1, 2023

Normative: Validate receiver fields before fields of any param objects #2478

Normative: Validate receiver fields before fields of any param objects #2478

Conversation

ptomato commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2023 • edited Loading

Codecov Report

ptomato commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

ptomato commented Jan 17, 2023

ptomato commented Feb 1, 2023

codecov bot commented Jan 16, 2023 •

edited

Loading

gibson042 Jan 17, 2023 •

edited

Loading

gibson042 Jan 19, 2023 •

edited

Loading