Stage 3 Reviewers

[leobalter]

Tracking issue for spec reviews for advancement to Stage 3.

• Richard Gibson @gibson042
• Yulia Startsev @codehag
• Daniel Ehrenberg @littledan
• Jordan Harband @ljharb
• (Editor) Michael Ficarra @michaelficarra
• (Editor) Kevin Gibbons @bakkot
• (Editor) Shu-yu Guo @syg

For this specific review I'd to consider the current spec minus the upcoming changes for the module graph aspect to be resolved.

[ljharb]

Here's some initial thoughts:

In https://tc39.es/proposal-realms/#sec-wrapped-function-exotic-objects-call-thisargument-argumentslist: why do you need to wrap the completion record value in a new record, as opposed to just passing on the completion record (when its type is normal or return)?

Related, why are you passing a completion record to GetWrappedValue? It seems like in the "if" branch, you could Return ? GetWrappedValue(callerRealm, result) and it'd do what you want via the implicit .[[Value]] on non-abrupt completion records?

Additionally, since the only possible types of completion record from the invocation is "abrupt" or "normal or return", it kind of seems like you could branch on whether it's abrupt, and not have to explicitly mention normal/return.

---

@bakkot StringToCodePoints doesn't seem to be auto-linking to https://tc39.es/ecma262/#sec-stringtocodepoints

---

Can eval, Function, and https://tc39.es/proposal-realms/#sec-performrealmeval be made to maximally share steps/AOs? It seems like it'd be helpful if they could all be expressed in terms of the same operation(s).

---

Should the name of the [[Realm]] slot be the same on functions and Realm instances? Perhaps it should, if it's holding the same types of values, but perhaps it'd be better to differentiate the two?

---

"Perform" is typically the verb associated with calling an AO and ignoring any non-abrupt return value. Can https://tc39.es/proposal-realms/#sec-performrealmimportvalue start with something besides "Perform"?

---

I'm not sure what all the "Extensible web:" phrases are in the notes - we don't have this anywhere in the spec, and it doesn't link anywhere. What's the purpose of that note prefix?

---

In https://tc39.es/proposal-realms/#sec-hostimportmodulebindingdynamically: the "failure path" sentence reads oddly to me, and I'm not sure what it means exactly. Also, for the "success path" case, should the completion record's type be constrained to be "normal"?

[leobalter]

@ljharb Thanks for the review! I opened #306 with detailed comments.

[syg]

First round review as follows.

• In the wrapped exotics' [[Call]]:

  • In Step 5, Lists are enumerated by elements, not indices, so key would be the argument itself in that loop. I'd also prefer a new List be made instead of mutating the existing one.
  • o is unbound
  • 'associate to' -> 'associated with'

• In PerformRealmEval:

  • inFunction, inMethod, and inDerivedConstructor are always false, that's wrong.
  • In Step 14, why is the running context suspended? Isn't this synchronous?

• In RealmImportValue:

  • In Step 10, why is the running context suspended?

• In Realm instances

  • I don't understand why it needs to stash an [[ExecutionContext]], which seems to be used for import. I was expecting a fresh execution context be made every time we call into the user realm, since everything is synchronous.

• Would like to see the HTML integration text for HostImportModuleBindingDynamically

[codehag]

Also doing a round of reviews, made a pr with the smaller stuff.

I think I largely noticed the same things as Shu. One issue is that with, inFunction, inMethod, and inDerivedConstructor are always false, but I not sure how they are intended to be set from the spec text.

Regarding suspending execution contexts: I think if it is possible to use push/pop instead it is preferable (currently having this discussion) Hopefully we can get general guidance on that. I think doing what shu suggested is a good idea, rather use push/pop for sync contexts.

In 3.1.1 the two realms might be the same realm, should an assertion be added to prevent that?

in 3.2.1 is there a case where the Realm function can be called with arguments? The text suggests that it can, but I didn't find it anywhere. If it can't, maybe reword it to "The abstract operation Realm takes no arguments." etc.

in 3.6.2 -- I found this segment difficult to read. It might benefit from another pass. For example, is the discrepancy in time between the success and failure path intentional?

[leobalter]

@syg:

In (PerformRealmEval's) Step 14, why is the running context suspended? Isn't this synchronous?

This synchronously suspended for the following steps setting evalContext to be stacked in the running execution context, see step 21 (Push evalContext onto the execution context stack; evalContext is now the running execution context.) and it's resumed in step 25.

This behavior matches steps 18 to 31 from PerformEval.

The similar behavior happens in RealmImportValue to shift the running execution context of the Host import hook to the target realm.

---

I don't understand why it needs to stash an [[ExecutionContext]], which seems to be used for import. I was expecting a fresh execution context be made every time we call into the user realm, since everything is synchronous.

As the Editor's note says, we might try an alternative, but I hope this can be an implementation detail for stage 3 defining how to better fit it in an eventual PR for ECMA-262. WDYT?

---

Would like to see the HTML integration text for HostImportModuleBindingDynamically

We have plans to revert HostImportModuleBindingDynamically to HostImportModuleDynamically and pick the binding value from it. This would remove the requirement for the HTML integration text.

---

I'm answering @codehag feedback in a separate comment.

[leobalter]

@codehag:

I think I largely noticed the same things as Shu. One issue is that with, inFunction, inMethod, and inDerivedConstructor are always false, but I not sure how they are intended to be set from the spec text.

Thanks! I believe those were originally intended to be used in direct evals from the PerformEval. We imported them by mistake. I'm removing them here: https://github.com/tc39/proposal-realms/pull/309/files#diff-181371b08d71216599b0acccbaabd03c306da6de142ea6275c2135810999805aR145

Regarding suspending execution contexts: I think if it is possible to use push/pop instead it is preferable (currently having this discussion) Hopefully we can get general guidance on that. I think doing what shu suggested is a good idea, rather use push/pop for sync contexts.

I hope this can be something we address after Stage 3, but in general I support the spec text improvement.

In 3.1.1 the two realms might be the same realm, should an assertion be added to prevent that?

I don't think it's possible for the two realm being the same with the current proposed spec.

PerformRealmEval only happens from Realm.prototype.evaluate:

4. Let callerRealm be the current Realm Record.
5. Let evalRealm be O.[[Realm]].
6. Return ? PerformRealmEval(sourceText, callerRealm, evalRealm).

As the API can't transfer non callable objects across realms, it can't access any O.[[Realm]] that matches the current Realm.

Also, the functions are not unwrapped... A realm could return a function that is sent and rewrapped to the function's original realm:

const r = new Realm();

const wrapper = realm.evaluate('fn => fn');

const originalFn = x = x * 2;
const wrappedFn = wrapper(originalFn); // originalFn is wrapped in the realm r

// and wrapped again in the return of wrapper

originalFn !== wrappedFn; // true, the function is never unwrapped.

// wrappedFn is a wrapping of the realm r's received `fn`, and `fn` is a wrapped function of `originalFn`.

---

in 3.2.1 is there a case where the Realm function can be called with arguments? The text suggests that it can, but I didn't find it anywhere. If it can't, maybe reword it to "The abstract operation Realm takes no arguments." etc.

I just cleaned up the constructor text in 067c381. There are some consistency challenges here: the Realm constructor is not exactly an internal abstraction as the text for "The abstract operation" is seen on those only. The previous text saying "called with no arguments" is seen often in Annex B methods (e.g. String.prototype.blink) and the commit just reuses a similar prose to other methods with no parameters such as String.prototype.toString.

WDYT?

---

in 3.6.2 -- I found this segment difficult to read. It might benefit from another pass. For example, is the discrepancy in time between the success and failure path intentional?

The current HostImportModuleBindingDynamically operates as an extension to HostImportModuleDynamically, in addition to those bullet points for successful and failure paths.

This abstraction is indeed difficult to read and still confuses me a bit, that's why we are considering refactoring this to just reuse the original hook and fetch the requested value without adding a new hook.

Ideally, this desired abstraction should operate on top of HostImportModuleDynamically, and if this one succeeds, it tries to get the property identified from _exportedName_, if that's undefined, it should lead to a failure path, or a promise rejection.

I'll circle back when I have updates on this part.

[chris-kruining]

@leobalter

In 3.1.1 the two realms might be the same realm, should an assertion be added to prevent that?

I don't think it's possible for the two realm being the same with the current proposed spec.

PerformRealmEval only happens from Realm.prototype.evaluate:

4. Let callerRealm be the current Realm Record.
5. Let evalRealm be O.[[Realm]].
6. Return ? PerformRealmEval(sourceText, callerRealm, evalRealm).
As the API can't transfer non callable objects across realms, it can't access any O.[[Realm]] that matches the current Realm.

I've never worked on spec before, so please correct me if I'm wrong. If the realms currently can't be the same, why not add it to the spec that they shouldn't be the same? Wouldn't this extra explicitness make the spec more robust and maybe prevent weird situations when this spec is changed/expanded in lets say 2-3 years time?

My comment is not meant as criticism, but rather as an inquiry as to the inner workings of speccing ES :D

[leobalter]

From this perspective, I probably misinterpreted @codehag's feedback and maybe this is just about having a "assert" line rather than an actual assertion that would prevent that by actually comparing both realms and throwing an error if they're equal.

I'm fine adding the assert line if that's preferred.

[leobalter]

We extensively discussed these objections. Some of the suggestions are also hard objections from other delegates. I wonder if this can be considered in a path this proposal could in fact advance, ever.

[Jack-Works]

We extensively discussed these objections. Some of the suggestions are also hard objections from other delegates. I wonder if this can be considered in a path this proposal could in fact advance, ever.

Pick utils from Web API into the ES API so they can be used everywhere (#284 (comment))

At least this one is the safest one we can choose.

[leobalter]

Pick utils from Web API into the ES API so they can be used everywhere (#284 (comment))

There is an answer for that comment. The current ES hook does not prevent this from happening, but we need to finish the HTML integration to have a proper list of properties that are known to be added. After that, we can have a new discussion at TC39 if we should add these names to ES and what it means.

The current proposal does not request demand on things we desire to discuss in the HTML integration as the proper venue. Our ES requirements are already set in this draft.

[syg]

This synchronously suspended for the following steps setting evalContext to be stacked in the running execution context, see step 21 (Push evalContext onto the execution context stack; evalContext is now the running execution context.) and it's resumed in step 25.

This behavior matches steps 18 to 31 from PerformEval.

The similar behavior happens in RealmImportValue to shift the running execution context of the Host import hook to the target realm.

Oh wow, I've been misunderstanding "Suspend" all these years, thanks for pointing out what's going on in PerformEval. I thought Suspend was purely for actual suspension for things like generators, but it is in fact also used to mark which execution context is the running one. I thought "running" was purely distinguished by being top of the execution context stack.

This is kind of confusing, I'll think about improving this editorially.

[caridy]

@Jack-Works this issue is for stage 3 editorial review feedback on the current proposal, not for objections about the proposal itself, I will recommend that you comment on the original issue if you haven't done that yet.

[leobalter]

@syg @codehag #311 should resolve feedback and concerns regarding the HostImportModuleBindingDynamically with a simplified walk through, PTAL

[syg]

Editorially, the spec text mostly looks okay to me modulo the following:

• There are steps that say something like "If runningContext is not already suspended, suspend runningContext." What's the situation where the running context can already be suspended?
• I'm still not clear on why Realm instances need an [[ExecutionContext]] stashed away. Can it be created fresh each time it's needed? That shouldn't be observable, right?
• ValidateRealmObject needs an aoid presumably for the autolinking.

[littledan]

The spec LGTM; my main concern was addressed in #311

[gibson042]

I found some issues with the spec text, but they all seem fairly easy to fix:

1. The "Internal Slots of Wrapped Function Exotic Objects" table defines slot [[WrappedTargetFunction]] as holding "the callable object from the other Realm", despite the definition of wrapped function exotic object not being limited to cross-realm scenarios (e.g., "is an exotic object that wraps a callable object from another Realm.").
2. Wrapped Function Exotic Objects [[Call]] step 4 invokes GetFunctionRealm on the wrapped function exotic object, which will return the current Realm Record because wrapped function exotic objects have no [[Realm]] slot and are neither bound functions nor proxies. Is that intentional, and if so then why jump into GetFunctionRealm at all?
3. PerformRealmEval step 19 is missing the privateEnv argument in its invocation of EvalDeclarationInstantiation.
4. Does PerformRealmEval intentionally skip the check for "If result.[[Type]] is normal and result.[[Value]] is empty" that appears in PerformEval after evaluation? What are the consequences of that omission?
5. Nit: in PerformRealmEval step 23, I think throw a newly created TypeError object associate to the _callerRealm_ should be phrased like throw a *TypeError* exception (optionally with a note to clarify its associated realm).
6. RealmImportValue makes a type assertion about every argument except evalContext, is that intentional?
7. Are you sure you want ExportGetter functions to throw TypeError when the requested export is not provided? I believe importing an unexported name is a SyntaxError: https://jsfiddle.net/531ntwq9/
8. Nit: in Realm() step 2, there should be asterisks around "%Realm.prototype%" (and realm would probably be a better name for the value than O in it and most of the other algorithms).
9. Nit: There should also be asterisks around all other literal String values, such as the second argument to CreateBuiltinFunction and the "Realm" in Realm.prototype[@@toStringTag].
10. Nit: in HostInitializeSyntheticRealm, "provide platform capabilities with no authority to cause side effects…" is worded a bit strangely.
11. Nit: I think there should be a space between the parentheses in Realm () and before the left parenthesis in ValidateRealmObject( _argument_ ).
12. Question: Why does Realm.prototype.evaluate require its argument to be a String while Realm.prototype.importValue performs type coercion?

[caridy]

great feedback @gibson042, inline...

1. The "Internal Slots of Wrapped Function Exotic Objects" table defines slot [[WrappedTargetFunction]] as holding "the callable object from the other Realm", despite the definition of wrapped function exotic object not being limited to cross-realm scenarios (e.g., "is an exotic object that wraps a callable object from another Realm.").

That is intentional, to have room for implementers to do optimizations. E.g.: if a realm calls a Wrapped Function Exotic Objects, and passes as an argument a regular function, which will be wrapped on the way in, if that Wrapped Function Exotic Objects comes back to the realm via another call, there is nothing that prevent the implements to optimize the new Wrapped Function Exotic Objects to have [[WrappedTargetFunction]] pointing to the original function from the same realm. Since this is not observable, should be possible, meaning you're creating a Wrapped Function Exotic Objects with a [[WrappedTargetFunction]] slot pointing to a callable from the same realm.

2. Wrapped Function Exotic Objects [[Call]] step 4 invokes GetFunctionRealm on the wrapped function exotic object, which will return the current Realm Record because wrapped function exotic objects have no [[Realm]] slot and are neither bound functions nor proxies. Is that intentional, and if so then why jump into GetFunctionRealm at all?

We had some back-and-forward with this particular issue very recently. Initially, I specified that the Wrapped Function Exotic Objects should have a [[Realm]] slot so we could hit step 2.a of GetFunctionRealm. It might or might not be a problem, I'm not sure. We made some changes to simplify that, relying on the fact that when a Wrapped Function Exotic Objects is created, its [[Prototype]] is well defined, and assuming it can only be see by one realm, then step 5 of GetFunctionRealm seems sufficient. But now that you mentioned it again, this might be problematic in the Web. E.g.: If a same domain iframe access a
Wrapped Function Exotic Objects from another realm, and attempts to call it, the _callerRealm_ will be the iframe's Realm, which is weird, in the sense that depending on who calls the Wrapped Function Exotic Objects, the result will be different. /cc @erights @littledan for guidance here. A simple solution is to bring back the [[Realm]] slot cc @leobalter.

3. PerformRealmEval step 19 is missing the privateEnv argument in its invocation of EvalDeclarationInstantiation.

Good catch. @leobalter the value of _privateEnv_, which is a PrivateEnvironment, should be null for indirect eval, which is the case here.

4. Does PerformRealmEval intentionally skip the check for "If result.[[Type]] is normal and result.[[Value]] is empty" that appears in PerformEval after evaluation? What are the consequences of that omission?

I believe the editorial note explains a little bit, but basically, for the value to be empty, it must be a return statement of some sort, which is not allow by syntax in this case. Maybe I'm missing something here.

5. Nit: in PerformRealmEval step 23, I think throw a newly created TypeError object associate to the _callerRealm_ should be phrased like throw a *TypeError* exception (optionally with a note to clarify its associated realm).

An alternative here is to use the full reference to that TypeError, similar to what we do in RealmImportValue. e.g.: _callerRealm_.[[Intrinsics]].[[%TypeError%]].

6. RealmImportValue makes a type assertion about every argument except evalContext, is that intentional?

Probably an oversight, we should assert it.

7. Are you sure you want ExportGetter functions to throw TypeError when the requested export is not provided? I believe importing an unexported name is a SyntaxError: https://jsfiddle.net/531ntwq9/

I believe I asked the same question in the PR from @littledan, I agree with this.

10. Nit: in HostInitializeSyntheticRealm, "provide platform capabilities with no authority to cause side effects…" is worded a bit strangely.

Yes, there is a PR for that particular piece: #315

12. Question: Why does Realm.prototype.evaluate require its argument to be a String while Realm.prototype.importValue performs type coercion?

I can answer the first part: it is that strict to avoid the confusion with eval, which has a bizarre behavior by returning the value of the first argument if it is not a string value. As for the second one, I believe it is a trying to match the semantics of dynamic import, which does not exhibit any bizarre behavior. An alternative here is to make evaluate to perform type coercion, which I'm open to, which will mean people moving from eval(value) to myRealm.evaluate(value) will not result in an error, but the behavior will not match the original behavior of eval either.

[gibson042]

1. The "Internal Slots of Wrapped Function Exotic Objects" table defines slot [[WrappedTargetFunction]] as holding "the callable object from the other Realm", despite the definition of wrapped function exotic object not being limited to cross-realm scenarios (e.g., "is an exotic object that wraps a callable object from another Realm.").

That is intentional, to have room for implementers to do optimizations. E.g.: if a realm calls a Wrapped Function Exotic Objects, and passes as an argument a regular function, which will be wrapped on the way in, if that Wrapped Function Exotic Objects comes back to the realm via another call, there is nothing that prevent the implements to optimize the new Wrapped Function Exotic Objects to have [[WrappedTargetFunction]] pointing to the original function from the same realm. Since this is not observable, should be possible, meaning you're creating a Wrapped Function Exotic Objects with a [[WrappedTargetFunction]] slot pointing to a callable from the same realm.

Ah, in that case I think the best fix is removing "from the other realm" text, leaving the description of [[WrappedTargetFunction]] as just "Stores the callable object from the other Realm."

An alternative here is to make evaluate to perform type coercion, which I'm open to, which will mean people moving from eval(value) to myRealm.evaluate(value) will not result in an error, but the behavior will not match the original behavior of eval either.

So realm.importValue(nonString1, nonString2) coerces its arguments but realm.evaluate(nonString) is deviating from eval(nonString) and there is a choice between the deviation being thrown TypeError vs. coercion to string? I don't have strong feelings either way, although TypeError is probably the right call because it can be reversed in the future if need arises. But I do wonder if the operation header should mention this intentional divergence.

[leobalter]

@syg

Editorially, the spec text mostly looks okay to me modulo the following:

• There are steps that say something like "If runningContext is not already suspended, suspend runningContext." What's the situation where the running context can already be suspended?

This inherits from the PerformEval steps and we are just keeping some consistency. Maybe this was just excessive caution and it's not necessary due to evaluate not having the same distinction between direct/indirect evals?

• I'm still not clear on why Realm instances need an [[ExecutionContext]] stashed away. Can it be created fresh each time it's needed? That shouldn't be observable, right?

It is not observable, but I remain tricked on how to change that. I'll try to get a PR soon.

• ValidateRealmObject needs an aoid presumably for the autolinking.

Resolved at 932220c

Thanks!

[leobalter]

@syg I opened #320, but I'm not sure if that's the same direction you meant, PTAL?

[leobalter]

Thanks everyone of the reviews! I'll sunset this thread while cleaning up the repo. We should have a new one soon for the Stage 4 roadmap.