[es6] Implement proper TDZ for parameters

[es6] Implement proper TDZ for parameters

Previously, examples like (({a = x}, x) => {})({}, 0) did not throw a ReferenceError like they should. This CL

- Splits up DeclareFormalParameters such that the formals can be recorded first and declared later.

- Declaration then takes the complete parameter list into account. If it is not simple, temporaries are introduced for all parameters.

- BuildParameterInitializationBlock desugars all parameters from non-simple lists into let-bindings.

- Refactored Pre/ParserFormalParameters, so that the arity information is no longer duplicated in Parser.

- Rest is currently handled specially, until rest-via-destructuring has landed.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:811
LOG=N

Committed: https://crrev.com/4273f66e9882f1239c6b7846c31730d47b870d81
Cr-Commit-Position: refs/heads/master@{#30025}

[adamk, 2015-08-03 18:43:52]

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode1205
src/parser.cc:1205: ParseFormalParameter(is_rest, &formals, &formals_classifier,
&ok);
Check ok before declaring?

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode2222
src/parser.cc:2222: !scope_->is_declaration_scope() ? LET : VAR;
This looks unrelated to the rest of the change; maybe it's further fixup from
the broadening of declaration scope definition?

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode3917
src/parser.cc:3917: for (int i = 0; i < parameters->arity; ++i) {
Seems scary to use arity for this instead of the params. As in the other change,
this separation of arity handling from the actual list of parameters feels
brittle, and I'd feel better if there were something other than a DCHECK keeping
the arity and params.size() in lock-step.

https://codereview.chromium.org/1259283002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1259283002/diff/1/src/parser.h#newcode559
src/parser.h:559: DCHECK(arity == params.length());
This code could be responsible for keeping arity in sync for the Parser version,
letting the preparser handle the incrementing in AddFormalParameter (see my
comment in preparser.h).

A more robust alternative: make the |arity| member private in the superclass
(arity_), and add an arity() getter which in the PreParser class returns that
private member and in this version returns params.length().

https://codereview.chromium.org/1259283002/diff/1/src/parser.h#newcode1331
src/parser.h:1331: DCHECK(parameters->arity == parameters->params.length());
Nit: DCHECK_EQ

https://codereview.chromium.org/1259283002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1259283002/diff/1/src/preparser.h#newcode1644
src/preparser.h:1644: bool is_rest) {}
One option that would make this thing slightly less brittle is if this
implementation incremented the arity, and the Parser one did the same. At the
least, it would make the Parser implementation a bit better-factored.

[rossberg, 2015-08-04 15:31:19]

Addressed the arity issue by separating out a FormalParametersBase class that
does not have an arity field, so that it only exists in the preparser. It is now
updated in PreParserTraits::AddFormalParameter.

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode1205
src/parser.cc:1205: ParseFormalParameter(is_rest, &formals, &formals_classifier,
&ok);
On 2015/08/03 18:43:51, adamk wrote:
> Check ok before declaring?

Done.

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode2222
src/parser.cc:2222: !scope_->is_declaration_scope() ? LET : VAR;
On 2015/08/03 18:43:51, adamk wrote:
> This looks unrelated to the rest of the change; maybe it's further fixup from
> the broadening of declaration scope definition?

You are right, I overlooked this one when splitting up the CL. It is needed
because there was some test started failing with this CL otherwise. I can
separate it out if you want.

https://codereview.chromium.org/1259283002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1259283002/diff/1/src/parser.h#newcode1331
src/parser.h:1331: DCHECK(parameters->arity == parameters->params.length());
On 2015/08/03 18:43:52, adamk wrote:
> Nit: DCHECK_EQ

Done.

[caitp (gmail), 2015-08-04 16:46:26]

These shouldn't block anything, just wondering

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode556
src/parser.h:556: const Parameter& at(int i) const { return params[i]; }
minor question: Is there a sort of system in place for deciding whether to name
methods with PascalCase vs camelCase vs snake_case? All three seem to be used in
V8, sometimes within the same classes (as here). The style-guide doesn't seem
super clear on this.

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode1319
src/parser.h:1319: parser_->allow_harmony_rest_parameters() || is_simple);
since these are just DCHECKs, why not make these more robust? Ensure the rest
flag is used for rest, destructuring flag used for patterns. Might avoid some
false DCHECK successes?

[adamk, 2015-08-04 17:25:04]

lgtm

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode2222
src/parser.cc:2222: !scope_->is_declaration_scope() ? LET : VAR;
On 2015/08/04 15:31:19, rossberg wrote:
> On 2015/08/03 18:43:51, adamk wrote:
> > This looks unrelated to the rest of the change; maybe it's further fixup
from
> > the broadening of declaration scope definition?
> 
> You are right, I overlooked this one when splitting up the CL. It is needed
> because there was some test started failing with this CL otherwise. I can
> separate it out if you want.

Given that the rest of this change is needed to exercise it (apparently), I
guess splitting it out is more trouble than it's worth. Just keep an eye on it
if this change has to be reverted for other reasons.

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode556
src/parser.h:556: const Parameter& at(int i) const { return params[i]; }
On 2015/08/04 16:46:25, caitp wrote:
> minor question: Is there a sort of system in place for deciding whether to
name
> methods with PascalCase vs camelCase vs snake_case? All three seem to be used
in
> V8, sometimes within the same classes (as here). The style-guide doesn't seem
> super clear on this.

camelCase shouldn't be used in C++ (and I can't think of a place where it is),
though we do use it in JS. For the other two, see this bit of the Google C++
style guide:

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

Short answer: trivial inlined members (such as getters/setters, or this at()
case) may have snake_case names (the idea being to signal that they are
trivial). Any non-trivial method uses PascalCase.

https://codereview.chromium.org/1259283002/diff/40001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1259283002/diff/40001/src/preparser.h#newcode...
src/preparser.h:1329: int Arity() const { return arity; }
This upper/lower thing is a bit confusing now, but I like the abstraction
better. Perhaps this will actually become more classlike in the future to make
the naming less wonky.

[rossberg, 2015-08-04 19:09:21]

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode556
src/parser.h:556: const Parameter& at(int i) const { return params[i]; }
On 2015/08/04 17:25:04, adamk wrote:
> On 2015/08/04 16:46:25, caitp wrote:
> > minor question: Is there a sort of system in place for deciding whether to
> name
> > methods with PascalCase vs camelCase vs snake_case? All three seem to be
used
> in
> > V8, sometimes within the same classes (as here). The style-guide doesn't
seem
> > super clear on this.
> 
> camelCase shouldn't be used in C++ (and I can't think of a place where it is),
> though we do use it in JS. For the other two, see this bit of the Google C++
> style guide:
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names
> 
> Short answer: trivial inlined members (such as getters/setters, or this at()
> case) may have snake_case names (the idea being to signal that they are
> trivial). Any non-trivial method uses PascalCase.

What Adam said. And yes, V8 is rather inconsistent with this convention. OTOH,
I'm not even convinced the convention makes a lot of sense.

https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode1319
src/parser.h:1319: parser_->allow_harmony_rest_parameters() || is_simple);
On 2015/08/04 16:46:25, caitp wrote:
> since these are just DCHECKs, why not make these more robust? Ensure the rest
> flag is used for rest, destructuring flag used for patterns. Might avoid some
> false DCHECK successes?

Good idea. Will incorporate that into the next CL.

[rossberg, 2015-08-04 19:09:29]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-08-04 19:09:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259283002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259283002/40001

[commit-bot: I haz the power, 2015-08-04 19:53:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-04 19:53:58]

Try jobs failed on following builders:
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/8284)

[rossberg, 2015-08-05 11:36:35]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-08-05 11:36:36]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1259283002/#ps60001
(title: "Adjust test expectations")

[commit-bot: I haz the power, 2015-08-05 11:36:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259283002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259283002/60001

[commit-bot: I haz the power, 2015-08-05 12:00:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-05 12:02:33]

Patchset 4 (id:??) landed as
https://crrev.com/4273f66e9882f1239c6b7846c31730d47b870d81
Cr-Commit-Position: refs/heads/master@{#30025}

[caitp (gmail), 2015-08-05 12:53:53]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-08-05 12:53:54]

Didn't notice last night, but don't want to change that in the rest CL

https://codereview.chromium.org/1259283002/diff/60001/src/parser.h
File src/parser.h (left):

https://codereview.chromium.org/1259283002/diff/60001/src/parser.h#oldcode1320
src/parser.h:1320: const AstRawString* name = is_simple
Another piece of potential cleanup, the name-picking ternary expression isn't
really needed, because AddFormalParameter() stores the empty string for
non-simple parameters anyways.