Options object consumption is observably different in internal branches

[gibson042]

I haven't checked how recent or how widespread this is, but the current spec text exposes internal branching by how it consumes an options object in at least Temporal.PlainDate.from, Temporal.PlainDateTime.from, Temporal.PlainYearMonth.from, Temporal.PlainMonthDay.from, and Temporal.ZonedDateTime.from, e.g.

const zdtString = "2024-05-21T12:00[America/New_York]";
const makeSpy = () => new Proxy({ log: [] }, {
  ownKeys: obj => obj.log.push("[[OwnPropertyKeys]]") && ["disambiguation", "offset", "overflow", "log"].reverse(),
  getOwnPropertyDescriptor: (obj, key) => obj.log.push(`[[GetOwnProperty]] ${key}`) &&
    { value: undefined, writable: true, enumerable: true, configurable: true },
  get: (obj, key) => obj.log.push(`[[Get]] ${key}`) && obj[key],
});
const spy1 = makeSpy(), spy2 = makeSpy();
Temporal.ZonedDateTime.from(Temporal.ZonedDateTime.from(zdtString), spy1);
Temporal.ZonedDateTime.from(zdtString, spy2);

spy1.log.slice(0, -1);
/* => [
  '[[Get]] disambiguation',
  '[[Get]] offset',
  '[[Get]] overflow',
] */

spy2.log.slice(0, -1);
/* => [
  '[[OwnPropertyKeys]]',
  '[[GetOwnProperty]] log',
  '[[Get]] log',
  '[[GetOwnProperty]] overflow',
  '[[Get]] overflow',
  '[[GetOwnProperty]] offset',
  '[[Get]] offset',
  '[[GetOwnProperty]] disambiguation',
  '[[Get]] disambiguation',
] */

All of these functions should SnapshotOwnProperties up front, passing down an unobservable clone to later steps and deeper operations.

[anba]

It seems unfortunate to have even more copying of options object just to support the custom calendars use case. Do we really need to copy all properties instead of just creating a new object which has a single "overflow" property? It's more easy for implementations to lazily allocate an object with a single property than somehow deferring the allocation of a copied options object.

(This extra overhead makes me sad, especially when compared to other proposals which micro-optimise to avoid a dozen or so assembly instructions.)

[ptomato]

Copying the own properties is only necessary if we need to pass the same options object to more than one user hook, which might mess with the object. In other words, regard any options object as poisoned after it's been exposed to user code. I don't think we should copy the own properties if not necessary.

Besides, this is not a change that I think we should even be considering at this point. Changes must be targeted at the goal of addressing implementations' concerns about making the proposal smaller and less complex, which this does not.

[gibson042]

I believe the options snapshotting (taking all properties where necessary to support user code reading arbitrary properties, but otherwise partial is fine) should be done at user code boundaries, and only there. Picking on the example above w.l.o.g., it doesn't make sense for Temporal.ZonedDateTime.from and ToTemporalZonedDateTime to both treat options as foreign. The former should either

ingest options before branching on "clone" vs. "create from scratch"

       <h1>Temporal.ZonedDateTime.from ( _item_ [ , _options_ ] )</h1>
       <p>
         This function performs the following steps when called:
       </p>
       <emu-alg>
-        1. Set _options_ to ? GetOptionsObject(_options_).
+        1. Set _options_ to ? SnapshotOwnProperties(! GetOptionsObject(_options_), *null*).
         1. If _item_ is an Object and _item_ has an [[InitializedTemporalZonedDateTime]] internal slot, then

         ToTemporalZonedDateTime (
           _item_: an ECMAScript language value,
-          optional _options_: an Object,
+          optional _resolvedOptions_: an Object,
         ): either a normal completion containing a Temporal.ZonedDateTime, or a throw completion
       </h1>
       <dl class="header">
         <dt>description</dt>
         <dd>
           It returns its argument _item_ if it is already a Temporal.ZonedDateTime instance, converts _item_ to a new Temporal.ZonedDateTime instance if possible, and throws otherwise.
         </dd>
       </dl>
       <emu-alg>
-        1. If _options_ is not present, set _options_ to *undefined*.
-        1. Let _resolvedOptions_ be ? SnapshotOwnProperties(! GetOptionsObject(_options_), *null*).
+        1. If _resolvedOptions_ is not present, set _resolvedOptions_ to ! GetOptionsObject(*undefined*).
+        1. Assert: _resolvedOptions_ is an ordinary extensible Object that is not directly observable from ECMAScript code and for which the value of the [[Prototype]] internal slot is null and every property is a configurable data property.
         1. Let _offsetBehaviour_ be ~option~.
         1. Let _matchBehaviour_ be ~match-exactly~.

or else should

defer completely to ToTemporalZonedDateTime with a signal that creation is mandatory

       <h1>Temporal.ZonedDateTime.from ( _item_ [ , _options_ ] )</h1>
       <p>
         This function performs the following steps when called:
       </p>
       <emu-alg>
-        1. Set _options_ to ? GetOptionsObject(_options_).
-        1. If _item_ is an Object and _item_ has an [[InitializedTemporalZonedDateTime]] internal slot, then
-          1. NOTE: The following steps read options and perform independent validation in alphabetical order (ToTemporalDisambiguation reads *"disambiguation"*, ToTemporalOffset reads *"offset"*, and ToTemporalOverflow reads *"overflow"*).
-          1. Perform ? GetTemporalDisambiguationOption(_options_).
-          1. Perform ? GetTemporalOffsetOption(_options_, *"reject"*).
-          1. Perform ? GetTemporalOverflowOption(_options_).
-          1. Return ! CreateTemporalZonedDateTime(_item_.[[Nanoseconds]], _item_.[[TimeZone]], _item_.[[Calendar]]).
         1. Return ? ToTemporalZonedDateTime(_item_, _options_).
       </emu-alg>

         ToTemporalZonedDateTime (
           _item_: an ECMAScript language value,
-          optional _options_: an Object,
+          optional _creationOptions_: an Object,
         ): either a normal completion containing a Temporal.ZonedDateTime, or a throw completion
       </h1>
       <dl class="header">
         <dt>description</dt>
         <dd>
-          It returns its argument _item_ if it is already a Temporal.ZonedDateTime instance, converts _item_ to a new Temporal.ZonedDateTime instance if possible, and throws otherwise.
+          It returns its argument _item_ if it is already a Temporal.ZonedDateTime instance and _creationOptions_ is not present, converts _item_ to a new Temporal.ZonedDateTime instance if possible, and throws otherwise.
         </dd>
       </dl>
       <emu-alg>
-        1. If _options_ is not present, set _options_ to *undefined*.
-        1. Let _resolvedOptions_ be ? SnapshotOwnProperties(! GetOptionsObject(_options_), *null*).
+        1. NOTE: These added steps could be abstracted into their own operation.
+        1. If _creationOptions_ is not present or _creationOptions_ is *undefined*, then
+          1. Let _resolvedOptions_ be ! GetOptionsObject(*undefined*).
+        1. Else,
+          1. Let _resolvedOptions_ be ? SnapshotOwnProperties(? GetOptionsObject(_creationOptions_), *null*).
         1. Let _offsetBehaviour_ be ~option~.
         1. Let _matchBehaviour_ be ~match-exactly~.
         1. If _item_ is an Object, then
           1. If _item_ has an [[InitializedTemporalZonedDateTime]] internal slot, then
-            1. Return _item_.
+            1. If _creationOptions_ is not present, return _item_.
+            1. Return ! CreateTemporalZonedDateTime(_item_.[[Nanoseconds]], _item_.[[TimeZone]], _item_.[[Calendar]]).

[gibson042]

Copying the own properties is only necessary if we need to pass the same options object to more than one user hook, which might mess with the object. In other words, regard any options object as poisoned after it's been exposed to user code. I don't think we should copy the own properties if not necessary.

Then what's the point of Temporal.ZonedDateTime.from reading disambiguation, offset, and overflow only to ignore the results, or ToTemporalZonedDateTime snapshotting before checking for a [[InitializedTemporalZonedDateTime]] slot that short-circuits out? I believe the point is hiding implementation details in support of future improvement, and the .from functions are currently failing in that goal.

Besides, this is not a change that I think we should even be considering at this point. Changes must be targeted at the goal of addressing implementations' concerns about making the proposal smaller and less complex, which this does not.

As seen above, it actually does—the second approach in particular would unify exposed .from functions with internal To$Type operations.

[ptomato]

Sorry, but I just don't agree with that. This kind of minor adjustment is not appropriate anymore at this point, even if it results in a bit of a weird inconsistency. We agreed to fix showstopper bugs only, which this is not.

Luckily, this becomes moot if we remove custom calendars and time zones. We won't have to snapshot own properties of any options object at all, because none of them will be passed to user methods.

[gibson042]

Sorry, but I just don't agree with that. This kind of minor adjustment is not appropriate anymore at this point, even if it results in a bit of a weird inconsistency. We agreed to fix showstopper bugs only, which this is not.

I believe literally nobody is claiming that epochMicroseconds support is a showstopper, and yet #2849 is still under consideration. And unlike simple removals, this kind of bug has the potential to hinder future improvements.

Luckily, this becomes moot if we remove custom calendars and time zones. We won't have to snapshot own properties of any options object at all, because none of them will be passed to user methods.

This I agree with.

[ptomato]

Since we approved removing custom calendars and time zones in the TC39 plenary of 2024-06-12, I think we have agreed that this no longer applies. In my branch implementing that removal, I've removed all of the offending uses of SnapshotOwnProperties and just read the relevant properties eagerly when processing the function's argument. (@gibson042 when I post that PR and you review it, please do check that we agree on how it should be implemented.)

I think we can close this now.