Implement ES6 object-literal __proto__ restrictions/semantics

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(18 attachments)

4.12 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.36 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.77 KB, patch
Details | Diff | Splinter Review
5.41 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.83 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.08 KB, patch
shu
: review+
Details | Diff | Splinter Review
11.21 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.60 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.61 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.81 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.49 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.64 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.76 KB, patch
shu
: review+
Details | Diff | Splinter Review
3.24 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.19 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.96 KB, patch
shu
: review+
Details | Diff | Splinter Review
17.93 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.53 KB, patch
shu
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: es6
Assignee

Comment 7

5 years ago
Simplifications to all this copied code to follow.
Attachment #8482900 - Flags: review?(shu)
Assignee

Comment 14

5 years ago
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.
Attachment #8482919 - Flags: review?(shu)
Assignee

Comment 15

5 years ago
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.
Attachment #8482927 - Flags: feedback?(shu)
Assignee

Comment 16

5 years ago
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.
Attachment #8482929 - Flags: review?(shu)
Assignee

Comment 18

5 years ago
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.
Attachment #8482934 - Flags: review?(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.
Attachment #8482890 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482892 - Flags: review?(shu) → review+
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.
Attachment #8482896 - Flags: review?(shu)

Updated

5 years ago
Attachment #8482897 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482898 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482899 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482900 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482901 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482902 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482907 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482910 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482913 - Flags: review?(shu) → review+
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?
Attachment #8482914 - Flags: review?(shu) → review+

Updated

5 years ago
Attachment #8482919 - Flags: review?(shu) → review+
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?
Attachment #8482927 - Flags: feedback?(shu) → review+
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.
Attachment #8482929 - Flags: review?(shu) → review+
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.
Attachment #8482930 - Flags: review?(shu) → review+
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.
Attachment #8482934 - Flags: review?(shu) → review+
Assignee

Comment 26

5 years ago
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.
Depends on: 1064391
Assignee

Comment 29

5 years ago
(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.
Assignee

Comment 30

5 years ago
(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.
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
You need to log in before you can comment on or make changes to this bug.