1061853 - Implement ES6 object-literal __proto__ restrictions/semantics

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

In ES6, an object literal can only contain a single __proto__:v member.  And only __proto__:v induces [[Prototype]] mutation.  All other member forms mentioning "__proto__" define regular old own properties.

We are currently wrong about the latter case, in that method members like __proto__() {} will overwrite the [[Prototype]].

And we're currently wrong about the former case because it's a new restriction.

The bytecode emitter code for destructuring is a hideously unreadable mess.  Same (to much lesser extent) for some of the parsing code.  So you get a large stack of micro-refactorings to sort through, before the meat.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Give some variables in EmitObject better names

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Negate an if to convert a super-large indented block into code after a continue in a loop

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Make an if-else into an if-stuff-and-return, other-stuff-and-return for better readability

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Rename |pn| to |pattern| for readability

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Rename |pn2| to |member| for readability

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Rename |pn3| to |subpattern| for readability

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Split EmitDestructuringOpsHelper into array/object flavors

Simplifications to all this copied code to follow.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Remove most object-specific code from the array-destructuring helper

Comment 9

[Jeff Walden [:Waldo]]

Attachment: More array-specific destructuring simplifications

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Modify array-destructuring stack comments to include the possible presence of the object being destructured

These comments are the best idea ever.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Remove most array-specific code from the object-destructuring helper

Comment 12

[Jeff Walden [:Waldo]]

Attachment: More object-specific destructuring simplifications

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Add stack-layout comments to destructuring of object patterns

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Make destructuring elision an early exit, rather than one of two alternatives

Spreading should possibly be another separate exit, maybe.  Dunno.  Taking one step at a time, that one probably won't be in this bug actually, as I got to a point, then fixed this bug's issues, and realized I'd left that bit undone.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Add better explanation of stack layout being created while destructuring an object pattern, in the PushInitialValues case

This is almost but not entirely there -- still talks in terms of arrays, when really it's objects that are relevant here.  But at least you can tell me if I've made some horribly stupid mistake.

I haven't done the same for arrays yet, because there's more screwiness in this to comprehend.  Ultimately it'd be nice to share some code between these two loop-tails, but I'm unsure I'm at the point of understanding it well enough to do that yet.

Pay no attention to the vacuous assertion in this code.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Add a test for __proto__ in object literals in various forms (normal, shorthand, computed, method)

This test fails currently.  It also fails with __proto__() {} fixed to define a property.  Only with duplicate __proto__:v as early error does it pass.  I wrote the whole thing with an eye to final semantics, originally.  I tried going back to fix to intermediate semantics, but it looked messy, and not worth it, so I punted.  Guess this'll have to land after all the (non-refactoring) fixes or so.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Reintroduce PNK_MUTATEPROTO to distinguish ({ __proto__: v }) as mutating the [[Prototype]] from ({ __proto__() {} }) as not doing so

This fixes __proto__() {} to not mutate [[Prototype]].

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Make duplicate __proto__ in an object literal a syntax error

And this fixes the bug that __proto__:v is allowed more than once in an object literal.

The Reflect.parse changes change that API, to be sure.  jimb thinks we should be bold in breaking.  That seems good to me.  If absolutely necessary I can refactor this back to making PNK_MUTATEPROTO binary with a kid for __proto__, but it seems a bit stupid unless absolutely necessary.  And I think the API addition is worth it -- it's not easy detecting actual mutation from not-mutation, otherwise.  Editors *should* detect that, for sure.

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8482890 [details] [diff] [review]
Give some variables in EmitObject better names

Review of attachment 8482890 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Nit: I think 'property' or 'propDef' or something like that is a bit clearer than 'member', since the spec talks about PropertyDefinitionList for object literal exprs.

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 8482896 [details] [diff] [review]
Make an if-else into an if-stuff-and-return, other-stuff-and-return for better readability

Review of attachment 8482896 [details] [diff] [review]:
-----------------------------------------------------------------

Personally I don't think this makes things more readable. The meat of this function discriminates on being an array or an object destructure. Returning true early makes this less clear by eyeballing IMO. Re-request r? if you feel strongly.

Comment 21

[Shu-yu Guo [:shu]]

Comment on attachment 8482914 [details] [diff] [review]
Add stack-layout comments to destructuring of object patterns

Review of attachment 8482914 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3378,3 @@
>                      return false;
>              } else {
> +                if (!EmitAtomOp(cx, name, JSOP_GETPROP, bce))          // ...OBJ PROP

Nit: space between ... OBJ

@@ +3396,5 @@
>          int32_t depthBefore = bce->stackDepth;
>          if (!EmitDestructuringLHS(cx, bce, subpattern, emitOption))
>              return false;
>  
> +        // The stack is now XXX

Accidentally left in?

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 8482927 [details] [diff] [review]
Add better explanation of stack layout being created while destructuring an object pattern, in the PushInitialValues case

Review of attachment 8482927 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3425,5 @@
> +        //
> +        // (where of course x = obj[0][0] and y = obj[0][1], where []-indexing
> +        // is really iteration-indexing).  We want to return to the loop-entry
> +        // state to destruct z from OBJ, so pick OBJ out of the stack and move
> +        // it to the top of the stack to accomplish this.

Nit: I like single-space after periods.

@@ +3443,4 @@
>          // Per the above loop invariant, the value being destructured into this
>          // object pattern is atop the stack.  Pop it to achieve the
>          // post-condition.
> +        if (Emit1(cx, bce, JSOP_POP) < 0)                              // ... <pattern's target name values, seriatim>

Maybe more mathy notation up to _n, where _n is # of targets in pattern?

Comment 23

[Shu-yu Guo [:shu]]

Comment on attachment 8482929 [details] [diff] [review]
Add a test for __proto__ in object literals in various forms (normal, shorthand, computed, method)

Review of attachment 8482929 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/Expressions/object-literal-__proto__.js
@@ +1,5 @@
> +// Any copyright is dedicated to the Public Domain.
> +// http://creativecommons.org/licenses/publicdomain/
> +
> +//-----------------------------------------------------------------------------
> +var BUGNUMBER = 999999;

Real bug number here.

Comment 24

[Shu-yu Guo [:shu]]

Comment on attachment 8482930 [details] [diff] [review]
Reintroduce PNK_MUTATEPROTO to distinguish ({ __proto__: v }) as mutating the [[Prototype]] from ({ __proto__() {} }) as not doing so

Review of attachment 8482930 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: js/src/frontend/Parser.cpp
@@ +3149,4 @@
>               * hasn't been officially linked to its def or registered in
>               * lexdeps.  Do that now.
>               */
> +            if (!member->isKind(PNK_MUTATEPROTO) && member->pn_right == member->pn_left) {

The comment above seems out of date. If so, this conditional can be deleted.

Comment 25

[Shu-yu Guo [:shu]]

Comment on attachment 8482934 [details] [diff] [review]
Make duplicate __proto__ in an object literal a syntax error

Review of attachment 8482934 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +7150,4 @@
>  
>          atom = nullptr;
>  
> +        atom = nullptr;

Unnecessary second nulling.

Comment 26

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f00e4df5f58
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b1d990afc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/31abc5473afd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e826a944ad33
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d63cee2a7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f18ce7148c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/077759e55ad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad8e619986f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/acdc20cf4236
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4c3e607675
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63847a76b6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14a0c0a7c99
https://hg.mozilla.org/integration/mozilla-inbound/rev/836f7fb827cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2359593dae4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8fc947b2be
https://hg.mozilla.org/integration/mozilla-inbound/rev/8acb4009398c
https://hg.mozilla.org/integration/mozilla-inbound/rev/642d3f6d79d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d6b5bf9f7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/19961d1aebb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6ef3cea4bc

(In reply to Shu-yu Guo [:shu] from comment #19)
> Nit: I think 'property' or 'propDef' or something like that is a bit clearer
> than 'member', since the spec talks about PropertyDefinitionList for object
> literal exprs.

Fair.  Renamed after all this bug's patches, so as not to make patching pain for me in propagating the rename through all the other patches here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d6b5bf9f7b

(In reply to Shu-yu Guo [:shu] from comment #20)
> Personally I don't think this makes things more readable. The meat of this
> function discriminates on being an array or an object destructure. Returning
> true early makes this less clear by eyeballing IMO.

Broke it into two functions, requiring some template specialization declaration fugly, but oh well.  One other benefit: this pointed out that the |blockObj| local is entirely useless, so I got to remove that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/31abc5473afd

(In reply to Shu-yu Guo [:shu] from comment #22)
> Nit: I like single-space after periods.

Boo-urns.

Interestingly, the posted patch and the landed patch managed to perfectly align the ends of every line in this comment.  (Okay, I may have tried a little -- but only a little! -- to make this happen.  :-) )

> Maybe more mathy notation up to _n, where _n is # of targets in pattern?

Given the other comments shied away from talking in non-general terms, and you have to in order to discuss how many targets (if any!) there might be, I left this as-is.  Or at least that's what I interpreted this comment to mean.

(In reply to Shu-yu Guo [:shu] from comment #23)
> Comment on attachment 8482929 [details] [diff] [review]
> Add a test for __proto__ in object literals in various forms (normal,
> shorthand, computed, method)

This test got substantially modified to add in getters, setters, and computed versions of these.  I folded it into the patch to make duplicate [[Prototype]] mutation an error, as it would have been very impractical to make it work without that change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/642d3f6d79d9

(In reply to Shu-yu Guo [:shu] from comment #24)
> >               * hasn't been officially linked to its def or registered in
> >               * lexdeps.  Do that now.
> >               */
> > +            if (!member->isKind(PNK_MUTATEPROTO) && member->pn_right == member->pn_left) {
> 
> The comment above seems out of date. If so, this conditional can be deleted.

Yeah, looks it.  Try+our tests said it was okay, removed in a followup revision for bisection purposes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/19961d1aebb7(In reply to Shu-yu Guo [:shu] from comment #25)
> ::: js/src/frontend/Parser.cpp
> @@ +7150,4 @@
> >  
> >          atom = nullptr;
> >  
> > +        atom = nullptr;
> 
> Unnecessary second nulling.

Disagree, it was the only way to be sure.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f00e4df5f58
https://hg.mozilla.org/mozilla-central/rev/c5b1d990afc2
https://hg.mozilla.org/mozilla-central/rev/31abc5473afd
https://hg.mozilla.org/mozilla-central/rev/e826a944ad33
https://hg.mozilla.org/mozilla-central/rev/46d63cee2a7a
https://hg.mozilla.org/mozilla-central/rev/3f18ce7148c9
https://hg.mozilla.org/mozilla-central/rev/077759e55ad8
https://hg.mozilla.org/mozilla-central/rev/ad8e619986f3
https://hg.mozilla.org/mozilla-central/rev/acdc20cf4236
https://hg.mozilla.org/mozilla-central/rev/ce4c3e607675
https://hg.mozilla.org/mozilla-central/rev/d63847a76b6c
https://hg.mozilla.org/mozilla-central/rev/c14a0c0a7c99
https://hg.mozilla.org/mozilla-central/rev/836f7fb827cc
https://hg.mozilla.org/mozilla-central/rev/2359593dae4a
https://hg.mozilla.org/mozilla-central/rev/2a8fc947b2be
https://hg.mozilla.org/mozilla-central/rev/8acb4009398c
https://hg.mozilla.org/mozilla-central/rev/642d3f6d79d9
https://hg.mozilla.org/mozilla-central/rev/a9d6b5bf9f7b
https://hg.mozilla.org/mozilla-central/rev/19961d1aebb7

Comment 28

[Florian Scholz (Open Web Docs)]

This will need notes on these pages probably (plus Fx 35 for devs)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer

Comment 29

[Jeff Walden [:Waldo]]

(In reply to Florian Scholz [:fscholz] (elchi3) from comment #28)
> This will need notes on these pages probably (plus Fx 35 for devs)
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Object/proto

I don't think this page needs a change, or should be changed.  __proto__-the-setter is entirely different functionality from __proto__-the-syntax-in-object-literals.  Conflating the two would just be confusing.

Comment 30

[Jeff Walden [:Waldo]]

(In reply to Florian Scholz [:fscholz] (elchi3) from comment #28)
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/
> Object_initializer

I made various changes to this to document __proto__ as distinct from normal property definitions, also noting that it may occur only once per object literal.

Comment 31

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto
> I don't think this page needs a change, or should be changed. 
> __proto__-the-setter is entirely different functionality from
> __proto__-the-syntax-in-object-literals.  Conflating the two would just be
> confusing.

Thanks for clearing that one up. It is indeed better to have a separate page for the literal syntax now.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer
> I made various changes to this to document __proto__ as distinct from normal
> property definitions, also noting that it may occur only once per object
> literal.

Great, thanks! Most of the time we try to avoid assert() in code samples. Pretty sure people dealing with prototype mutations will know what it means, but then the code samples also aren't executable in Scratchpad or the web console directly, for example.

Also added to developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/35#JavaScript