-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
delegate review: Calendar and TimeZone should be brand-checked, not normal, objects #1294
Comments
|
I'd love to hear more about that design decision; given that |
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. |
Also remove %Temporal.Calendar.prototype.toString% intrinsic. These are not needed because toString() is a required method of the Calendar protocol, so there should always be a toString() there to call. See: #1294
Also remove %Temporal.TimeZone.prototype.toString% intrinsic. Similarly to the previous commit, these are not needed because toString() is a required method of the TimeZone protocol, so there should always be a toString() there to call. See: #1294
Also remove %Temporal.Calendar.prototype.toString% intrinsic. These are not needed because toString() is a required method of the Calendar protocol, so there should always be a toString() there to call. See: #1294
Also remove %Temporal.TimeZone.prototype.toString% intrinsic. Similarly to the previous commit, these are not needed because toString() is a required method of the TimeZone protocol, so there should always be a toString() there to call. See: #1294
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 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 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. |
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 |
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). |
Here is some context on why strings are used for the protocol method names and not symbols: |
Worth pointing out that in the linked notes:
so this isn't a new opinion i'm bringing up :-) |
@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:
|
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. |
Do you want non-subclasses to be able to participate in the Calendar and TimeZone protocols? If so, how should they do it? |
No, i do not want that. Only subclasses should be considered true calendars, whether they use built-in methods or override them all. |
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 |
I am not sure to understand all the stakes that are discussed here, and my opinion only reflects a calendar's author experience. |
@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 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 |
The convention in 262 for reading eagerly is in all of the Promise combinators, where 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. |
This introduces a bunch of abstract operations, and brings the existing ones in line with each other, to consistently use GetMethod (which throws if the method isn't callable) to get the calendar methods off of the calendar object. Any methods for which we have agreed that Temporal core (and not the calendars) should validate the return value, are now also consistently validated in these abstract operations. Bring the spec text in line with this as well. Some of these abstract operations existed, but weren't consistently used. (The exception to this is where the method was fetched only once from the calendar object to be called multiple times. This will be resolved one way or the other in issue #1294.)
This introduces a bunch of abstract operations, and brings the existing ones in line with each other, to consistently use GetMethod (which throws if the method isn't callable) to get the calendar methods off of the calendar object. Any methods for which we have agreed that Temporal core (and not the calendars) should validate the return value, are now also consistently validated in these abstract operations. Bring the spec text in line with this as well. Some of these abstract operations existed, but weren't consistently used. (The exception to this is where the method was fetched only once from the calendar object to be called multiple times. This will be resolved one way or the other in issue #1294.)
When constructing a new Temporal object with a calendar object, eagerly get all the calendar methods, and save them in a Record, which goes into the Temporal object's [[CalendarRecord]] internal slot. _For internal use of the intrinsic constructors only_, in the polyfill constructors accept a calendar record object instead of a calendar object. This has no observable effect on user code, because user code can never obtain a calendar record object. Still to do: - spec text - inconsistency introduced between e.g. the calendar records in the objects returned from pdt.toPlainDate() (same as pdt) and pdt.with() (different from pdt if Calendar.prototype was mutated since pdt was created) See: #1294
Similar to calendar records, when constructing a new ZonedDateTime, eagerly get all the required time zone methods, and save them in a Record, which goes into the ZonedDateTime's [[TimeZoneRecord]] internal slot. _For internal use of the intrinsic ZonedDateTime constructor only_, in the polyfill the constructor accepts a time zone record object instead of a Temporal.TimeZone object. This has no observable effect on user code, because user code can never obtain a time zone record object. Still to do: - spec text - same problem as calendar records where the time zone record is lost when calling through user code - when calling a TimeZone method directly, we create a new time zone record first, which Gets all the methods. Maybe this isn't needed unless we're calling the method from ZonedDateTime where we have the record? See: #1294
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 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? |
@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. |
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 |
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. |
@ljharb So, you would prefer the above but without copying the calendar record when creating any new object? e.g.
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. |
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. |
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 |
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. |
@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. |
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. |
@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. |
@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. |
@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. |
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. |
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 |
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. |
Per champions meeting of 2023-01-19, no further changes needed as part of #1808. |
from #1275 (comment):
The text was updated successfully, but these errors were encountered: