Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1146683002:
[destructuring] Implement initializers in patterns. (Closed)
|
Created:
5 years, 7 months ago by Dmitry Lomov (no reviews) Modified:
5 years, 7 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[destructuring] Implement initializers in patterns.
R=arv@chromium.org,rossberg@chromium.org,wingo@igalia.com
BUG=v8:811
LOG=N
Committed: https://crrev.com/5cdaa3e6e5da4998c8e1351174a662b3cb5828ce
Cr-Commit-Position: refs/heads/master@{#28482}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Correct patch this one is. #Patch Set 3 : Rebased onto ToT #
Messages
Total messages: 24 (5 generated)
PTAL. This is based on https://codereview.chromium.org/1139773005/
On 2015/05/18 12:30:14, Dmitry Lomov (chromium) wrote: > PTAL. This is based on https://codereview.chromium.org/1139773005/ Very nicely done! LGTM. Not sure what to do about duplicate identifiers in the pre-parser though; any idea? E.g. "let { x, x } = y".
On 2015/05/18 13:06:08, wingo wrote: > On 2015/05/18 12:30:14, Dmitry Lomov (chromium) wrote: > > PTAL. This is based on https://codereview.chromium.org/1139773005/ > > Very nicely done! LGTM. Not sure what to do about duplicate identifiers in the > pre-parser though; any idea? E.g. "let { x, x } = y". My understanding is that { x, x } is a valid pattern. var { x, x } = {} is completely ok. let { x, x } = {} fails later on when you try to declare two lexical bindings with the same name.
On 2015/05/18 13:13:44, Dmitry Lomov (chromium) wrote: > On 2015/05/18 13:06:08, wingo wrote: > > On 2015/05/18 12:30:14, Dmitry Lomov (chromium) wrote: > > > PTAL. This is based on https://codereview.chromium.org/1139773005/ > > > > Very nicely done! LGTM. Not sure what to do about duplicate identifiers in > the > > pre-parser though; any idea? E.g. "let { x, x } = y". > > My understanding is that { x, x } is a valid pattern. > > var { x, x } = {} is completely ok. > > let { x, x } = {} fails later on when you try to declare two lexical bindings > with the same name. Right. But "later on" is still early: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-let-and-const-declar... So e.g. function foo() { var {x,x} = {} } should signal an early error, even if foo() is never run and only parsed by the preparser. Similarly, ({x,x})=>34 is an early error -- not the same productions, but similar needs.
> So e.g. function foo() { var {x,x} = {} } should signal an early error, even
if
> foo() is never run and only parsed by the preparser.
I meant function foo() { let {x,x} = {} } of course.
On 2015/05/18 13:19:52, wingo wrote:
> > So e.g. function foo() { var {x,x} = {} } should signal an early error, even
> if
> > foo() is never run and only parsed by the preparser.
>
> I meant function foo() { let {x,x} = {} } of course.
Hmm, I see no difference between:
function foo() { let {x,x} = {x:5} }
and
function foo() {
let x = 5;
let x = 5;
}
Pre spec, both are early errors.
In our implementation we will report both at foo's _compile time_ (whenever that
occurs)
So far in V8, delaying early errors until compile time has been a fair game, the
only requirement being that the errors we report are the same regardless of when
compilation occurs
(whether we compile eagerly or lazily you are going to get the same error in
'foo').
LGTM Beautiful! https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc#new... src/pattern-rewriter.cc:210: Variable* Parser::PatternRewriter::CreateTemp(Expression* value) { CreateTempVar maybe? https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc#new... src/pattern-rewriter.cc:245: DCHECK(node->op() == Token::ASSIGN); Maybe add a comment explaining the desugaring let {<pattern> = init} = <current_value> becomes temp = <current_value> <pattern> = temp === undefined ? init : temp https://codereview.chromium.org/1146683002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1146683002/diff/1/src/preparser.h#newcode2909 src/preparser.h:2909: classifier->RecordBindingPatternError( Just curious... Is the reason why you are not doing a RecordAssignmentPatternError here because assignment patterns are n ot yet implemented? https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6366: "x", this is a duplicate of the next line https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:54: -1 newline https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:57: for(var {z = 3} = {}; z != 0; z--) { ws https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:254: Did you want to add tests for let {x, x} = {x: 1}; and some more variants of that: let {x, x = 2} = {x: 1}; let {x = (function() { x = 2; })()} = {}; // error let {x = (function() { x = 2; })()} = {x: 1}; // no error
On 2015/05/18 13:38:13, Dmitry Lomov (chromium) wrote:
> On 2015/05/18 13:19:52, wingo wrote:
> > > So e.g. function foo() { var {x,x} = {} } should signal an early error,
even
> > if
> > > foo() is never run and only parsed by the preparser.
> >
> > I meant function foo() { let {x,x} = {} } of course.
>
> Hmm, I see no difference between:
> function foo() { let {x,x} = {x:5} }
> and
> function foo() {
> let x = 5;
> let x = 5;
> }
> Pre spec, both are early errors.
> In our implementation we will report both at foo's _compile time_ (whenever
that
> occurs)
> So far in V8, delaying early errors until compile time has been a fair game,
the
> only requirement being that the errors we report are the same regardless of
when
> compilation occurs
> (whether we compile eagerly or lazily you are going to get the same error in
> 'foo').
I see! This means also that we should defer duplicate argument detection to
compile-time as well, methinks. Great.
https://codereview.chromium.org/1146683002/diff/1/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1146683002/diff/1/src/parser.h#newcode1015 src/parser.h:1015: Scope* TemporaryDeclarationScope() const { Nit: is there a strong reason to say "Temporary" here? https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6392: "{var: x = 42}", Add a test with computed property names. https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6460: "{x : x += a}", Add a test with method syntax, e.g. "{m() {} = 0}". https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:62: Nit: newline https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:66: var o = { get x() { Can we modify this test (and below) such that it also checks the order of the calls? https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:220: (function TestTDZInIntializers() { Add tests like let {x, y = eval("x")} = {x:42} let {x = () => y, y} = {y:42}; x() === 42 let {x = () => eval("y"), y} = {y:42}; x() === 42
Comments addressed + rebased on top of latest patch for https://codereview.chromium.org/1139773005/ https://codereview.chromium.org/1146683002/diff/1/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1146683002/diff/1/src/parser.h#newcode1015 src/parser.h:1015: Scope* TemporaryDeclarationScope() const { On 2015/05/18 16:17:19, rossberg wrote: > Nit: is there a strong reason to say "Temporary" here? Yes, it is a declaration scope for temporary variables (as opposed to declaration scope for the variables pattern matcher declares, which is controlled by DeclarationDescriptor and ultimately by whether this is a lexical declaration or not). However I inlined into CreateTempVar. https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc#new... src/pattern-rewriter.cc:210: Variable* Parser::PatternRewriter::CreateTemp(Expression* value) { On 2015/05/18 14:36:20, arv wrote: > CreateTempVar maybe? Done. https://codereview.chromium.org/1146683002/diff/1/src/pattern-rewriter.cc#new... src/pattern-rewriter.cc:245: DCHECK(node->op() == Token::ASSIGN); On 2015/05/18 14:36:20, arv wrote: > Maybe add a comment explaining the desugaring > > let {<pattern> = init} = <current_value> > > becomes > > temp = <current_value> > <pattern> = temp === undefined ? init : temp Done. https://codereview.chromium.org/1146683002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1146683002/diff/1/src/preparser.h#newcode2909 src/preparser.h:2909: classifier->RecordBindingPatternError( On 2015/05/18 14:36:20, arv wrote: > Just curious... Is the reason why you are not doing a > RecordAssignmentPatternError here because assignment patterns are n ot yet > implemented? Yes. https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6366: "x", On 2015/05/18 14:36:20, arv wrote: > this is a duplicate of the next line Done. https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6392: "{var: x = 42}", On 2015/05/18 16:17:19, rossberg wrote: > Add a test with computed property names. I want to cover computed property names in a separate CL; this CL is about intializers. https://codereview.chromium.org/1146683002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:6460: "{x : x += a}", On 2015/05/18 16:17:19, rossberg wrote: > Add a test with method syntax, e.g. "{m() {} = 0}". Done. https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... File test/mjsunit/harmony/destructuring.js (right): https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:54: On 2015/05/18 14:36:20, arv wrote: > -1 newline Done. https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:57: for(var {z = 3} = {}; z != 0; z--) { On 2015/05/18 14:36:20, arv wrote: > ws Done. https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:62: On 2015/05/18 16:17:20, rossberg wrote: > Nit: newline Done. https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:66: var o = { get x() { On 2015/05/18 16:17:19, rossberg wrote: > Can we modify this test (and below) such that it also checks the order of the > calls? Done. https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:220: (function TestTDZInIntializers() { On 2015/05/18 16:17:19, rossberg wrote: > Add tests like > > let {x, y = eval("x")} = {x:42} > let {x = () => y, y} = {y:42}; x() === 42 > let {x = () => eval("y"), y} = {y:42}; x() === 42 Done. https://codereview.chromium.org/1146683002/diff/1/test/mjsunit/harmony/destru... test/mjsunit/harmony/destructuring.js:254: On 2015/05/18 14:36:20, arv wrote: > Did you want to add tests for > > let {x, x} = {x: 1}; > > and some more variants of that: > > let {x, x = 2} = {x: 1}; > let {x = (function() { x = 2; })()} = {}; // error > let {x = (function() { x = 2; })()} = {x: 1}; // no error Done.
PS3 is the correct patch on top of https://codereview.chromium.org/1139773005/
Patchset #2 (id:20001) has been deleted
On 2015/05/18 18:53:26, Dmitry Lomov (chromium) wrote: > PS3 is the correct patch on top of https://codereview.chromium.org/1139773005/ (ok now it is PS2 :))
The CQ bit was checked by dslomov@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com, arv@chromium.org Link to the patchset: https://codereview.chromium.org/1146683002/#ps60001 (title: "Rebased onto ToT")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146683002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dslomov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146683002/60001
On 2015/05/19 14:26:59, rossberg wrote: > lgtm Thanks - here goes Big Destructuring Landing Party
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5cdaa3e6e5da4998c8e1351174a662b3cb5828ce Cr-Commit-Position: refs/heads/master@{#28482} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
