[es6] emit error when for-in loop declarations are initialized in strict mode

[es6] emit error when for-in loop declarations are initialized in strict mode

The ES6 grammar forbids the initialization of variable declarations in IterationStatements.

This CL will report `for (var x = y in z)` as a SyntaxError in strict mode (as done in JSC). It is possible that this could break sites in sloppy mode, and so that change can wait.

BUG=
R=
LOG=N

Committed: https://crrev.com/1fb76f055a4a0d0a2f5e5d2784b0f070fc279b2b
Cr-Commit-Position: refs/heads/master@{#27639}

[caitp (gmail), 2015-03-24 21:33:43]

PTAL --- JavaScriptCore is doing a similar thing (emit the error only in strict
mode in for-in loops), so if they can do it without breaking the web, it should
be safe-ish.

As a bonus, the error message for a for-of loop initializer is improved
("Unexpected token of" -> the more detailed message from SpiderMonkey).

The reason I'd like this is so that developers are less likely to write code
which break on Safari (and also to improve spec compliance and developer
friendliness)

[caitp (gmail), 2015-03-24 22:02:56]

A nice follow-up to this would be improving the error message for multiple
variable declarations in a for-in/of loop lhs ---  spidermonkey's is still
confusing, but it's a bit better than "unexpected token"

[arv (Not doing code reviews), 2015-03-25 09:07:59]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-03-25 09:07:59]

"For legacy compatibility, this change must not affects loppy mode" - This is
not true. It should not be allowed in sloppy mode either.

I'm fine doing strict mode first but the description is misleading.

[arv (Not doing code reviews), 2015-03-25 09:17:43]

https://codereview.chromium.org/1033823002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1033823002/diff/1/src/messages.js#newcode201
src/messages.js:201: strict_for_in_loop_initializer: ["for-in loop variable
declaration may not have an initializer in strict mode."],
This is misleading...

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

https://codereview.chromium.org/1033823002/diff/1/src/parser.cc#newcode2221
src/parser.cc:2221: Block* result = ParseVariableDeclarations(var_context, NULL,
names, &ignore,
NULL -> nullptr

https://codereview.chromium.org/1033823002/diff/1/src/parser.cc#newcode2393
src/parser.cc:2393: if (first_initializer_loc && !initializer_recorded) {
Can you use IsValid here instead of adding the boolean?

[caitp (gmail), 2015-03-25 11:22:42]

https://codereview.chromium.org/1033823002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1033823002/diff/1/src/messages.js#newcode201
src/messages.js:201: strict_for_in_loop_initializer: ["for-in loop variable
declaration may not have an initializer in strict mode."],
On 2015/03/25 09:17:43, arv wrote:
> This is misleading...

I know it's not technically allowed in sloppy mode, but I don't have telemetry
to know if it would break anyone. I'm more confident with strict-mode only (for
now) because of JSC is already doing that.

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

https://codereview.chromium.org/1033823002/diff/1/src/parser.cc#newcode2221
src/parser.cc:2221: Block* result = ParseVariableDeclarations(var_context, NULL,
names, &ignore,
On 2015/03/25 09:17:43, arv wrote:
> NULL -> nullptr

Acknowledged.

https://codereview.chromium.org/1033823002/diff/1/src/parser.cc#newcode2393
src/parser.cc:2393: if (first_initializer_loc && !initializer_recorded) {
On 2015/03/25 09:17:43, arv wrote:
> Can you use IsValid here instead of adding the boolean?

Acknowledged.

[caitp (gmail), 2015-03-26 00:50:48]

caitpotter88@gmail.com changed reviewers:
+ marja@chromium.org

[arv (Not doing code reviews), 2015-03-31 14:39:15]

LGTM

https://codereview.chromium.org/1033823002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1033823002/diff/20001/src/parser.cc#newcode3384
src/parser.cc:3384: (is_strict(language_mode()) || mode ==
ForEachStatement::ITERATE)) {
Add a TODO that this should be an error in sloppy mode too

https://codereview.chromium.org/1033823002/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1033823002/diff/20001/src/preparser.cc#newcod...
src/preparser.cc:751: (is_strict(language_mode()) || mode ==
ForEachStatement::ITERATE)) {
here too

[marja, 2015-04-07 08:30:34]

You could add a test-parsing test too.

https://codereview.chromium.org/1033823002/diff/40001/src/parser.cc
File src/parser.cc (left):

https://codereview.chromium.org/1033823002/diff/40001/src/parser.cc#oldcode2251
src/parser.cc:2251: 
Nit: this was probably an accidental deletion.

https://codereview.chromium.org/1033823002/diff/40001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1033823002/diff/40001/src/parser.cc#newcode2234
src/parser.cc:2234: VariableDeclarationProperties* decl_props,
Isn't decl_props now unnecessary? If not, why not?

(decl_props is set to kHasInitializers iff first_initializer_loc is set to
something valid.)

https://codereview.chromium.org/1033823002/diff/40001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1033823002/diff/40001/src/preparser.cc#newcod...
src/preparser.cc:427: VariableDeclarationProperties* decl_props, int* num_decl,
Ditto

https://codereview.chromium.org/1033823002/diff/40001/src/preparser.cc#newcod...
src/preparser.cc:735: ForEachStatement::VisitMode mode;
Why this change?

[caitp (gmail), 2015-04-07 11:56:20]

Thanks, making these edits

https://codereview.chromium.org/1033823002/diff/40001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1033823002/diff/40001/src/parser.cc#newcode2234
src/parser.cc:2234: VariableDeclarationProperties* decl_props,
On 2015/04/07 08:30:34, marja wrote:
> Isn't decl_props now unnecessary? If not, why not?
> 
> (decl_props is set to kHasInitializers iff first_initializer_loc is set to
> something valid.)

Yes, it can be removed

https://codereview.chromium.org/1033823002/diff/40001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1033823002/diff/40001/src/preparser.cc#newcod...
src/preparser.cc:735: ForEachStatement::VisitMode mode;
On 2015/04/07 08:30:34, marja wrote:
> Why this change?

It matches the version of this code in parser.cc, and makes the line-wrapping a
bit easier on the eyes

[caitp (gmail), 2015-04-07 14:58:58]

I think a test-parsing patch should wait until a followup.

Right now, the preparser and parser have different mechanisms for deciding to
accept an "in" token or not, and it's a bit challenging to make them match. As a
result of this, there's a different error message between the preparser and
parser in certain cases.

What I'd like to do in a followup patch, is remove the conditions which
determine whether in/of is accepted, and just report better errors in the case
where it's not accepted (because, when it's not accepted and it shows up, it's
going to be a SyntaxError anyways, just a harder one to grok).

Building that into this one patch is doable too, but it just makes it bigger and
messier. What do you think? Preliminary version and then a followup simplifying
it, or just fix it all up now?

[marja, 2015-04-07 15:04:02]

It's ok to add it as a follow up, to keep the patches small.

[marja, 2015-04-07 15:05:53]

lgtm

[caitp (gmail), 2015-04-07 18:39:49]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-04-07 18:39:49]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1033823002/#ps80001
(title: "delete decl_props")

[commit-bot: I haz the power, 2015-04-07 18:40:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1033823002/80001

[caitp (gmail), 2015-04-07 18:40:25]

On 2015/04/07 15:05:53, marja wrote:
> lgtm

followup is ready, gonna send CL after this lands (if this lands)

[commit-bot: I haz the power, 2015-04-07 19:28:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-07 19:28:53]

Patchset 5 (id:??) landed as
https://crrev.com/1fb76f055a4a0d0a2f5e5d2784b0f070fc279b2b
Cr-Commit-Position: refs/heads/master@{#27639}

[caitp (gmail), 2015-05-20 14:05:13]

On 2015/04/07 19:28:53, I haz the power (commit-bot) wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/1fb76f055a4a0d0a2f5e5d2784b0f070fc279b2b
> Cr-Commit-Position: refs/heads/master@{#27639}

https://bugzilla.mozilla.org/show_bug.cgi?id=1164741 and
https://bugs.webkit.org/show_bug.cgi?id=130269 seem to have partially reverted
the restrictions on initializers in ForIn. We might want to do that too.

:(