Closed Bug 748550 Opened 12 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: brendan, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files)

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
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
(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
(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
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
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla15
Attached patch proposed patchSplinter Review
Will think about adding a test for SyntaxError. Really want to get this into nightlies after mozilla14 branches.

/be
Attachment #618098 - Flags: review?(jorendorff)
Fuzzer maintainers take note.

/be
(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
Attachment #618098 - Flags: review?(jorendorff) → review+
Jason: is Brendan's for-in patch still relevant? You r+'d his patch in 2012, but he never landed it.
Blocks: es6
Flags: needinfo?(jorendorff)
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: mozilla15 → ---
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
Depends on: 987889
Not that I've seen come up in the Tech Evanglism component.
(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)
Mike's regex only matches single-letter variable names, so there could be more broken sites.
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]
Attachment #8587769 - Flags: review?(jorendorff)
Assignee: brendan → jwalden+bmo
Attachment #8587769 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/dfdcce6b319a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1156624
Depends on: 1162249
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: