AsyncContext.{Snapshot,Variable}.prototype.run leave no room to specify receiver

[gibson042]

(originally posted by @gibson042 in https://github.com/tc39/proposal-promise-try/issues/15#issuecomment-2045965423)

I have reservations about Promise.try AsyncContext.{Snapshot,Variable}.prototype.run accepting variadic arguments that leave no room for ever specifying a receiver—setting it apart from Function.prototype.{apply,call} and Reflect.apply.

[bakkot]

Can you say more about what exactly those reservations are? What use case are you worried about ruling out here?

[gibson042]

Well, it is conceivable that the future evolution of the language will lead to a place where heavy use of receivers is commonplace. The existing primary indirect invocation facilities—Function.prototype.apply, Function.prototype.call, and Reflect.apply—all provide a way to specify receiver, and for that matter so do others such as callback-accepting Array.prototype methods (every, filter, map, etc.). They are therefore prepared for such a future, but the run methods proposed here are not and never can be because there's nowhere to put a thisArg parameter... authors would be forced to instead jump through hoops like asyncSnapshot.run((...args) => Reflect.apply($callback, $receiver, args)). So we've got 1) extra cognitive burden to track which argument-propagating facilities also propagate a receiver and which do not, and 2) missing functionality.

From my perspective, the correct framing is not "what is lost by omitting a thisArg parameter?", but rather "what benefits justify its omission as something other than a gratuitous deviation?".

[jridgewell]

A few points:

• Ecosystem precedent, Node's AsyncLocalStorage.p.run similarly takes variadic args without a thisArg.
• I have never once used an Array method thisArg, and they're not variadics.
• .apply()/.call() aren't run-of-the-mill functions, they're the special functions that expose a low-level ability to devs.

I don't think it's common to need to change the context, and when an object's context needs to be carried over to a method I'd much rather to see a dev write run(() => obj.method()) than run(obj.method, obj).

[Qard]

AsyncLocalStorage not taking a receiver is generally considered to be a mistake, which is why other related things like diagnostics_channel do take receivers.

[jridgewell]

Is there a thread on that?

[jridgewell]

We discussed this in today's meeting. No one voiced support for adding thisArg to run() methods, and if we get pushback from the committee we'd rather remove the variadic args entirely than add it.

[Qard]

So the main reason for it is to enable store.run(...) to universally be able to replace a call without additional closures. For example, foo(bar, bar) can directly become store.run(context, foo, bar, bar). With instance methods we need to do something like store.run(context, instance.method.bind(instance), ...) which is a bit cumbersome but honestly not a huge deal.

The one downside to not doing it though is that optimizations could likely be made to combine the call/apply that needs to happen with the context change. Could also probably do some magic to avoid dealing with iterators since arguments are passed positionally, just offset by two from the context object and function reference--might be able to do something like forwarding an offset pointer as args into the given function or things like that.

My thinking with context runs has always been to treat it like a special case of function call/apply.

[jridgewell]

In cases where code wants to invoke a method on an object, I'd much rather see run(()=>foo.bar()) than run(foo.bar, foo). And in cases where you're changing the context dynamically, I'd again much rather see run(() => bar.call(foo)).

I was the only one supportive of using variadic arg style for run, because I practice static functions as my default coding style. Everyone else just said to use an arrow closure.

[bakkot]

If this does come up, I'd prefer to address it by adding a runMethod(foo.bar, foo, ...args) helper rather than by changing the signature of run.

[andreubotella]

My thinking with context runs has always been to treat it like a special case of function call/apply.

For what's worth, my feeling is that most non-APM uses of AsyncContext don't see run as a special case of function call/apply by any means. They'll use run with an arrow function, seeing it as a logical part of the caller function, and would prefer to use something like a disposable scope instead, which is why #60 is a topic of discussion.

[Qard]

Yes, it is the case that often closures will be used by users, but they're also expensive if the run happens in a hot-path so I think we should consider offering tools that are at least capable of avoiding closure use.

Having separate methods is perfectly reasonable to me, so long as the capability exists and we have an interface which can be optimized well for those that care about performance.

[jridgewell]

Some discussion happened in Matrix yesterday. The other delegates agree with my opinions that supporting thisArg ins't desirable. A few comments to highlight:

maybe because most of the time you don't need to specify receiver, and when you do, you have call-bound call to bail you out? – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L23

I think "call-bound call" (const call = Function.call.bind(Function.call); call(obj.method, obj, arg1, arg2)) is a little too much setup to manually, but if we get a demethodize or static helper equivalent then this could be solved orthogonally:

// Imagine this already exists
Reflect.call = Function.call.bind(Function.call);

snapshot.run(Reflect.call, obj.method, obj, arg1, arg2);

Function.prototype.apply/call and Reflect.apply are special helpers, which are very rarely sensible precedent to use for other methods – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L24

Very much agree.

+1 to this, especially because of language learnability. this/receivers aren't things that come early when learning JS, especially because of how they differ from the equivalents in other languages. Even if Promise.try and AsyncContext are advanced use cases, language learning is non-linear. – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L39

This is a good point that I hadn't fully considered. I like seeing () => obj.method() because it's easier for me to understand, but this might be really important for beginners so that they don't get confused about this binding.

[Qard]

Passing in Reflect.call seems reasonable to me, so long as we keep the argument pass-through so we can avoid a closure wrap or a bind-and-then-call when we care about performance.

My primary concern is that run be fast enough that we can use it liberally. Us APM vendors are going to be calling this all the time to store our current spans and other observability data so performance is absolutely critical to us.

Random side thought: could a decorator transform any form of existing call (bare function, method call, etc.) in a way that V8 could apply the context around it without additional function bind/call/apply? That could be another interesting way to wrap an existing call in a context with minimal extra cost. 🤔

[gibson042]

Function.prototype.apply/call and Reflect.apply are special helpers, which are very rarely sensible precedent to use for other methods – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L24

For the record, color me convinced.