delegate review: Calendar and TimeZone should be brand-checked, not normal, objects

[ljharb]

from #1275 (comment):

• https://tc39.es/proposal-temporal/#sec-temporal-createtemporaldate only requires that calendar be an Object, it does not validate that it has the internal slots of a Temporal.Calendar - it should. This appears to also be the case for all the other constructors that accept a calendar. Additionally, all the abstract operations that accept one should assert their type in some way.
• similarly, https://tc39.es/proposal-temporal/#sec-temporal-createtemporalzoneddatetime does not validate that timeZone is actually a TimeZone object, only that it is an Object.
• Why does https://tc39.es/proposal-temporal/#sec-temporal-calendartostring exist at all? ToString(calendar) should achieve the same thing as long as the calendar extends Temporal.Calendar (which, per above, it should).

[Ms2ger]

• Supporting custom calendars that don't inherit from Temporal.Calendar is a deliberate design decision, though I don't know how strongly the champions feel about that at this point.
• Same for TimeZone.
• I don't recall exactly why CalendarToString needs or wants the fallback to %Temporal.Calendar.prototype.toString%; it's a required method of the protocol.

[ljharb]

I'd love to hear more about that design decision; given that Temporal.Calendar exists, allowing a "calendar-like" to be either a subclass OR an object conforming to an interface would be repeating the design decisions of RegExp, which seems to have be consistently almost-universally disliked.

[ptomato]

Background and discussion for this decision is in #300.

The %Temporal.Calendar.prototype.toString% does seem to be unnecessary. I'll check whether it can be removed.

[littledan]

Calendar and TimeZone are protocols, like Iterator. We can get a hint for what to do here by following the pattern there.

Even though there is a built-in iterator prototype, there is no iterator brand that for-of loops check. However, particular implementations of iterators do have brand checks that are used in their next method (since internal slate is needed.)

Calendar and TimeZone are also protocols, with built-in implementation. There is a need to brand-check in methods which use the built-in implementations' internal state (e.g., in the methods on Temporal.Calendar.prototype) because they are specific to that implementation of the protocol. However, it is not appropriate to check for a brand for usages of the protocol (such as the cases @ljharb cited); instead, they should just call the appropriate methods at the appropriate time.

We discussed this issue in the Temporal champion group meeting on 2021-01-21. I agreed with everyone else in that call that we don't want to hold up this proposal over this relatively superficial issue. At the same time, we think the current design of the proposal makes sense.

It would be possible to meet all use cases by forcing everyone who creates a custom calendar to extend a built-in one and override all methods, leaving the internal state vestigial and unused. However, this doesn't fit with how I understand TC39 to typically create protocols, and I'd be confused about why we'd start here and whether we'd want to continue with this form of design.

[ljharb]

New protocols should be symbol-based, not string-based, but all the calendar and timezone methods have string methods.

It's fine to have a protocol or a class, but having both repeats the mistakes of RegExp. I don't find this a superficial issue, and I do think it should hold up advancement of the entire proposal. Indeed, I want to force all custom calendars to extend the builtin calendar, as RegExp should have done.

[littledan]

The thing that makes this a protocol isn't whether it does brand checking or not, but whether it calls user-defined methods. Whether we do a brand check doesn't affect whether this repeats the pattern of RegExp. I am proposing a discussion topic for TC39 on the nature of protocols to continue this thread (or we can have a dedicated call if there isn't enough time).

[ptomato]

Here is some context on why strings are used for the protocol method names and not symbols:

• Symbols or strings for Temporal.Calendar and Temporal.TimeZone methods? #310
• https://github.com/tc39/notes/blob/master/meetings/2020-02/february-6.md#strings-or-symbols-for-temporalcalendar-protocol
• https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2020-02-20.md

[ljharb]

Worth pointing out that in the linked notes:

JHD: It seems that this should be a subclass that uses an internal slot, and use strings

so this isn't a new opinion i'm bringing up :-)

[littledan]

@ljharb Sure, but I'm having trouble understanding what you meant in #300 (comment) , where I got the impression that you agreed with not doing brand checking:

This seems right to me; it enables non-subclasses to participate in the protocol, but the "happy path" is to subclass the builtin objects, and override methods.

[ljharb]

Rereading it, i clearly misunderstood your OP there. I expect calendar-specific data and operations to only be read by the calendar’s methods, but i still expect everything calendar-like to have the internal slots of a calendar.

[littledan]

Do you want non-subclasses to be able to participate in the Calendar and TimeZone protocols? If so, how should they do it?

[ljharb]

No, i do not want that. Only subclasses should be considered true calendars, whether they use built-in methods or override them all.

[ljharb]

If these objects are spiritually intended to be options bags, then @littledan's opinion here: tc39/ecma402#132 is exactly what I think Temporal should be doing (as opposed to storing the Calendar/TimeZone object and Get-ing the methods off of it every time).

[Louis-Aime]

I am not sure to understand all the stakes that are discussed here, and my opinion only reflects a calendar's author experience.
I defined my experimental calendars as objects, not as subclasses. The first reason was that I wished to freely define the calendar's ID string. A second reason was that I wanted to define a general "scheme" for the historical calendars of the western European countries: Julian, then Gregorian, with a switching date to Gregorian by country (if not by town), and for this, I (possibly wrongly) thought that I had to define a complete class of its own.
Of course calendar authors may make mistakes, but I think that they should have a good level of freedom, as long as there is no harm for the servers and other users. There are already many controls, including errors when the author's code does not conform to the documented rules.
I am not aware of the problem with RegExp, but my feeling is that there much less "degrees of freedom" with Temporal objects than with RegExp.

[littledan]

@ljharb To allow non-subclasses to be used as Calendars or TimeZones, the spec points that you started this thread with must not add checks in the locations that you mentioned in the start of this issue. It's up to JS programmers to decide what they want to consider "true" calendars, but if you want to check whether an instance has TimeZone or Calendar internal slots, you can use the toString() methods.

About reading the methods more eagerly: I'm not sure if we have a strong convention to adopt from ECMA-402 (since that issue is still open, and it was just a suggestion), but the main benefit that I see of reading these methods early is to make it easier to implement an optimization to avoid bothering to allocate TimeZone and Calendar objects and present things as strings instead. If there is a Realm-wide "protector" bit, the protector would only need to be checked when PlainDate, etc, objects are created, not when they are used (it's a fairly minor difference tbh). This is analogous to how the next method get is now hoisted in the iteration protocol. I think this change is worth looking into, but it's such a small corner that it shouldn't affect the rest of the Stage 3 review.

[ljharb]

The convention in 262 for reading eagerly is in all of the Promise combinators, where resolve is now looked up eagerly on the constructors.

The reason I think it's important now is that doing so would reinforce and endorse the view that a Calendar and a TimeZone are always expected to be normal options bags, and that the classes only exist as convenient factories for those options bags.

[ptomato]

At long last and after fixing several bugs which this helped uncover, I have a version of eagerly reading in the Calendar and TimeZone methods that I think could work. It introduces Calendar Records and TimeZone Records, which are stored in Temporal objects' internal slots instead of the Calendar and TimeZone objects directly. You can see how it is implemented in the polyfill at https://github.com/tc39/proposal-temporal/compare/1294-methods-bags?expand=1 I would like to get feedback on the way that this works before I spend the time writing the spec text.

On the positive side, it eliminates the need to think about how many observable property Gets we do in the more complicated calendar-dependent algorithms such as BalanceDurationRelative. @littledan pointed out that, thinking ahead to implementation in engines, it also eliminates the need to do a "prototype has been mutated" check every time a Calendar or TimeZone method is called. Instead the prototype only needs to be checked once when the Record is created.

On the negative side, it introduces some unintuitive behaviour if Calendar.prototype is mutated. First consider the following code:

const p = Temporal.now.plainDateTimeISO();
const z = Temporal.now.zonedDateTimeISO();

p.monthsInYear // => 12
z.monthsInYear // => 12

// Mutate Temporal.Calendar.prototype
Temporal.Calendar.prototype.monthsInYear = () => 13;

// So far, so good:
p.monthsInYear // => 12
z.monthsInYear // => 12

The first weirdness is that p.calendar.monthsInYear(p) will now return 13 and be unequal to p.monthsInYear, but that doesn't seem insurmountable.

It seems correct that Temporal objects derived from other Temporal objects should be constructed with the first object's calendar record:

const z2 = p.toZonedDateTime(Temporal.now.timeZone());
const p2 = z.toPlainDateTime();

z2.monthsInYear // => 12
p2.monthsInYear // => 12

It also seems correct that new Temporal objects will get the new method because they create a new calendar record:

const p3 = Temporal.now.plainDateTimeISO();

p3.monthsInYear // => 13

The above seems like it is all as expected. However, if a new Temporal object with a [[CalendarRecord]] slot could be created by user code, then it cannot preserve the existing calendar record. The following operations will do Gets on all the Calendar.prototype methods and create new calendar records:

const derived1 = p.toPlainYearMonth();
const derived2 = p.with({ hour: 12 });
const derived3 = p.add({ months: 1 });

derived1.monthsInYear // => 13 ⚠
derived2.monthsInYear // => 13 ⚠
derived3.monthsInYear // => 13 ⚠

In none of the above cases is it really obvious that the calendar passes through user code. I can elaborate here if needed, but it's definitely not going to be obvious to the programmer.

I'm leaning towards not having calendar and time zone records because of this. It makes it hard for the programmer to predict whether a derived object will get a new record or the existing record, they have to know which methods are calendar-dependent and which ones aren't. I have thought of a couple of ways to get around this, but neither of them are ideal.

@ljharb @littledan Since the two of you seem to be the ones most in favour of changing it to work like this, could you give your opinion on how you think the advantages outweigh the disadvantage?

[ptomato]

@bmeck You may also be interested in the above, this is the change I was referring to last week when we were talking about what is stored in the internal slots.

[littledan]

Yeah, I'm pretty convinced that the weirdnesses @ptomato mentions are a good reason to not make the suggested change. I'm bothered enough by the mismatch between p.calendar.monthsInYear(p) and p.monthsInYear. This doesn't behave like an options bag because we still preserve its identity to get the calendar out. It'd be quite a waste to have to create a fresh calendar (with a null prototype?) each time the accessor is reached.

[ljharb]

I don't see what's weird about this. The objects I created already are conceptually immutable; if I mutate a builtin, I should expect that objects created before I did so won't necessarily reflect the changes I've made, and might not even be coherent.

[ptomato]

@ljharb So, you would prefer the above but without copying the calendar record when creating any new object? e.g. const p2 = z.toPlainDateTime(); creates a new calendar record instead of copying the one from z, so that the new object does reflect the mutation to Temporal.Calendar.prototype?

if I mutate a builtin, I should expect that objects created before I did so won't necessarily reflect the changes I've made, and might not even be coherent.

I agree that is a consistent way of thinking about it, but I don't agree that it's the most important concern. I think this behaviour is surprising and inconsistent with the mental model of dates and calendars used in Temporal and will cause confusion to developers. I guess you could make a "priority of constituencies" argument about this; it would be better to omit this behaviour and thereby limit the harm to users caused by developers not understanding this quirk, than it would be to include it for optimization reasons or consistency in the spec.

[ljharb]

I don't think it's that important to copy the calendar record - since the position of the proposal is that the Calendar and TimeZone class is not really a class instance, but rather a factory for options bags, nobody should have any expectation that changing a method on the class would affect already-created Temporal instances.

In other words, the only way Calendar and TimeZone modifications should be expected to persist is if in fact they create branded instances, and if users are also required to do so in order to make custom calendars and timezones. Since they're not, that shouldn't be expected.

[littledan]

I can't actualy understand what the semantics are that you're thinking of, @ljharb . It's one thing if we read the fields out of the calendar object and throw it away (as normal for options bags), but here, we keep the calendar around (both for its internal slot and for the .calendar getter). What do you think the .calendar getter should return? And what should happen when you do operations on the date/time object which should return a new object with the same calendar--should it re-Get the fields or use the ones in the internal slots? Should it carry forward the .calendar identity, or have something fresh?

[ljharb]

I’m not really sure; i haven’t thought it through. Perhaps the best thing is not to keep it around at all, but to instead extract everything and store it in an internal record. The .calendar getter could return an instance - even the originally passed object, i suppose - but that doesn’t mean the internal temporal operations should be using that instance every time.

However, if the inconsistency we’re worried about is “things will be potentially weird if someone follows the objectively bad practice of modifying objects they don’t own” then i don’t see the problem.

[littledan]

@ljharb I don't understand what you mean. The internal temporal operations do have to use an instance, to allow, for example, (in the case of built-in 402 calendars) the instance will have the internal slot that says which calendar it is. It's just unclear to me what are the semantics you're proposing.

[ljharb]

The semantics I would expect is that nothing about the internal slots of a Temporal instance should change after the fact - that if I cache original copies of all the prototype methods/getters, they should continue to report the same values for the life of the instance. I'm not entirely certain yet how that would be achieved in a way that's different from the current proposal, but I'm not concerned with consistency when builtin methods/getters are replaced, since robust code would have cached the original methods and used those.

[littledan]

@ljharb It's still unclear to me what change would meet your goals here. My impression, given everything, is that, since we actually do need to keep the instance around to serve as the receiver and to feed into certain other methods, it doesn't make much sense to think of this as an options bag that's Get'd eagerly. Therefore, I prefer the current semantics, and I still don't understand how the details would work out of what you're proposing.

[ljharb]

@littledan if it doesn’t make much sense to think of it as an options bag, then we’re back to “timezone and calendar instances should be brand checked instances, not options bags”. Which one position does Temporal take here? I don’t find it coherent to hold both positions.

[littledan]

@ljharb I don't think that follows. Calendar and Timezone are protocols, along with a correspoinding built-in implementation. It doesn't make sense to brand-check protocols, only the methods within an implementation, as we discussed in the previous TC39 meeting.

[ptomato]

The conclusion at the January plenary was "plain-object time zones and calendars must be supported in Temporal", not "plain-object time zones and calendars should be supported but only if they are thought of as options bags." There is a similarity with options bags here, but also not a 100% correspondence, because with an options bag you don't need to keep the bag object around in order to use the options. So I don't think there's a you-must-have-one-or-the-other argument to be made here.

I spent a lot of time investigating the advantages and disadvantages of this change last week, and I don't recommend making this change. I seem to have reached that opinion on other grounds than @littledan has, but I have reached it nonetheless after investigating in depth.

How do we expect to get this resolved, other than arguing back and forth on GitHub? The Temporal champions haven't discussed this issue as a group, so maybe a good next step could be to put it on the agenda in the next Temporal meeting and try to reach a consensus that represents the Temporal champions as a whole, which we recommend to the plenary.

[littledan]

I doubt that more champion group-scoped efforts are what's needed here, as the strong opinions come from outside of the champion group. We've discussed the object model as a whole for calendars and timezones several times within the champion group, and there didn't seem to be concerns from within that group once we talked it through. I think we should consider this issue settled, and I don't see any changes that should be made before considering the proposal complete and ready for Stage 3. We can hear in plenary if TC39 will give consensus to Stage 3.

If the proposal is blocked for Stage 3 due to this issue, we can have a supplementary call like we did about from lookups, but I honestly don't understand what alternative is being proposed (whereas I did have more of an idea on the from topic).

[ptomato]

We'll take a look at whether any further changes need to be made here as part of the ongoing discussion of #1808, and otherwise close it.

[ptomato]

Per champions meeting of 2023-01-19, no further changes needed as part of #1808.