Closed Bug 1164741 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(1 file, 1 obsolete file)

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.)
Attached patch Ignore the initializer (obsolete) — Splinter Review
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.
Attachment #8605609 - Flags: review?(jorendorff)
Attachment #8605609 - Flags: review?(efaustbmo)
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.
---
Yeah, it's reasonable to prohibit this for destructuring patterns.  Gave me an opportunity to make this warn, too, when a name's used.
Attachment #8605609 - Attachment is obsolete: true
Attachment #8605609 - Flags: review?(jorendorff)
Attachment #8605609 - Flags: review?(efaustbmo)
Attachment #8606059 - Flags: review?(jorendorff)
Attachment #8606059 - Flags: review?(efaustbmo)
Whiteboard: [DocArea=JS]
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.
Attachment #8606059 - Flags: review?(jorendorff) → review+
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.
Attachment #8606059 - Flags: review?(efaustbmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/9841c5d229b8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
Attachment #8606059 - Flags: approval-mozilla-aurora?
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.
Attachment #8606059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.