1164741 - Add back partial support for |for (var i = 0 in obj);| syntax, ignoring the initializer rather than failing on it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Instead of removing the syntax completely just yet (bug 748550), let's just try ignoring the initializer instead.  That'll give a little more time for sites that used this to fix their code.  (I have yet to see a site that actually used this deliberately, expecting its semantics.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Ignore the initializer

Relevant context for reviewing this consists of https://hg.mozilla.org/mozilla-central/rev/dfdcce6b319a which removed |for (var i = 0 in foo)| syntax.  The tests added are almost entirely readding all the test changes made in that rev -- except to a new test file, to ease removal at some future time.  (Plus a couple other tests for over-removal in that rev, and for a testcase I accidentally made assert at one point while writing this test.)

Requesting double the reviews because jorendorff looked at the original thing, and because efaust reviewed the grammar parametrization work that's going to come into play with this in bugwork to be posted Very Soon, to get rid of parsingForInit.  Plus it probably doesn't hurt, for a bug to backport to the branch cut earlier this week.

Comment 2

[André Bargull [:anba]]

JSC also reverted their for-in initializer changes for compatibility reasons: https://bugs.webkit.org/show_bug.cgi?id=130269. In JSC the initializer is only allowed in for-in loops without a destructuring pattern and the initializer expression itself is ignored:
---
>>> for (var a = print("never executed") in {p: 0});
undefined

>>> for (var [a] = 0 in {p: 0});
Exception: SyntaxError: Unexpected keyword 'in'. Cannot use initialiser syntax when binding to a pattern during enumeration.

>>> for (var {a} = 0 in {p: 0});
Exception: SyntaxError: Unexpected keyword 'in'. Cannot use initialiser syntax when binding to a pattern during enumeration.

>>> for (var [a] = 0 of [0]);
Exception: SyntaxError: Unexpected identifier 'of'. Cannot use initialiser syntax when binding to a pattern during enumeration.

>>> for (var {a} = 0 of [0]);
Exception: SyntaxError: Unexpected identifier 'of'. Cannot use initialiser syntax when binding to a pattern during enumeration.

>>> for (var a = 0 of [0]);
Exception: SyntaxError: Unexpected identifier 'of'. Cannot use initialiser syntax in a for-of enumeration.
---

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Altered to JSC semantics (for now)

Yeah, it's reasonable to prohibit this for destructuring patterns.  Gave me an opportunity to make this warn, too, when a name's used.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8606059 [details] [diff] [review]
Altered to JSC semantics (for now)

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

r=me

::: js/src/frontend/Parser.cpp
@@ +3928,5 @@
> +                // |for (var foo = ... in ...);|.  This was Dumb and permitted
> +                // only because of grammar Dumb, and ES6 doesn't include it.
> +                // Make this a syntax error when the variable name is a
> +                // destructuring pattern, so that code using new features can't
> +                // use this fatuity.

This seems a little long and editorial to me; if you want, you could just write:

    // Ban the nonsensical |for (var V = E1 in E2);| where V is a destructuring
    // pattern. See bug 1164741 for background.

@@ +4001,5 @@
> +                // looking in the console knows to fix this.  Initializer
> +                // semantics were defined by ES3-5 but abandoned in ES6, and
> +                // handling them in this case is incredibly difficult for
> +                // extraordinarily little gain: just pretend the initializer
> +                // isn't there.

The last sentence here could be shortened too.

::: js/src/tests/ecma_6/extensions/for-in-with-assignments.js
@@ +16,5 @@
> + **************/
> +
> +// This is a total grab-bag of junk originally in tests changed when this
> +// syntax was removed.  Leaving it all in one file will make it easier to
> +// eventually remove.  Avert your eyes!

"I looked at the trap, Ray"

@@ +62,5 @@
> +
> +/******************************************************************************/
> +
> +with (0)
> +  for (var b = 0 in 0);  // don't assert in parser

Add a test or two for the new initializer-ignoring behavior, please.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8606059 [details] [diff] [review]
Altered to JSC semantics (for now)

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

I second Jason's comments about needing another test and shortening that comment. The rest looks good.

::: js/src/frontend/Parser.cpp
@@ +3928,5 @@
> +                // |for (var foo = ... in ...);|.  This was Dumb and permitted
> +                // only because of grammar Dumb, and ES6 doesn't include it.
> +                // Make this a syntax error when the variable name is a
> +                // destructuring pattern, so that code using new features can't
> +                // use this fatuity.

"Gee, Jeff. Tell us how you really feel"

::: js/src/tests/ecma_6/Statements/for-in-with-destructuring-assignments.js
@@ +25,5 @@
> +
> +/******************************************************************************/
> +
> +assertThrowsInstanceOf(function() {
> +  // Abandon all hope, ye who try to read this.

Good lord.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> This seems a little long and editorial to me; if you want, you could just
> write:
> The last sentence here could be shortened too.

Yeah, probably.  But I liked "fatuity" so much here!  :-)

> Add a test or two for the new initializer-ignoring behavior, please.

That file does contain a couple tests that exercise/check this.  I'm shocked, shocked that you missed them.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9841c5d229b8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9841c5d229b8

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8606059 [details] [diff] [review]
Altered to JSC semantics (for now)

Approval Request Comment
[Feature/regressing bug #]: bug 748550
[User impact if declined]: an extraordinarily small number of sites might not work, or work quite as desired
[Describe test coverage new/current, TreeHerder]: tests in patch
[Risks and why]: Relevant parsing code is a little finicky logic-wise, but it tracks out, and at least one reviewer noticed this tracking-out.  It should be solid on that front.
[String/UUID change made/needed]: N/A

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8606059 [details] [diff] [review]
Altered to JSC semantics (for now)

Taking it because webcompat matters.

Even if the patch is a bit big in a critical section, it is early in the aurora cycle and we would have time to find and fix potential regressions.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b852ee9e576f

Comment 12

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#Initializer_expressions
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Initializer_in_for-of.2Fin_loop_head_declaration_is_no_longer_allowed
https://developer.mozilla.org/en-US/Firefox/Releases/40#JavaScript