delegate review: observable "from" lookups

[ljharb]

from #1275 (comment):

https://tc39.es/proposal-temporal/#sec-temporal-timezonefrom observably looks up the from property on Temporal.TimeZone. This doesn't enable any subclassing nor does it make builtins robust against userland modification. The undefined fallback only protects against deletion, not replacement. Also https://tc39.es/proposal-temporal/#sec-temporal-calendarfrom, and presumably a bunch of other places.

[Ms2ger]

• The observable lookup is intentional - it's the extension point for users to create custom time zones / calendars for a given identifier.
• I don't feel strongly about the undefined checks. It seems slightly more robust, but probably not a likely error case.

[ljharb]

It's not an extension point if it's looking up "from" directly on the intrinsic, because that wouldn't ever reach a "from" method on a subclass.

[Ms2ger]

The extension point is replacing the from method on the intrinsic. How would we find the subclass to get the from off?

[ljharb]

I strongly object to any API designed around modifying builtins, which is a decades-old bad practice and something we should never be encouraging.

At this point in the AO, we wouldn't be able to. However, typically the receiver is used to look up the appropriate constructor, and that information is passed around all the AOs to be used in places like this.

[Ms2ger]

To help me understand, could you clarify an approach where "the receiver is used to look up the appropriate constructor" in, for example, new Temporal.PlainDate(2000, 1, 1, "customCalendarIdentifier")?

[ptomato]

Background for this is in #294 and https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2020-04-16.md#294-idtocalendaridtotimezone

[ljharb]

a custom calendar identifier alone wouldn’t work there all, nor should it. I’d expect instead to have to do: new Temporal.PlainDate(2000, 1, 1, new CustomCalendar()). String identifiers are only for known identifiers, and since there’s no mutable registry in the proposal, there’s no way for a custom calendar to make its identifier known.

[ljharb]

If that’s a desired goal, then there should be some kind of explicit stateful static method that takes an identifier and a constructor, and “registers” the identifier so that the base Temporal classes can leverage it. However, i suspect such a stateful registry would have other challenges, and would suggest it be part of a follow-on proposal rather than this one.

[ptomato]

One consideration for this in particular, was that it's impossible to parse new Temporal.PlainDate(2000, 1, 1, new CustomCalendar()).toString() back into a Temporal object, unless there is either an extension point or a stateful registry.

[ljharb]

With that, just like every other serialization method, you have to “know” what the format is - to register it in the first place.

(as for any kind of auto-vivication of user code, as i recall, that’s the cause of two of the larger CVEs in the Ruby ecosystem, discussed here, and is presumably not a can of worms we wish to open)

[ptomato]

We discussed this in the 2021-01-21 champions meeting. There are a few solutions that we considered back when we discussed #294:

1. Calendar and TimeZone deserialization funnels down to a single point, TimeZone.from()/Calendar.from(), which can be overridden to customize (in other words, the status quo)
   • alternatively, it could be an even more narrowly-defined point such as Calendar.fromName()/TimeZone.fromName() that only handles an ID string
2. Deserialization has no user-exposed convergence point. To customize, all deserialization entry points must be overridden. (Because Temporal methods accept string arguments anywhere a Temporal object is expected, this means a lot of entry points)
3. Deserialization is a lookup on a user-exposed object or map such as Calendar.registry/TimeZone.registry. To customize, manipulate or override that.

None of these are ideal, but we think it's a good goal for Temporal to allow deserialization of custom calendars, and we are convinced that option 1 is the least bad one. Specifically, we think it's better than doing nothing (Option 2), among other reasons because people are going to do it anyway, and with Option 2 it's more likely they'll do it badly. Option 3 we don't think is acceptable at all.

It's not clear that Option 1 could be introduced in a follow up proposal while remaining web compatible.

[bakkot]

I strongly agree with Jordan here. Introducing a new global namespace (which this does) strikes me as a bad idea in itself. Introducing one for which the intended method of coordination is replacing a built-in seems even worse.

I'm not convinced that it makes sense to support deserialization with a custom calendar without providing the calendar to the method doing the deserialization. At least, if the tradeoff is between "have global state" and "cannot deserialize with custom calendars", I would very strongly favor the latter.

I assume the option of "in the case of custom calendars, require the user to deserialize ahead of time using a method explicitly intended for deserialization and which supports custom calendars as an addition argument" was discussed. Is there a reason that's not viable?

[ptomato]

See #294 for consideration of that option and https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2020-04-16.md#294-idtocalendaridtotimezone for further discussion. Passing in custom calendars as an additional argument doesn't allow polyfilling of time zones and calendars not present in the host's locale data, and therefore comes down to the above option 2 in practice.

[bakkot]

Polyfilling is a very different use case than user-defined custom calendars. It's accepted practice to mutate globals for polyfills - indeed, that's the only way polyfilling is possible, as a rule. That doesn't mean we should encourage it for non-polyfill code.

Let me try to expand on the important difference here. For polyfills, the usual state of affairs is that you have only a single set of polyfills across your entire application. Moreover, the set of behavior to be polyfilled is globally agreed upon (as being the latest specified behavior). So even if you have two different polyfills running, they're probably not going to have orthogonal behavior: at worst you might end up with a slightly-less-recent version than you'd hoped. So the coordination problem is relatively narrowly scoped. (Not to say it doesn't cause issues, even then.)

By contrast, a global namespace which is expected to be written to typical application code - that is, non-polyfill code - requires coordination across the entire application. If you and I both publish libraries (or internal components for use on our multi-team webapp) which define a "test" timezone ID, and patch them onto from, there's no reason to expect us to be defining the same behavior. It then becomes impossible to use our libraries on the same webpage correctly. Worse still, the incorrect behavior will almost certainly manifest as a bug in one of our libraries - some internal deserialization that ends up unexpectedly wrong - rather than anything which suggests the problem is the use of both libraries in conjunction. This is a maintainership nightmare for both the application itself and for the libraries.

In modern code, it is widely accepted that the only code for which it is acceptable to override builtins is in polyfills. This prevents the coordination problem arising from having user code fighting over a shared namespace. As such, even if the cost of option 2 is that polyfills need to patch a bunch of different builtins, I continue to feel strongly that this is better than suggesting to users that they should patch builtins themselves.

[gibson042]

Multiple use cases have been identified as important by various parties (the first two have a "polyfill" flavor to them, while the third does not):

• Add custom calendars and/or time zones
• Patch built-in calendars and/or time zones with "better" data and behavior
• Limit access of virtualized code to only specific calendars and/or time zones, possibly with overridden data/behavior

Does this issue relate to the validity of those use cases, or to the mechanism(s) by which they should be supported? Assuming the latter, I don't have a fundamental problem with funneling logic to a single overrideable value, although I do agree that the inherent shadow linkage between built-ins doesn't quite work, e.g.

Temporal.TimeZone = class TimeZone extends Temporal.TimeZone {
	static from() {
		throw new Error("Why am I not invoked?");
	}
}
Temporal.ZonedDateTime.from("2021-02-11T18:22:42[Etc/UTC]") // where's my error?

[bakkot]

Does this issue relate to the validity of those use cases, or to the mechanism(s) by which they should be supported?

The mechanism.

In particular, I wouldn't regard adding custom calendars or time zones to be a polyfill-like use case, and am therefore opposed to global state and overriding built-ins as the mechanism by which that use-case is supported. I don't have strong feelings about that use case in general; I'd be fine with other means of supporting it as long as those means avoid global state and, especially, telling users that they should override a builtin.

[ljharb]

Additionally, the ergonomics of polyfilling or locking down aren’t nearly as important as the mere achievability of those goals, and “you have to override almost all the methods” is completely fine as a requirement.

[ptomato]

In particular, I wouldn't regard adding custom calendars or time zones to be a polyfill-like use case

@bakkot I suspect this might be the root of the disagreement between you and the Temporal champions.

@ljharb @bakkot We discussed this morning that we'd like to dedicate next week's Temporal meeting to this topic and invite both of you to participate, since it doesn't seem this thread is changing anyone's mind in its current form. I'll create an issue on the Reflector with details and to invite other delegates who may be interested.

[bakkot]

Sure, I'll aim to be there.

[syg]

I'll try to be there for the second half of the call. My concerns largely align with @bakkot and @ljharb. I find the particular mechanics of the extension point problematic. To wit, that it is all about overriding a particular method of a built-in object (e.g. GetMethod(%Temporal.Calendar%, "from")) and not replacing the built-in object wholesale (e.g. looking up Temporal.Calendar.from) is very weird to me.

[ptomato]

@syg Have I understand correctly that your concern would be addressed by Temporal effectively doing GetMethod(Get(Get(globalThis, "Temporal"), "Calendar"), "from") when it needs to convert a string ID to a Temporal.Calendar object, rather than GetMethod(%Temporal.Calendar%, "from")?

[syg]

@ptomato I personally prefer no extensibility at all for the Temporal classes, but won't block on it. Still trying to better understand the motivations. If extensibility is non-negotiable, I do find GetMethod(Get(Get(globalThis, "Temporal"), "Calendar"), "from") less weird, yes. That's the full monkeypatching route and is more easily explained. The current state is like, the classes themselves aren't monkeypatchable, but specific static methods are. Why draw that line?

[ptomato]

@syg OK, thanks. The motivations are summarized in #1293 (comment) and #1293 (comment) and the meeting minutes in the second half of this item, but hopefully @gibson042 can answer your questions tomorrow.

I'm not sure why it was originally done with GetMethod(%Temporal.Calendar%, "from") and not GetMethod(Get(Get(globalThis, "Temporal"), "Calendar"), "from") but my guess is that no-one feels strongly about drawing that line.

[ljharb]

I do not think looking up the current value of “Temporal” in the global, nor any of the Temporal types, makes any sense whatsoever. I should always be able to cache a builtin in a variable and then delete it off the global entirely, and nothing should work differently.

[bakkot]

Yeah, that's a good point: the current approach, as well as the lookup-on-global approach, would make it impossible for early-running code to be defensive against late-running code mucking with stuff (short of preemptively making from property non-configurable & non-writable, I guess), which seems bad.

Though I guess that's just a specific manifestation of the more general "global state is bad" critique.

[ptomato]

I do not think looking up the current value of “Temporal” in the global, nor any of the Temporal types, makes any sense whatsoever.

@ljharb If you read the meeting notes — you'll see that the champions are in rough agreement that this is the least bad of several bad options. You can read there that nobody thinks the current approach is great, but we think it's better than the alternatives. It's a bit rich to pronounce that "it makes no sense whatsoever" when clearly several people have spent time and effort thinking about it. I've tried to document carefully and exactly in the meeting notes, the reasons why people do think it makes sense in this particular case. In order to ensure a productive discussion tomorrow, I'd appreciate if you could make a bit of effort to understand our rationale, even if you don't agree with it — as I think we have done with yours.

[ljharb]

@ptomato i did not mean to imply that, but what I’m describing is a security/encapsulation axiom, and i am highly invested in ensuring it is never violated, as are other delegates. I will certainly try to understand the rationale, but if this is the cost, then I’d rather completely forgo extensibility - to me that is far less important.

That said, I’m quite unconvinced that observable lookups like this are a necessary cost to achieve the goals, and I’m hoping in the call tomorrow to better understand those goals.

[ptomato]

In anticipation of people reviewing the minutes, here's my summary of the Temporal meeting on this topic on 2021-03-03:

• Polyfillability of custom calendars and time zones in order to support geopolitical decisions made on short notice (delegate review: observable "from" lookups #1293 (comment)) is a valid use case, but ergonomics of this use case should be prioritized lower than early-running code being able to defend against late-running code (delegate review: observable "from" lookups #1293 (comment)).
• We will land a patch before the TC39 plenary removing the observable lookups.
• An extra option to from() to pass in a "local registry" object as described in Semantics of idToCalendar function in Temporal.Date.from() #294 would be a way to mitigate the ergonomics hit.

[ptomato]

Now for my personal opinion, I'm not sure that I'll be able to get a "local registry" object finished before the plenary. Shu suggested in the meeting to keep it as an additional diff to the spec text, that could be reviewed separately. I'm guessing it could be possible to mention in the March meeting that we intend to add this local registry, and get consensus in April to merge a PR for it?

[ljharb]

It seems totally reasonable to me that plenary might grant stage 3 in this meeting, with the understanding that an additive piece might also become part of it in the next meeting.