Stage 2.7 review

[nicolo-ribaudo]

HasPropertyWhichCouldRunUserCode

• In step 4, it should then default to returning false.
• (minor) I would prefer to call this PropertyAccessCouldRunUserCode

I have a larger concern about how this AO assumes a lot abuot the object that is passed to it, without actually requiring anything from its callers.

• Is this AO meant to be safe to call without triggering user code, or is it expected that it could run user code in order to tell if a future access would run user code? (I assume the answer is "yes"?)
• The note says that that AO relies on the fact that ordinary [[GetPrototypeOf]] never runs user code, but the AO itself does not check that the object is an ordinary object.

I think we should also add a check along the lines of:

• If o's [[GetPrototypeOf]] is not one of the internal [[GetPrototypeOf]] methods defined in sections 1.a.i, 3.c.ii, ..., then
  • If o's [[GetPrototypeOf]] is not defined by this specification, return a host-defined Boolean.
  • Return true.

And similar for [[GetOwnProperty]], if we rely on that being side-effect free. For example, the [[GetOwnProperty]] as extended by https://tc39.es/proposal-defer-import-eval/ can run user code.

That list might get hard to maintain, so we might also decide to instead mark whether a [[GetPrototypeOf]]/[[GetOwnProperty]] internal method can trigger side effects or not in the method definition itself. I'm curious to hear what the other @tc39/ecma262-editors think about this.

RequiresDeferredPromiseResolution

• Nit: nest this inside SafePromiseResolve.

PerformPromiseResolution

• Nit: I would find it clearer if called was renamed to thenTiming or thenCallTiming.
• Most of the errors generated by this AO are reported by rejecting promise, but the error step 8.a re-returns a potential throw completion instead. Is it intentional? Or should you handle that completion by potentially rejecting the promise too?

CreateResolvingFunctions

• _{(depending on the changes to PerformPromiseResolution)} It does not return a completion record, so you cannot do call PerformPromiseResolution with ?. You need to handle its completion and properly reject the promise, as the old CreateResolvingFunctions does.

SafePromiseResolve

• In step 3, onFulfilled and onRejected are unused.
• Nit: Remove the acknowledgement of Justin from the note :) We have <emu-note type="editor"> for notes that are just meant as editorial comments about the proposal itself and not to actually be considered as part of the spec text.

[mgaudet]

Thank you very much! I'll work through this next week. In particular I appreciate you highlighting that the side-effect free-ness not being sufficiently well speced.

[mgaudet]

Ok: As I work through this feedback: I'm inclined to address the internal operation feedback by adding the following check to (now renamed) PropertyAccessCouldRunUserCode

If _o_ uses different steps than the ordinary object steps for [[GetPrototypeOf]] or [[GetOwnProperty]], return *true*

Which feels like it's 1) more targeted 2) avoid having to enumerate the "good" cases and instead just assumes that if you're dealing with something exotic, assume it can go badly.

[mgaudet]

Most of the errors generated by this AO are reported by rejecting promise, but the error step 8.a re-returns a potential throw completion instead. Is it intentional? Or should you handle that completion by potentially rejecting the promise too?

So, AFAICT the only way this could concievably reject is if [[Reject]] were to reject for some object, but I'm not sure how that's possible. We could make it an assertion perhaps?

[ljharb]

if "resolution" is an object but its then getter throws, then it'd reject, and there's nothing in the AO preventing that from being the case that i can see.

[mgaudet]

Maybe I have misunderstood, so let me explain what I understood/traced:

1. Nicolo points at PerformPromiseResolution: The problem being that 8.a could return an abrupt completion, without rejecting the promise. I agree, it looks that way. However:
2. PerformPromiseResolveThenable can only return an abrupt completion if thenCallResult is abrupt, and the resolvingFunctions.[[Reject]] returns an abrupt completion.
3. The resolving functions used here are those created by CreateResolvingFunctions. The [[Reject]] steps used here do not ever return an abrupt completion.

So I don't think we can actually get into a situation where we abruptly return. I'd suggest the way to address this would be to mark the call in PerformPromiseResolution with ! normal-completion-assert shorthand.

Having said that, I'm definitely open to having misunderstood the feedback here.

[ljharb]

ahh ok, gotcha. by step 8a, resolution is guaranteed to be an object with a callable then value, at which point PerformPromiseResolveThenable can only complete abruptly if the [[Reject]] function throws, which in this case it can't ever do.

That suggests to me that both PerformPromiseResolveThenable step 4.a and PerformPromiseResolution step 8.a should use ! instead of ??

[nicolo-ribaudo]

Ok: As I work through this feedback: I'm inclined to address the internal operation feedback by adding the following check to (now renamed) PropertyAccessCouldRunUserCode

If _o_ uses different steps than the ordinary object steps for [[GetPrototypeOf]] or [[GetOwnProperty]], return *true*

Which feels like it's 1) more targeted 2) avoid having to enumerate the "good" cases and instead just assumes that if you're dealing with something exotic, assume it can go badly.

That seems ok, just a nit about wording:

1. If _o_'s [[GetPrototypeOf]] or [[GetOwnProperty]] essential internal methods are not the ordinary object ones (<emu-xref href="#sec-ordinary-object-internal-methods-and-internal-slots"></emu-xref>), ...

Note that this would mean that it marks as "potentially having side effets" these object types:

• ECMA-262
  • List-like (Arrays, TypedArrays, Arguments, String)
  • Module namespaces (good, because of import defer)
  • Proxies (good, obviously)
• Web (WebIDL)
  • Objects that support named property getters, i.e. getters with arbitrary property names (document, DOMStringMap, ...)
  • Legacy platform objects

As an alternative we could simplify the check to be if _o_ is an exotic object, return true, which would however also include:

• Bound functions
• Immutable prototype exotic objects (Object.prototype, Web global objects)

---

Seems reasonable :)

So I don't think we can actually get into a situation where we abruptly return. I'd suggest the way to address this would be to mark the call in PerformPromiseResolution with ! normal-completion-assert shorthand.