Recursive calendar or timeZone property not rejected if property value is a Temporal object

[justingrant]

According to #925 (comment):

• 3.1 To prevent infinite loops, recursive property bags aren't supported. The following should throw:

TimeZone.from({timeZone: {timeZone: 'Asia/Tokyo'}});
Calendar.from({calendar: {calendar: 'japanese'}});

and

5. The statements above that relate to Calendar.from and TimeZone.from also apply to any Temporal API where a calendar and/or timezone is accepted as input.

I found an interesting case where the no-recursion limit may not hold true: if there's a 2-level recursive property bag where a non-TimeZone/non-Calendar Temporal instance is used as the second-level calendar or timeZone property value. Repro:

date = Temporal.PlainDate.from('2020-01-01');
Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: 'iso8601' });
  // no exception (expected) 

Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: date });
  // no exception (expected) 

Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: 'iso8601' } });
  // no exception (expected) 

Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: date } });
  // expected?: RangeError (because recursive property bags should not be allowed)
  // actual: no exception; `date.calendar` is used as the calendar of the resulting instance

// Deeper recursion is disallowed, as expected: 
Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: { calendar: date } } });
  // RangeError 

Tracing through the polyfill code, what's happening in the unexpected case above is that the nested calendar property is not a Calendar so it's assumed to be either a user-generated calendar or a string. Then it's coerced to a string: "2020-01-01". Finally, that string is passed to ToTemporalCalendar which doesn't see a calendar annotation in that string so returns the ISO calendar.

As I remember, the reason recursion was prohibited was to prevent infinite loops. An infinite loop can't happen here. But the current behavior does feel at least somewhat contrary to our "no recursion" rule, because date isn't a calendar... instead the calendar is inside date which is one more level of recursion.

Is this OK?

I'm inclined to say that the current behavior is not exactly as intended, but it doesn't seem to be harmful and we're in Stage 3, so we can just leave it as-is. But I wanted to check with the other champions in case I'm missing something.

[ptomato]

Your polyfill trace checks out with how I interpret the spec text:

• Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: date } })
  • 3. ToTemporalDate({ year: 2020, month: 1, day: 1, calendar: { calendar: date } }, Object.create(null))
    • 3.d. calendar = GetTemporalCalendarWithISODefault({ year: 2020, month: 1, day: 1, calendar: { calendar: date } })
      • 2. calendarLike = { calendar: date }
      • 3. ToTemporalCalendarWithISODefault({ calendar: date })
        • 2. ToTemporalCalendar({ calendar: date })
          • 1.b. HasProperty is true, so no return
          • 1.c. temporalCalendarLike = date
          • 1.d. Type is Object and HasProperty is true, so no return
          • 2. identifier = date.toString()
          • etc.

I wonder if it was maybe unintended that falling off the end of Step 1 of ToTemporalCalendar should go on to Step 2 where ToString is called on the object? Maybe there's a missing throw there?

There are a couple of cases I could think of where this would make a difference:

Temporal.PlainDate.from({
  year: 2020, month: 1, day: 1, calendar: {
    calendar: {
      calendar: 'gregory',
      toString() { return 'gregory'; },
    },
  },
});
// returns 2020-01-01[u-ca=gregory], with the built-in 'gregory' calendar,
// despite being a plain object with 3x nested calendar property

customCal = { toString() { return "not-builtin"; } };
date = new Temporal.PlainDate(2020, 1, 1, customCal);
Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: date } });
// throws RangeError "invalid calendar identifier 'not-builtin'"
// despite being the same invocation that succeeds in the example above

[justingrant]

This issue is complicated enough that it may be best to walk through in real time. My eyes glaze over when I try to understand all the various choose-your-own-adventure paths here. Should we cover this at a future meeting, or perhaps in a 1:1 call?

[ptomato]

Yes, let's talk over this in a 1:1 call and try to come up with a recommendation for the champions group, that we can decide quickly yes/no in the next meeting.

[justingrant]

@ptomato and I paired and came up with a solution: at the end of Step 1, instead of falling through, we'd require the calendar property to be a calendar identifier (or an object that's coercible to one) and throw otherwise. It's very similar to the other branch starting with Step 2, except no call to ParseTemporalCalendarString.

However, we're not sure it's worth making this obscure change in Stage 3. Let's discuss.

    <emu-clause id="sec-temporal-totemporalcalendar" aoid="ToTemporalCalendar">
      <h1>ToTemporalCalendar ( _temporalCalendarLike_ )</h1>
      <emu-alg>
        1. If Type(_temporalCalendarLike_) is Object, then
          1. If _temporalCalendarLike_ has an [[InitializedTemporalDate]], [[InitializedTemporalDateTime]], [[InitializedTemporalMonthDay]], [[InitializedTemporalTime]], [[InitializedTemporalYearMonth]], or [[InitializedTemporalZonedDateTime]] internal slot, then
            1. Return _temporalCalendarLike_.[[Calendar]].
          1. If ? HasProperty(_temporalCalendarLike_, *"calendar"*) is *false*, return _temporalCalendarLike_.
          1. Set _temporalCalendarLike_ to ? Get(_temporalCalendarLike_, *"calendar"*).
          1. If Type(_temporalCalendarLike_) is Object and ? HasProperty(_temporalCalendarLike_, *"calendar"*) is *false*, return _temporalCalendarLike_.
          1. Let _identifier_ be ? ToString(_temporalCalendarLike_).
          1. If IsBuiltinCalendar(_identifier_) is *false*, throw a *RangeError* exception.
          1. Return ! CreateTemporalCalendar(_identifier_).
        1. Let _identifier_ be ? ToString(_temporalCalendarLike_).
        1. If IsBuiltinCalendar(_identifier_) is *false*, then
          1. Set _identifier_ to ? ParseTemporalCalendarString(_identifier_).
          1. If IsBuiltinCalendar(_identifier_) is *false*, throw a *RangeError* exception.
        1. Return ! CreateTemporalCalendar(_identifier_).
      </emu-alg>
    </emu-clause>

[ptomato]

We had some discussion on this at the 2022-05-12 champions meeting but didn't come to a conclusion, partly because it's difficult to understand what exactly is going on in the edge case. As for Justin's and my recommendation to change nothing, at least @gibson042 considers this worth fixing. We'll continue discussing this next time.

[ptomato]

Meeting, 2022-07-07: We will make the above fix that Justin outlined; given that we are proposing several other similarly subtle changes from implementor experience.

[ptomato]

I've found that the proposed fix is inconsistent:

const bag = { year: 2022, month: 7, day: 11, calendar: "2016-12-31T23:59[u-ca=gregory]" };
Temporal.PlainDate.from(bag)
  // returns a PlainDate with 'gregory' built-in calendar
Temporal.Calendar.from(bag)
  // throws RangeError (expected: 'gregory' built-in calendar?)

It seems to me that this is not intended, because it violates the principle that a property bag with the appropriate properties should be able to stand in for a Temporal object (and Temporal.Calendar.from(plainDate) is fine).

I'm not sure what to do with this. We may have to special-case Temporal.Calendar.from() to treat its argument differently than just running it through ToTemporalCalendar.

[gibson042]

Honestly, I find the success of Temporal.PlainDate.from({ year: 2022, month: 7, day: 11, calendar: "2016-12-31T23:59[u-ca=gregory]" }) much more surprising than the failure of Temporal.Calendar.from with the same input—a date-time string with calendar annotation is not a good representation of a calendar, and should not be valid in my opinion.

If Calendar.from were designed like e.g. PlainDate.from, then there would be only be one code path for extracting a Calendar instance from the [[Calendar]] slot of a Temporal type instance or a non-Temporal-type-instance object's "calendar" property (the above-mentioned equivalence) and a mutually exclusive code path for serializing the input to a string and interpreting the result as a calendar type (NB: not for extracting a calendar type from input representing other data along with a calendar). The first code path would not accept the name of a calendar in the "calendar" property any more than PlainDate.from would accept a PlainDate instance in the "day" property of plain object input—Temporal.PlainDate.from({ year: 2022, month: 7, day: Temporal.Now.plainDateISO() }) justifiably throws an exception, as would Temporal.Calendar.from({ calendar: "iso8601" }) and Temporal.PlainDate.from({ year: 2022, month: 7, day: 11, calendar: { calendar: "iso8601" } }) (inverting the "no exception (expected)" case above that seems to be at the root of this) because "iso8601" is not a Calendar instance and is not an expected value of the "calendar" property of any Temporal type instance.

I would like to see that model adopted, in which there is no deep processing and a property bag is instead subject to the same expectations as a Temporal type instance—specifically, that any "calendar" property value must be an object and not undergo further transformation. Would it break any relevant use cases?

---

And (I hope) obviously, TimeZone and Calendar should behave similarly with respect to both from functions and To<Type> operations.

[justingrant]

@ptomato - does the original proposed fix in this comment have the same issue?

@gibson042 - there are a some use cases where it's helpful to have Temporal.[Calendar,TimeZone].from be able to operate on full ISO strings. One of them is an ergonomic way to parse the date/time portion of an Instant string:

function parseDateTimeOfInstantString(s) {
  return Temporal.Instant.from(s).toZonedDateTimeISO(s).toPlainDateTime();
}

parseDateTimeOfInstantString('2020-01-01T12:34Z');
// => 2020-01-01T12:34:00
parseDateTimeOfInstantString('2020-01-01T12:34+02:00');
// => 2020-01-01T12:34:00

Another case: do two ISO strings share the same calendar, without needing to build different versions of this function for every Temporal type? (and while being able to compare strings of different types too)

function hasSameCalendar (s1, s2) {
  return Calendar.from(s1).id === Calendar.from(s2).id;
}

I suspect there are more cases like this: admittedly not super-popular ones, but useful enough that I'd want to see some specific gain we get from removing this capability that outweighs the loss of cases like these.

[ptomato]

does the original proposed fix in this comment have the same issue?

Yes, the issue arose with that fix.

[gibson042]

@justingrant Thank you for the examples. I appreciate the cleverness and convenience, but I've got to be honest—they both look like kludges to me, difficult to comprehend and probably rather brittle. The right way to handle them would probably be a generic multi-type parser of the sort that has been kicked down the road, and I'm personally uncomfortable with the workaround.

[ptomato]

It sounds like we need to withdraw this one from presenting in next week's meeting, as it's not clear-cut like we initially thought.

[ptomato]

In the 2022-10-27 Temporal champions meeting, we discussed this and didn't reach any agreement. However, if we go with the builtin-calendars-as-strings resolution of #1808, this issue will change substantially and might no longer need to be solved. So, the discussion is deferred until #1808 is resolved.

[ptomato]

Champions meeting, 2023-01-19: We have a resolution of #1808. It doesn't require a change to this issue, so the issue still needs to be solved. Therefore this goes back on the agenda next week.

One proposal is to go with what Richard suggested, which I'll prepare a draft PR for. That PR may need to be staged on top of the solution to #1808.

I just verified that the proposed solution for #1808 changes the example (4) above to throw a TypeError. I need to investigate whether this is an unavoidable consequence or an unintended side effect that can be removed. Regardless, we agreed in the meeting that we'd proceed with #1808 whether or not it affected the edge cases listed in this issue.

[ptomato]

Discussed in the champions meeting of 2023-01-26, although it took a while to work out the details. This explanation is partly a rehash of my comment on #2483. I'll be describing the situation with time zones but the same resolution was made for calendars, without loss of generality.

Currently, the following kinds of input are accepted in ToTemporalTimeZone:

1. Temporal.TimeZone instance, of course
2. Temporal.ZonedDateTime instance (the time zone is obtained from the [[TimeZone]] slot)
3. Plain object (distinguished from (7) by not having a timeZone property), assumed to be an object implementing the time zone protocol
4. Time zone ID (Asia/Tokyo or -07:00)
5. ISO string with annotation, possibly with other components missing (e.g. 2023-01-25T00[Asia/Tokyo], 2023-01-25T00-07:00[-07:00])
6. ISO string without annotation, getting offset time zone from UTC offset or Z if present (e.g. 2023-01-25T00-07:00, 2023-01-25T00Z)
7. Plain object with a timeZone property, interpreted recursively up to depth 1 (e.g. { timeZone: "2023-01-25T00Z" })

The builtin-time-zones-as-strings change discussed in #1808 (and implemented in PR #2482), makes it necessary to reexamine this list, because now ZonedDateTime's [[TimeZone]] internal slot may contain either a string (for fast, builtin time zone behaviour where engines can elide creating any objects) or an object (for custom time zone behaviour where the engine has to call methods on the object.) We don't want to create a situation where user code can easily and silently drop from the fast builtin time zone path into the slow custom time zone path.

Note that the types of input given in (1), (3), and (4) are semantically time zones themselves, while the conversions in (2), (5), (6), and (7) are extracting a time zone from another object that is semantically not a time zone. We're conceptually splitting these cases into abstract operations ToTimeZone and ExtractTimeZone. (No guarantee that the spec text will actually use these names)

We are dropping (7) altogether. It's now not worth the confusion about whether it produces a ZonedDateTime with a slow-path or fast-path time zone. Without (7), you always know: strings are fast builtin time zones, ZonedDateTime instances preserve whatever the instance already had, and other objects are slow custom time zones.

For (2), even though this is, strictly speaking, extracting a time zone rather than accepting a time zone object, we'll continue to support this case in ToTimeZone so that it's easier and more natural for user code to stay on the fast path if it is already on it. Otherwise, you would have to use timeZone: zonedDateTime.getISOFields().timeZone.

For (3), we'll do HasProperty checks on the required properties from the protocol, and throw a TypeError if the object doesn't implement the protocol. These checks are only observable by Proxy objects, so if the object isn't a Proxy and doesn't have one in its prototype chain, implementations are free to elide these checks.

Interpretation of property bags that include a timeZone property, such as those passed to Temporal.ZonedDateTime.from(), will only perform the ToTimeZone operation. So, the following are good input for Temporal.ZonedDateTime.from():

// builtin time zones:
Temporal.ZonedDateTime.from({ ...ymd, timeZone: 'Asia/Tokyo' })
Temporal.ZonedDateTime.from({ ...ymd, timeZone: '-07:00' })
Temporal.ZonedDateTime.from({ ...ymd, timeZone: Temporal.Now.zonedDateTimeISO() })
// custom time zones:
Temporal.ZonedDateTime.from({ ...ymd, timeZone: Temporal.TimeZone.from(...) })
Temporal.ZonedDateTime.from({ ...ymd, timeZone: { getOffsetNanosecondsFor(...) {...}, ... } })

The following are not good input for Temporal.ZonedDateTime.from():

Temporal.ZonedDateTime.from({ ...ymd, timeZone: '2023-01-26T00[Asia/Tokyo]' })
Temporal.ZonedDateTime.from({ ...ymd, timeZone: {} })

We additionally considered allowing these property bags to alternately have a timeZoneId property which may only be a string identifier, but have not made that decision at this time.

Interpretation of input passed where a time zone is expected, such as Temporal.TimeZone.from(), or Temporal.Instant.prototype.toZonedDateTimeISO(), will additionally perform the ExtractTimeZone operation if ToTimeZone fails. So, the following are good input for entry points taking time zones:

// builtin time zones:
instant.toZonedDateTimeISO('Asia/Tokyo')
instant.toZonedDateTimeISO('-07:00')
instant.toZonedDateTimeISO('2023-01-26T00Z')
instant.toZonedDateTimeISO('2023-01-26T00-07:00')
instant.toZonedDateTimeISO('2023-01-26T00[Asia/Tokyo]')
instant.toZonedDateTimeISO(Temporal.Now.zonedDateTimeISO())
// custom time zones:
instant.toZonedDateTimeISO(Temporal.Now.zonedDateTimeISO().getTimeZone())
instant.toZonedDateTimeISO({ getOffsetNanosecondsFor(...) {...}, ... })

Concretely, for the examples given in the OP of this thread:

date = Temporal.PlainDate.from('2020-01-01');
/* 1 */ Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: 'iso8601' });
// no exception, iso8601 calendar (unchanged)
	  
/* 2 */ Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: date });
// no exception, iso8601 calendar (unchanged)
  
/* 3 */ Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: 'iso8601' } });
// Before: no exception, iso8601 calendar
// After: TypeError, object doesn't implement the calendar protocol
	  
/* 4 */ Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: date } });
// Before: no exception, iso8601 calendar (but the lack of an exception was the unexpected behaviour that caused this issue to be opened in the first place)
// After: TypeError, object doesn't implement the calendar protocol
	  
/* 5 */ Temporal.PlainDate.from({ year: 2020, month: 1, day: 1, calendar: { calendar: { calendar: date } } });
// Before: RangeError (`[object Object]` is not parseable as an ISO string)
// After: TypeError, object doesn't implement the calendar protocol

#2485 is updated as per these conclusions.