748550 - Remove InitialiserNoIn[opt] from ... in for(var ... in obj) to help simplify ES6

Status: RESOLVED FIXED

Description

[Brendan Eich [:brendan]]

From ES5.1, 12.6.4 "The for-in Statement":

The production

IterationStatement : for ( var VariableDeclarationNoIn in Expression ) Statement

is evaluated as follows:

1. Let varName be the result of evaluating VariableDeclarationNoIn.
2. Let exprRef be the result of evaluating the Expression.
[... etc.]

VariableDeclarationNoIn is from 12.2 "Variable Statement":

  VariableDeclarationNoIn :
    Identifier InitialiserNoIn[opt]

This goes back to ES1, possibly it's a JScript-ism. We believe it is unwanted. It is a royal pain to compile all the new ES6 variants (let and const) and the initialiser is useless: either invisible except for effects if the loop iterates zero times, or overwritten in the loop-local let or const binding by the iterated vale.

We definitely do not want for-of to have this botch (in any form, including the paren-free variant of for-of in comprehensions and generator expressions).

This bug is about trying in nightly builds and beyond, as JSC folks have been doing, to remove the old bad thing to see what breaks.

/be

Comment 1

[Allen Wirfs-Brock]

Note that the current ES6 draft uses the following production:

IterationStatement : for ( var BindingIdentifer in Expression ) Statement 

and that for this case the binding is not allowed to be a destructuring pattern because the iteration values are strings which don't make a lot of sense to destructure.

for-of permits either a destructuring pattern or BindingIdentifier

Comment 2

[Brendan Eich [:brendan]]

(In reply to Allen Wirfs-Brock from comment #1)
> Note that the current ES6 draft uses the following production:
> 
> IterationStatement : for ( var BindingIdentifer in Expression ) Statement 
> 
> and that for this case the binding is not allowed to be a destructuring
> pattern because the iteration values are strings which don't make a lot of
> sense to destructure.

Thanks -- of course, JS1.8+ allow such destructuring patterns as [c1, c2] and {length}, but I will forbid them in the patch for this bug, in the interest of not deviating from draft ES6. I'm going to have to disable or remove some tests...

/be

Comment 3

[Brendan Eich [:brendan]]

(In reply to Allen Wirfs-Brock from comment #1)
> Note that the current ES6 draft uses the following production:
> 
> IterationStatement : for ( var BindingIdentifer in Expression ) Statement 

No for-let-in? We support that (destructuring as well; separate issue). We've talked about it having per-loop-iteration let bindings.

/be

Comment 4

[Brendan Eich [:brendan]]

Actually, I take back comment 2 -- one issue per bug is best, I'll leave removing for (var {...} in ...); and for (var [...] in ...); for another bug.

/be

Comment 5

[Brendan Eich [:brendan]]

Created attachment 618098 [details] [diff] [review]
proposed patch

Will think about adding a test for SyntaxError. Really want to get this into nightlies after mozilla14 branches.

/be

Comment 6

[Brendan Eich [:brendan]]

Fuzzer maintainers take note.

/be

Comment 7

[Allen Wirfs-Brock]

(In reply to Brendan Eich [:brendan] from comment #3)
> (In reply to Allen Wirfs-Brock from comment #1)
> > Note that the current ES6 draft uses the following production:
> > 
> > IterationStatement : for ( var BindingIdentifer in Expression ) Statement 
> 
> No for-let-in? We support that (destructuring as well; separate issue).
> We've talked about it having per-loop-iteration let bindings.

Sure, but since it is new syntax (for ES) a syntax change isn't required to remove the initializer.  In ES it is just born without it.

I just did some digging on the discussion concerning all of this on es-discuss.  It looks to like  we concluded we would allow destructuring  in the binding position of all for-in declarations.  That means I still have some spec. cleanup to do do, but the grammar for for-in should be

IterationStatement :
   ...
   for ( var ForBinding in Expression ) Statement
   for ( ForDeclaration in Expression ) Statement
   ...
ForBinding :
   BindingIdentier
   BindingPattern
ForDeclaration : LetOrConst ForBinding

Comment 8

[Chris Peterson [:cpeterson]]

Jason: is Brendan's for-in patch still relevant? You r+'d his patch in 2012, but he never landed it.

Comment 9

[Brendan Eich [:brendan]]

The Apple JSC guys bravely took the lead on finding out that this change broke the web. But the known offending site has been evangelized (thanks @miketaylr) and fixed (bug 987889).

Mike, any more like bug 987889 that should block this bug?

/be

Comment 10

[Mike Taylor [:miketaylr] (62 Regression Engineering Owner)]

Not that I've seen come up in the Tech Evanglism component.

Comment 11

[Mike Taylor [:miketaylr] (62 Regression Engineering Owner)]

(In reply to Mike Taylor [:miketaylr] from comment #10)
> Not that I've seen come up in the Tech Evanglism component.

Sorry, commented without thinking too much. There's not much reason for these issues to be reported here yet. ^_^

I grepped over my corpus of the top ~64,621 Alexa sites with JS inlined and got the following matches in 11 sites:

egrep -or "for *\( *var . *= *. in ." .

./01/telegraph.co.uk.html.txt:for ( var i = 0 in this._listeners[event].eventFunc) {
./31/smbcnikko.co.jp.html.txt:for (var i=0 in ar){if(sc_referrer.indexOf(ar[i]) > -1) {
./31/smbcnikko.co.jp.html.txt:for (var i=0 in ar){if(sc_referrer.indexOf(ar[i]) > -1) {
./31/smbcnikko.co.jp.html.txt:for (var i=0 in ar){if(sc_referrer.indexOf(ar[i]) > -1) {
./31/smbcnikko.co.jp.html.txt:for (var i=0 in ar){if(tmp_host.match(ar[i])) {
./31/smbcnikko.co.jp.html.txt://    for (var i=0 in ar){if(sc_referrer.indexOf(ar[i]) > -1) {
./3b/public.fr.data.txt:for(var i=0 in data)
./42/parismatch.com.data.txt:for(var i=0 in data)
./4a/asahibeer.co.jp.html.txt:for (var i=0 in ar)
./5e/jijikala.ir.html.txt:for (var i = 0 in listenersObj[dataFromPage.topic])
./64/globalnews.ca.html.txt:for (var i=0 in this.mapObjs)
./81/oddsportal.com.html.txt:for(var i=0 in allowed)
./8e/premiere.fr.data.txt:for(var i=0 in data)
./99/busyteacher.org.html.txt:for (var i=0 in obj)
./cc/prokopievsk.ru.html.txt:for(var i=0 in where)

Comment 12

[Jesse Ruderman]

Mike's regex only matches single-letter variable names, so there could be more broken sites.

Comment 13

[Jeff Walden [:Waldo]]

Created attachment 8587769 [details] [diff] [review]
Updated patch, minus any new test-fixes for trunk

This is largely the previous patch here, rebased to trunk.  Running tests locally now to see what other tests will need updates for this syntax removal.  This was semi-triggered by a #jslang discussion -- it sounds like v8 and Chakra are both about to get on this, so the timing is good.

[2015-04-02 15:42:12 EDT]
<caitp-> speaking of reporting errors on other peoples bugs, is it cool if I land the for-in initializer thing before next week?
<bterlson> making the initializer a syntax error?
<caitp-> yeah
<bterlson> yes please
<bterlson> land asap
<bterlson> we will follow
<bterlson> Waldo loves for-in initializers though so probably SM won't change ;)
<Waldo> I will hurt you
* bterlson ducks
<Waldo> :-D
<caitp-> jsc is already doing almost the right thing, so it's probably safe-ish
<bterlson> aren't they just ignoring the initializer?
<Waldo> that's pretty high on my list of things to do as well, actually
<caitp-> they throw in strict mode
<bterlson> ahh
<bterlson> caitp-/Waldo: have bugs for your work to remove this garbage?
<caitp-> i can't remember if I filed one, lets see..
<Waldo> likewise
<caitp-> no bug, but it was annoying me that people kept accidentally writing things that broke in safari
<bterlson> alright np
<bterlson> thanks for checking :)
<Waldo> https://bugzilla.mozilla.org/showdependencytree.cgi?id=694100&hide_resolved=1 looks not to have such a bug filed
<Waldo> I'll file one now, I need the bug-number hook soon anyway
<Waldo> https://bugzilla.mozilla.org/show_bug.cgi?id=748550 FYI
<Waldo> don't ask me how it is we have/had a patch and never landed it :-)
<caitp-> my version of that didn't get to delete nearly as many lines of code :(
<Waldo> that's probably because your engine's not so convoluted as some :-)
<caitp-> or more convoluted, since it involves adding a lot of code to figure out if there was an initializer to begin with
[2015-04-02 17:08:05 EDT]

Comment 14

[Jeff Walden [:Waldo]]

Created attachment 8588054 [details] [diff] [review]
And the rest of the test-fixes

The patches together are green on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc6020164859

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdcce6b319a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfdcce6b319a

Comment 17

[Florian Scholz [:fscholz] (MDN)]

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

(clearing the ni, too, as the question looks solved)