Skip to content
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

Editorial: Address Gibson's feedback #318

Merged
merged 1 commit into from
Jul 12, 2021
Merged

Editorial: Address Gibson's feedback #318

merged 1 commit into from
Jul 12, 2021

Conversation

leobalter
Copy link
Member

Ref Comment: #304 (comment)

cc @gibson042

Resolved in this PR:

  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.").
  1. PerformRealmEval step 19 is missing the privateEnv argument in its invocation of EvalDeclarationInstantiation.
  2. 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?
  3. 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).
  4. RealmImportValue makes a type assertion about every argument except evalContext, is that intentional?
  1. 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).
  2. 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].
  1. Nit: I think there should be a space between the parentheses in Realm () and before the left parenthesis in ValidateRealmObject( argument ).

Other bullet points:

  1. 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?

I'll have a separate work for that.

  1. 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/

importValue is not simulating a static import so I don't believe a SyntaxError would fit. Everything else is a TypeError, even if the import is not successfully parsed, I believe the TypeError keeps consistency and this is due to the fact there isn't much information coming from what happened in the other Realm. A SyntaxError here would work specifically as a hasOwnProperty verification for the given module namespace.

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

I believe this is resolved by #315, does it look good to you?

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

I believe Caridy answered this, but in any case, I believe it's all ok for importValue and I'm in favor of coercing the Realm.prototype.evaluate argument to string but it is a different path compared to eval.

@leobalter
Copy link
Member Author

For the records: all the fixes here are purely editorial.

@leobalter leobalter requested a review from rwaldron July 8, 2021 23:10
@caridy
Copy link
Collaborator

caridy commented Jul 10, 2021

I believe Caridy answered this, but in any case, I believe it's all ok for importValue and I'm in favor of coercing the Realm.prototype.evaluate argument to string but it is a different path compared to eval.

I'm fine with that too... let's coercing it!

@leobalter leobalter merged commit ebafcea into main Jul 12, 2021
@leobalter leobalter deleted the leo/304-gibson-fb branch July 12, 2021 18:41
leobalter added a commit that referenced this pull request Jul 12, 2021
@@ -172,6 +174,9 @@ <h1>PerformRealmEval ( _sourceText_, _callerRealm_, _evalRealm_ )</h1>
<emu-note type=editor>
Some steps from PerformRealmEval are shared with |eval| and |Function| and should result into a shared abstraction when merged to ECMA-262.
</emu-note>
<emu-note>
This abstraction requires the performed evaluation to result into a normal completion. Otherwise, if the result is not a normal completion, the abstraction will throw a TypeError exception associated to its original running execution context.
</emu-note>>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it hasn't already been cleaned up, there's a typo here.

Suggested change
</emu-note>>
</emu-note>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I got that earlier today and fixed it already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants