Temporal.TimeZone.prototype.equals: Missing call to ToTemporalTimeZoneSlotValue

[ptomato]

From @anba in https://github.com/tc39/proposal-temporal/issues/2682:

Step 3:

Return ? TimeZoneEquals(timeZone, other).

But other first needs to be converted to a time-zone value through ToTemporalTimeZoneSlotValue.

[ptomato]

I missed this during stage 3 review, unfortunately. I think we should just fix it since it's pretty clear what the intention was. However the current behaviour doesn't violate any assertions and would be observable, so I suspect it's technically a normative change. What do you think?

[ptomato]

Thinking about it some more, maybe the change does not have to be a normative change after all. By my reading, here's what I see happening for four different inputs, (1) string identifier of built-in time zone, (2) other string, (3) instance of Temporal.TimeZone, (4) plain object implementing TimeZone protocol, (5) other plain object, (6) other non-string non-object value.

1. timeZone.equals("America/Vancouver")

• TimeZoneEquals step 3 calls ToTemporalTimeZoneIdentifier("America/Vancouver")
• ToTemporalTimeZoneIdentifier step 1.b returns the value unchanged.
• TimeZoneEquals step 6 does not throw because the identifier conforms to the TimeZoneIdentifier grammar.
• The rest of the operation completes normally.
• → No change from what would happen with ToTemporalTimeZoneSlotValue.

2. timeZone.equals("not a time zone")

• TimeZoneEquals step 3 calls ToTemporalTimeZoneIdentifier("not a time zone")
• Assertion failure in ToTemporalTimeZoneIdentifier step 1.a.
• → Current spec is unimplementable. ToTemporalTimeZoneSlotValue should throw in step 3 in this case.

3. class MyTimeZone extends Temporal.TimeZone {}; timeZone.equals(new MyTimeZone("UTC"))

• TimeZoneEquals step 3 calls ToTemporalTimeZoneIdentifer and passes the MyTimeZone instance.
• ToTemporalTimeZoneIdentifier step 2 observably calls Get("id") on the MyTimeZone instance
• ToTemporalTimeZoneIdentifier step 4 returns "UTC".
• The rest of the operation completes normally.
• → No change from what would happen with ToTemporalTimeZoneSlotValue.

4. timeZone.equals({ getOffsetNanosecondsFor() {}, getPossibleInstantsFor() {}, id: "UTC" })

• TimeZoneEquals step 3 calls ToTemporalTimeZoneIdentifier and passes the plain object.
• ToTemporalTimeZoneIdentifier step 2 observably calls Get("id") on the plain object.
• ToTemporalTimeZoneIdentifier step 4 returns "UTC".
• The rest of the operation completes normally.
• → Changed: with ToTemporalTimeZoneSlotValue, we would see observable Has calls on the plain object in ObjectImplementsTemporalTimeZoneProtocol.

5. timeZone.equals({})

• TimeZoneEquals step 3 calls ToTemporalTimeZoneIdentifier and passes the plain object.
• ToTemporalTimeZoneIdentifier step 2 observably calls Get("id") on the plain object.
• ToTemporalTimeZoneIdentifier step 3 throws a TypeError because the value is undefined.
• → Changed: with ToTemporalTimeZoneSlotValue, we would see observable Has calls on the plain object in ObjectImplementsTemporalTimeZoneProtocol.

6. timeZone.equals(true)

• TimeZoneEquals step 3 calls ToTemporalTimeZoneIdentifier(true).
• Assertion failure because ToTemporalTimeZoneIdentifier takes a String or Object parameter.
• → Current spec is unimplementable. ToTemporalTimeZoneSlotValue should throw in step 2 in this case.

So it looks like the spec is unimplementable as written and therefore this may be classified as an editorial change after all. What do you think?

[ptomato]

#42 is a possible fix.

[anba]

So it looks like the spec is unimplementable as written and therefore this may be classified as an editorial change after all. What do you think?

Yes, that's correct. The current spec isn't actually implementable.

[justingrant]

Good catch finding this! Given that this proposal has already been merged into Temporal, I assume that the fix should be on the Temporal side?

We can also fix it here too for comprehensiveness, but the fix that really matters for implementers will be in Temporal, right?

[ptomato]

Yes, I'd copy the fix into Temporal once it landed here.

[justingrant]

Cool. I'll review the PR shortly and so we should be able to turn this around quickly. And I agree that this is editorial because the current spec is not implementable.

[justingrant]

Approved and merged here. @ptomato, ball is in your court now.