André Bargull [:anba] Assignee
	Description • 9 years ago

Similar to bug 866624, but this time restricted to the special __proto__ property.

Test case 1 - Destructuring binding pattern:
---
var {__proto__: a, __proto__: b} = {};
---

Test case 2 - Destructuring assignment:
---
({__proto__: a, __proto__: b} = {});
---


Expected: No SyntaxError
Actual: SyntaxError: property name __proto__ appears more than once in object literal


Note: This issue also points out a specification bug, because ES2015 B.3.1 does not specify whether or not the duplicate __proto__ restriction applies to the covered ObjectLiteral or the actual ObjectLiteral production.

	André Bargull [:anba] Assignee
	Comment 1 • 8 years ago

There seem to be many unnecessary PossibleError allocations throughout the whole Parser (in expr(), assignExpr(), unaryOpExpr(), memberExpr()). Do you think my removals are correct, or am I missing something here?

And pending errors are also thrown too early, e.g. `({z=1}={}, {w=2}, {e=3})=>{}` should parse, but currently throws a SyntaxError (there are many other examples, I still need to write the tests).

	André Bargull [:anba] Assignee
	Comment 2 • 8 years ago

The actual patch is simple. PossibleError can't store message arguments, so I had to a new JSMSG_DUPLICATE_PROTO_PROPERTY error message. The message itself should probably be amended, e.g. "duplicate __proto__ initializer in object literal". WDYT?

	Tooru Fujisawa [:arai]
	Comment 3 • 8 years ago

Comment on attachment 8796683 [details] [diff] [review]
bug1204024.part1.patch

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

Looks good.

::: js/src/frontend/Parser.cpp
@@ +3732,5 @@
>  {
> +    if (hasError()) {
> +        DebugOnly<bool> result = parser_.reportWithOffset(reportKind_, strict_, offset_,
> +                                                          errorNumber_);
> +        MOZ_ASSERT(!result, "PossibleError shouldn't be used for recoverable errors");

Can we just always use ParseError instead of reportKind_?
That way result should always be false.

I don't see any reason to make reportKind_ configurable.
if it's not necessary, can you remove the member and corresponding setPending parameter?

@@ +4946,5 @@
>  Parser<ParseHandler>::expressionStatement(YieldHandling yieldHandling, InvokedPrediction invoked)
>  {
>      tokenStream.ungetToken();
> +    Node pnexpr = expr(InAllowed, yieldHandling, TripledotProhibited,
> +                       nullptr /* possibleError */, invoked);

while you're touching this, can you rewrite to this syntax?
  ..., /* possibleError = */ nullptr, ...

it's used in other places.

@@ +8205,5 @@
>  typename ParseHandler::Node
>  Parser<ParseHandler>::memberExpr(YieldHandling yieldHandling, TripledotHandling tripledotHandling, TokenKind tt,
>                                   bool allowCallSyntax, InvokedPrediction invoked)
>  {
> +    Node pn = memberExpr(yieldHandling, tripledotHandling, nullptr /* possibleError */, tt,

here too,
  /* possibleError = */ nullptr

::: js/src/frontend/Parser.h
@@ +1130,1 @@
>                InvokedPrediction invoked = PredictUninvoked);

default parameter should be better written also in definition, as a comment,
especially if it's nullable:

  Parser<ParseHandler>::expr(InHandling inHandling, YieldHandling yieldHandling,
                             TripledotHandling tripledotHandling,
                             PossibleError* possibleError /* = nullptr */,
                             InvokedPrediction invoked /* = PredictUninvoked */)

| = PredictUninvoked| is omitted in all places tho, it would be nice to add it too.
maybe in followup bug.

@@ +1133,1 @@
>                      InvokedPrediction invoked = PredictUninvoked);

same here.

@@ +1146,1 @@
>                     InvokedPrediction invoked = PredictUninvoked);

same here.

@@ +1152,5 @@
>      Node primaryExpr(YieldHandling yieldHandling, TripledotHandling tripledotHandling,
>                       PossibleError* possibleError, TokenKind tt,
>                       InvokedPrediction invoked = PredictUninvoked);
>      Node exprInParens(InHandling inHandling, YieldHandling yieldHandling,
> +                      TripledotHandling tripledotHandling, PossibleError* possibleError = nullptr);

and here.

	Tooru Fujisawa [:arai]
	Comment 4 • 8 years ago

Comment on attachment 8796686 [details] [diff] [review]
bug1204024.part2.patch

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

please add browser.js and shell.js with empty content, in js/src/tests/ecma_7/Destructuring.

	André Bargull [:anba] Assignee
	Comment 5 • 8 years ago

(In reply to Tooru Fujisawa [:arai] from comment #4)
> please add browser.js and shell.js with empty content, in
> js/src/tests/ecma_7/Destructuring.

Yes, I know about this issue. I still need to update the patches for bug 1272784, bug 1306701 and bug 1204024, because they weren't generated with `hg diff --git`. Because when I installed TortioseHg for testing purposed, TortioseHg removed my "[diff] git = true" settings from .hgrc. (Apparently TortioseHg only understands "git = True", but not "git = true"... *sigh*)

	André Bargull [:anba] Assignee
	Comment 6 • 8 years ago

I had to completely redo part 1, because some of the changes were counter-productive for the upcoming patches to fix bug 1041341. The old "part 1" is now split into three different patches (part 1-2 & 4). The old "part 2" is now "part 5". And the new "part 3" is a clean-up patch to simplify pending error checks for destructuring patterns and to reduce code duplication. 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=921e413c5b16a228cfaa65a543ee84381261867e&selectedJob=28880368

	André Bargull [:anba] Assignee
	Comment 7 • 8 years ago

Don't create a PossibleError instance when we're in a context where destructuring forms cannot appear. This change also allows us to remove the duplicate expr(), assignExpr(), memberExpr(), and exprInParens() methods, which seems like a nice bonus; the primaryExpr() parameters were only reordered to match memberExpr(). And I've made the allowCallSyntax of memberExpr() a default parameter for convenience reasons, but I guess it should actually be an enum parameter (e.g. CallSyntaxHandling) - maybe for a follow-up bug.

	André Bargull [:anba] Assignee
	Comment 8 • 8 years ago

PossibleError can only be used for non-recoverable errors, therefore ParseReportKind and the boolean strict parameter aren't particularly useful as parameters for setPending(). The Node parameter was added for part 5, and the if-condition in transferErrorTo() for part 4.

	André Bargull [:anba] Assignee
	Comment 9 • 8 years ago

Remove the duplicate code in declarationPattern() with a direct call to destructuringDeclaration() and move all PossibleError checks for destructuring patterns to checkDestructuringPattern to avoid repeating the same steps in multiple places.

	André Bargull [:anba] Assignee
	Comment 10 • 8 years ago

Pending PossibleErrors were reported too early in expr() and objectLiteral() resp. PossibleError objects weren't properly passed along in objectLiteral() and arrayInitializer(). Both issues prevented us from recognizing some destructuring forms.

	André Bargull [:anba] Assignee
	Comment 11 • 8 years ago

No additional changes (except for adding {shell,browser}.js files). Carrying r+ from arai.

	Tooru Fujisawa [:arai]
	Comment 12 • 8 years ago

Comment on attachment 8799507 [details] [diff] [review]
bug1204024-part2.patch

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

::: js/src/frontend/Parser.cpp
@@ +3718,5 @@
>  }
>  
>  template <typename ParseHandler>
>  Parser<ParseHandler>::PossibleError::PossibleError(Parser<ParseHandler>& parser)
> +                                                   : parser_(parser), state_(ErrorState::None)

while you're touching this, can you fix the indent here to follow other places?

  template <typename ParseHandler>
  Parser<ParseHandler>::PossibleError::PossibleError(Parser<ParseHandler>& parser)
    : parser_(parser), state_(ErrorState::None)

@@ +3719,5 @@
>  
>  template <typename ParseHandler>
>  Parser<ParseHandler>::PossibleError::PossibleError(Parser<ParseHandler>& parser)
> +                                                   : parser_(parser), state_(ErrorState::None)
> +{ }

Please remove the space.

  {}

::: js/src/frontend/Parser.h
@@ +721,5 @@
>  template <typename ParseHandler>
>  class Parser final : private JS::AutoGCRooter, public StrictModeGetter
>  {
>    private:
> +    typedef typename ParseHandler::Node Node;

Can you change it to c++11 style while you're touching it?
  using Node = typename ParseHandler::Node;

	André Bargull [:anba] Assignee
	Comment 13 • 8 years ago

Updated part 2 per review comments. Carrying r+ from arai.

	André Bargull [:anba] Assignee
	Comment 14 • 8 years ago

Updated part 4 to apply cleanly on updated part 2. Carrying r+.

	Pulsebot
	Comment 15 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1135c6ee9d
Part 1: Set possibleError to nullptr in expression-only contexts. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c13851000ef0
Part 2: Restrict PossibleError to non-recoverable errors, because it's not possible to recover from a pending error. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ebb036c18ea
Part 3: Move destructuring error checking for PossibleError to checkDestructuringPattern. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/04339bee7352
Part 4: Delay error reporting for pending errors until the destructuring pattern is completely parsed. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5b906ef8c8
Part 5: Allow duplicate __proto__ properties in object destructuring. r=arai

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fe1135c6ee9d
https://hg.mozilla.org/mozilla-central/rev/c13851000ef0
https://hg.mozilla.org/mozilla-central/rev/5ebb036c18ea
https://hg.mozilla.org/mozilla-central/rev/04339bee7352
https://hg.mozilla.org/mozilla-central/rev/5f5b906ef8c8

	Florian Scholz (Open Web Docs)
	Comment 17 • 7 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/52#JavaScript