ES2017 review #775

anba · 2017-01-23T16:09:02Z

I've kept the changes in small commits so it's easier to review them.

eval() and Function() are built-in functions, so per 9.3.1 the "current Realm Record" is the same realm as the [[Realm]] of the active function object.

Move the HostEnsureCanCompileStrings call into CreateDynamicFunction so it's also invoked for Generator and AsynFunction

All function Environment Records have a [[HomeObject]] field, use HasSuperBinding() to determine if it's a method. Use [[FunctionKind]] to detect class constructors in PerformEval.

…cripts And change "Constructors" to "Constructor methods" to avoid confusion with normal functions, because those are also constructors.

This error condition can no longer happen with the new early error checks for eval scripts. It was previously needed to handle: (function(){ eval("super.p") })()

This error condition can no longer happen with the new early error checks for eval scripts. It was previously needed to handle: (function(){ eval("super()") })()

Keep the entries in alphabetical order.

In addition to Script and Module, goal symbols for the grammar can also be FunctionBody, GeneratorBody, AsyncFunctionBody, and FormalParameters.

This is needed to avoid executing 9.1.6.3 ValidateAndApplyPropertyDescriptor, step 9, because exotic string objects don't use real properties for the string index properties.

…consonant

…Expression This is similar to tc39#706.

The shorthand form is only valid for functional and method application styles per 5.2 Algorithm Conventions.

…s in typed array constructors Prefer srcByteLength instead of srcLength to avoid confusion with [[ArrayLength]] of a typed array.

…pe.set srcLength holds typedArray[[ArrayLength]], but we need typedArray[[ByteLengt]] when calling CloneArrayBuffer.

Similar to 'length' for %ArrayPrototype%.

Using <emu-clause> renders thisBooleanValue as property of Boolean.prototype. Alternatively we could use a nested <emu-clause> in Boolean.prototype.toString.

Change the length property back to 1 as defined in ECMA2016, implemented in all browsers, and also as tested in test262.

So it's properly linked in "Table 7: Well-known Intrinsic Objects".

…space exotic objects ResolveExport isn't specified as infallible (consider the case when the module isn't a Source Text Module Record), so we need to handle abrupt completions. Reverts tc39#507

…entifierRef and IsValidSimpleAssignmentTarget

…laration instantiation

…flict with AsyncFunctionDeclaration, not AsyncFunctionExpression

Also remove GeneratorMethod because it's already implied by MethodDefinition

…ructor/instances section

domenic · 2017-01-23T16:24:23Z

Maybe we should move the HostEnsureCanCompileStrings check into CreateDynamicFunction?

anba · 2017-01-23T16:30:00Z

Maybe we should move the HostEnsureCanCompileStrings check into CreateDynamicFunction?

That's fixed in the next commit 3f36151.

domenic · 2017-01-23T16:33:00Z

I see, I didn't expand the commit description; sorry for the noise!

ljharb

Most of the commits are either beyond my expertise or look fine - just a few questions.

ljharb · 2017-01-23T16:54:58Z

spec.html

@@ -8258,7 +8258,6 @@

      <emu-clause id="sec-set-immutable-prototype">
        <h1>SetImmutablePrototype ( _O_, _V_ )</h1>
-        <!-- This is used by host environment specs, even though it only has one consumer in this spec; don't inline it -->


How is this stale? I don't see anywhere else in the spec that references it (unless somewhere else in this PR references it?)

Either way, it seems prudent to leave it in, in case the other uses later get factored out.

There are now two callers of SetImmutablePrototype, so it's unlikely to get inlined.

Where are those two? I was unable to find them.

Either way tho, I think the comment is an important marker for the external dependency, and I'd expect every abstract operation that's referenced externally to be similarly noted.

SetImmutablePrototype is used in 9.4.6.1 and 9.4.7.1. And we don't control which abstract operations are used by external parties, so we can't really mark every operation that's used externally. Allen's proposal in #398 seems more doable than having an incomplete coverage of operations which may or may not be used somewhere else.

ljharb · 2017-01-23T16:55:47Z

spec.html

@@ -33566,8 +33566,8 @@

      <!-- es6num="24.1.2.1" -->
      <emu-clause id="sec-arraybuffer-length">
-        <h1>ArrayBuffer ( [ _length_ ] )</h1>
-        <p>When the `ArrayBuffer` function is called with optional argument _length_, the following steps are taken:</p>


Doesn't this only need to be changed if it's web-incompatible to change it?

Changing the length to zero is inconsistent with other functions, e.g. we could also change DataView.prototype.setInt32's length to zero because that function also "works" when called without any arguments, but the expectation is that setInt32 is called with at least two arguments. And the same applies for ArrayBuffer, where we expect it's called with a single length argument.

ljharb · 2017-01-23T17:00:06Z

spec.html

@@ -27139,7 +27139,7 @@
    <!-- es6num="21.1.3" -->
    <emu-clause id="sec-properties-of-the-string-prototype-object">
      <h1>Properties of the String Prototype Object</h1>
-      <p>The String prototype object is the intrinsic object <dfn>%StringPrototype%</dfn>. The String prototype object is an ordinary object. The String prototype is itself a String object; it has a [[StringData]] internal slot with the value *""*.</p>
+      <p>The String prototype object is the intrinsic object <dfn>%StringPrototype%</dfn>. The String prototype object is an exotic String object and has the internal methods specified for such objects. It has a [[StringData]] internal slot with the value *""*.</p>


Is it exotic solely because of the [[StringData]] internal slot?

An object is only exotic if it uses a different implementation for any of the internal methods. And since String.prototype is a String object instance, and String objects are exotic String objects, String.prototype is also exotic.

Gotcha - and for my edification, why are String objects exotic (which internal methods does it differ for)?

[[GetOwnProperty]], [[OwnPropertyKeys]], and (soon) [[DefineOwnProperty]] are overridden for exotic string objects in https://tc39.github.io/ecma262/#sec-string-exotic-objects. This is needed to support new String("test")[0] === "t", because the indexed properties aren't real properties but instead are defined as virtual properties, similar to how this is implemented in browsers.

Makes sense.

bterlson · 2017-02-02T20:48:39Z

Reviewed most of these, working on thinking through a few of the normative-y ones. Any opinions on whether I should squash this monster or keep the individual commits?

ljharb · 2017-02-02T22:10:21Z

The individual commits are super nice for review, and would also be nice for git blame.

michaelficarra · 2017-02-02T23:06:39Z

Keep the individual commits. A lot of the spelling/typo commits can be merge though IMO.

bterlson · 2017-02-02T23:12:06Z

I have a rebased branch with individual commits with updated commit messages. I'll do a rebase -i before I merge and join some like commits, but mostly leave them separate. Thanks for opinions :)

bterlson · 2017-02-04T01:55:12Z

Pushed most of the commits individually (other than the ones I called out here). I guess I don't see a reason to collapse :)

In terms of release, none of the ones I left behind are bad to take during the RF opt-out period, so we will have some time to get them in before ES2017 is final-final.

anba · 2017-02-06T14:19:44Z

Pushed most of the commits individually (other than the ones I called out here).

These five commits seem to be the ones left out:
65b2c27
81da5a0
88d70d7
72aa1de
0cd8543

The commit message for 65b2c27 should be changed, because it's incorrect (seems like I got confused by the current use of super restrictions in ch14). So basically the current spec text is correct w.r.t. the super-property restrictions, but applying 65b2c27 (with a corrected commit message!) makes the super-property restrictions more consistent with the ones in ch14.

bterlson · 2017-02-06T22:25:40Z

65b2c27: IIUC, this has the effect of making SuperCall allowed in functions in eval. If this is correct, that doesn't align with clause 14 rules. Thoughts @anba?
81da5a0: I'm reluctant to take this change without hearing from some JSC folks (@msaboff)

88d70d7: Pulled (f608ef7)
72aa1de: Pulled (c934bf6)
0cd8543: Pulled (d88d05f)

bterlson · 2017-02-06T22:52:28Z

Thinking again about 65b2c27, SuperCall is of course not allowed due to the "... outside constructors". I'm just not sure I see the mapping to Clause 14.

I dislike this whole "... outside blah" notation after some experience with it.

bterlson · 2017-02-08T01:35:33Z

Forgot to update to say:

65b2c27: Pulled (9c6e5b9)

@anba would you be open to making a PR for 81da5a0? I'll get to it later this week if you don't :)

bakkot · 2017-02-08T04:05:07Z

Some of these changes are normative; test262 will need updates to match.

Normative changes which have gone in include at least the following, though not all of them make sense to test:

anba · 2017-02-09T17:18:12Z

@anba would you be open to making a PR for 81da5a0? I'll get to it later this week if you don't :)

Filed #810.

bterlson · 2017-02-13T19:21:30Z

This has all been merged! Thanks @anba.

anba added 30 commits January 23, 2017 06:50

Inspect the execution context stack to retrieve the caller context

449b4c3

eval() and Function() are built-in functions, so per 9.3.1 the "current Realm Record" is the same realm as the [[Realm]] of the active function object.

Call HostEnsureCanCompileStrings for Generator and AsyncFunction

3f36151

Move the HostEnsureCanCompileStrings call into CreateDynamicFunction so it's also invoked for Generator and AsynFunction

Allow super property accesses in eval scripts within methods

65b2c27

Fix detection of method contexts in PerformEval

4f0cf90

All function Environment Records have a [[HomeObject]] field, use HasSuperBinding() to determine if it's a method. Use [[FunctionKind]] to detect class constructors in PerformEval.

Add links to grammar productions for early error semantics for eval s…

6e4333a

…cripts And change "Constructors" to "Constructor methods" to avoid confusion with normal functions, because those are also constructors.

Assert HasSuperBinding() is true in MakeSuperPropertyReference

c836aa2

This error condition can no longer happen with the new early error checks for eval scripts. It was previously needed to handle: (function(){ eval("super.p") })()

Assert [[NewTarget]] is an object when evaluating SuperCall

7daa524

This error condition can no longer happen with the new early error checks for eval scripts. It was previously needed to handle: (function(){ eval("super()") })()

Fix method order in Object constructor methods

afc82fe

Keep the entries in alphabetical order.

Enable ASI when parsing from CreateDynamicFunction

17ea6b2

In addition to Script and Module, goal symbols for the grammar can also be FunctionBody, GeneratorBody, AsyncFunctionBody, and FormalParameters.

Add [[DefineOwnProperty]] for exotic String objects

d7cc29d

This is needed to avoid executing 9.1.6.3 ValidateAndApplyPropertyDescriptor, step 9, because exotic string objects don't use real properties for the string index properties.

Add plural-s in 'The %TypedArray% Intrinsic Object' preamble

87886d4

Remove extra 's' in description of %ArrayPrototype%

f4ab16d

Change 'a' to 'an' when internal slot name starts with vowel

2a2f774

Change 'an' to 'a' when followed by internal slot name starting with …

1e5a297

…consonant

Don't stop IsValidSimpleAssignmentTarget for UnaryExpression : Update…

d644dbc

…Expression This is similar to tc39#706.

Replace invalid use of ReturnIfAbrupt shorthand form

d16258e

The shorthand form is only valid for functional and method application styles per 5.2 Algorithm Conventions.

Fix typo cloneLength->srcLength in CloneArrayBuffer

58e49d2

Remove duplicate variable assignment and use consistent variable name…

a8a5b5f

…s in typed array constructors Prefer srcByteLength instead of srcLength to avoid confusion with [[ArrayLength]] of a typed array.

Use byte length when calling CloneArrayBuffer in %TypedArray%.prototy…

2c83974

…pe.set srcLength holds typedArray[[ArrayLength]], but we need typedArray[[ByteLengt]] when calling CloneArrayBuffer.

Clarify that the StringPrototype object is an exotic object

1340176

Define 'length' property for %StringPrototype% object

d337b22

Similar to 'length' for %ArrayPrototype%.

Don't render thisBooleanValue as a property of Boolean.prototype

ec47f16

Using <emu-clause> renders thisBooleanValue as property of Boolean.prototype. Alternatively we could use a nested <emu-clause> in Boolean.prototype.toString.

Add dfn for thisNumberValue

f4b622a

Add dfn for thisTimeValue

d5a483c

Add dfn for thisStringValue

095412e

Change ArrayBuffer.length back to 1 per web-reality

19992e8

Change the length property back to 1 as defined in ECMA2016, implemented in all browsers, and also as tested in test262.

Add dfn for %Generator%

6bcc992

So it's properly linked in "Table 7: Well-known Intrinsic Objects".

Add return-statement to InitializeHostDefinedRealm

ae01399

Document all possible return values for ResolveExport

2271aea

Handle abrupt completion from ResolveExport in [[Get]] of module name…

4f90710

…space exotic objects ResolveExport isn't specified as infallible (consider the case when the module isn't a Source Text Module Record), so we need to handle abrupt completions. Reverts tc39#507

anba added 6 commits January 23, 2017 06:51

Add missing PrimaryExpression:AsyncFunctionExpression entries to IsId…

ba1baaf

…entifierRef and IsValidSimpleAssignmentTarget

Handle async function declaration in function/block/global/module dec…

f2ef4b6

…laration instantiation

Mention async function declarations in environment records section

8805bd7

ExpressionStatement cannot start with 'async function' because of con…

f3c2251

…flict with AsyncFunctionDeclaration, not AsyncFunctionExpression

Handle async function in Function.prototype.toString

ec3426d

Also remove GeneratorMethod because it's already implied by MethodDefinition

Use emu-xref instead of fixed section number for async function const…

34243d6

…ructor/instances section

This was referenced Jan 23, 2017

Add HostEnsureCanCompileStrings check to AsyncGenerator tc39/proposal-async-iteration#79

Closed

Check grammar parameters again tc39/proposal-async-iteration#80

Closed

ljharb reviewed Jan 23, 2017

View reviewed changes

bterlson self-requested a review January 23, 2017 17:28

anba mentioned this pull request Jan 24, 2017

remove mentions of unused [[SourceText]] internal slot; renumber lists #779

Merged

bakkot added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Feb 8, 2017

anba mentioned this pull request Feb 9, 2017

Allow tail-calls in for-in loops #810

Merged

bterlson closed this Feb 13, 2017

jmdyck mentioned this pull request Jan 3, 2022

Editorial: missing syntax error (involving CoverCallExpressionAndAsyncArrowHead) #2607

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES2017 review #775

ES2017 review #775

anba commented Jan 23, 2017

domenic commented Jan 23, 2017

anba commented Jan 23, 2017

domenic commented Jan 23, 2017

ljharb left a comment

ljharb Jan 23, 2017

anba Jan 23, 2017

ljharb Jan 23, 2017

ljharb Jan 23, 2017

anba Jan 23, 2017

ljharb Jan 23, 2017

anba Jan 23, 2017

ljharb Jan 23, 2017

anba Jan 23, 2017

ljharb Jan 23, 2017

anba Jan 23, 2017

ljharb Jan 23, 2017

bterlson commented Feb 2, 2017

ljharb commented Feb 2, 2017

michaelficarra commented Feb 2, 2017

bterlson commented Feb 2, 2017

bterlson commented Feb 4, 2017

anba commented Feb 6, 2017

bterlson commented Feb 6, 2017

bterlson commented Feb 6, 2017

bterlson commented Feb 8, 2017

bakkot commented Feb 8, 2017

anba commented Feb 9, 2017

bterlson commented Feb 13, 2017

ES2017 review #775

ES2017 review #775

Conversation

anba commented Jan 23, 2017

domenic commented Jan 23, 2017

anba commented Jan 23, 2017

domenic commented Jan 23, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bterlson commented Feb 2, 2017

ljharb commented Feb 2, 2017

michaelficarra commented Feb 2, 2017

bterlson commented Feb 2, 2017

bterlson commented Feb 4, 2017

anba commented Feb 6, 2017

bterlson commented Feb 6, 2017

bterlson commented Feb 6, 2017

bterlson commented Feb 8, 2017

bakkot commented Feb 8, 2017

anba commented Feb 9, 2017

bterlson commented Feb 13, 2017