449811 - Create a fresh lexical binding for each iteration of for-in/for-of loops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

The following code snippet alerts "3" three times:

    for each (let a in [1, 2, 3]) {
      window.setTimeout(function() { alert(a) }, 0);
    }

I would have expected it to alert "1", then "2", then "3"; or, failing that, for "a" to be defined outside the ForStatement, which it isn't:

    for each (let a in [1, 2, 3]) {}
    alert(a); // throws "a is not defined"

Workarounds to obtain both outcomes are trivial:

    for each (let a in [1, 2, 3]) {
      let b = a;
      window.setTimeout(function() { alert(b) }, 0);
    }

    let a;
    for each (a in [1, 2, 3]) {}
    alert(a);

Nevertheless, the behavior confounds expectations.

I think it would be better if the ForInBinding (i.e. the variable "a" in these examples) were either redefined for each iteration of the ForStatement loop or defined in the scope of the block that encloses the ForStatement (preferably the former, which seems more useful).

Comment 1

[Brendan Eich [:brendan]]

This is a good point. We have a chance to do something better here. I'll try to get this spec'ed for ES-next-major (AKA ES4, ES-harmony).

/be

Comment 2

[Dave Herman [:dherman]]

I'll give this one a try.

Dave

Comment 3

[Dave Herman [:dherman]]

Okay, so what should be the behavior of:

    for (let i = 42 in f()) { ... }

and how about

    for (let i = 42 in []) { ... }

Yeesh.

Any chance we could just do away with the initialization (i.e., reject it) in the "let" case?

Dave, feeling icky

Comment 4

[Jason Orendorff [:jorendorff]]

Puh-lease do away with it in the let case.

Comment 5

[Brendan Eich [:brendan]]

If we are going to refactor the grammar, I hope we can get rid of the optional initializer for var as well as let. That means const is right out (what would be the point?).

/be

Comment 6

[Dave Herman [:dherman]]

> If we are going to refactor the grammar, I hope we can get rid of the optional
> initializer for var as well as let.

That doesn't break the web?

> That means const is right out (what would be the point?).

I thought const was being changed to have block-scoping instead of function-scoping, in which case it would be perfectly valid in for-in loops: it's just like let (fresh binding per iteration), and you can't mutate it.

Dave

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> > If we are going to refactor the grammar, I hope we can get rid of the optional
> > initializer for var as well as let.
> 
> That doesn't break the web?

Not AFAIK -- just testsuites.

> > That means const is right out (what would be the point?).
> 
> I thought const was being changed to have block-scoping instead of
> function-scoping, in which case it would be perfectly valid in for-in loops:
> it's just like let (fresh binding per iteration), and you can't mutate it.

The Harmony rule for const is that it must have an initializer, and it's an error to use before the const is initialized. If the loop iterates zero times the const is not initialized but it is also out of scope right away. Maybe this is ok but it isn't the "const must have syntactic initializer" agreement.

/be

Comment 8

[Dave Herman [:dherman]]

> > That doesn't break the web?
> 
> Not AFAIK -- just testsuites.

Great!

> The Harmony rule for const is that it must have an initializer...

Sounds like the rule could be refined a little; ISTM the intention is not to have code that gratuitously leads to uninitialized reads of const-variables, which for-const-in would not. This is probably worth bringing up in the F2F, though, so I'll add it to my slides and we can discuss.

Dave

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: v1

This applies on top of the for-of patches in bug 699565.

This fixes scoping of for-let-in, for-each-let, and for-let-of statements only.

In SpiderMonkey array comprehensions and generator expressions also suffer from (different) strange scoping rules that TC39 will be fixing up. I'll fix that in bug 469402.

I hear some members of TC39 are also eager to make for(let;;) loops generate a fresh binding each time through! Sounds great to me. Whenever I get word it's officially a done deal, I will file a new bug for that.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 591670 [details] [diff] [review]
v1

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

Nice patch.  I like the PNK_ITERNEXT trick.  I think you can kill JSOP_LEAVEFORLETIN now.  Also, you need to take a pass through JSOP_ENTERLET1, where the comments explicitly mention for-let-in and and there are some branches that can be removed.  (Not for this patch, but perhaps later: since JSOP_ENTERLET1 now only handles the switch+let weirdness, perhaps it could be renamed JSOP_ENTERSWITCHLET or removed by solving the problem differently.)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4813,5 @@
>          return false;
>  
> +    if (letDecl) {
> +        /* Leave the implicit block around the loop body. */
> +        if (!PopStatementBCE(cx, bce))

Can you assert bce->topStmt == &letStmt?

Comment 11

[Dão Gottwald [:dao]]

Jason, can you finish this patch and land it? We're hitting this in bug 802397 where we could work around it, but we really should get the underlying issue fixed since it could be causing similar subtle bugs that we're not aware of yet.

Comment 12

[Jason Orendorff [:jorendorff]]

OK, I unbitrotted this patch, but there are some bugs I'm working through -- there have been other changes to this code, in addition to luke's comments which will take some thought.

Comment 14

[Kyle Simpson]

I was hoping we could get this ES6'ish decision about let binding to for-loop iterations added to FF so we can play around and demonstrate the value of that feature.

(also, so I stop embarrassing myself during live demos in my JS workshops! ;)

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We have a separate bug for c-style for loops, is there also a separate bug for for-of style loops?

(Add devtools to the "me too" list of people getting bit by the lack of a fresh binding per iteration.)

Comment 16

[Dave Herman [:dherman]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> We have a separate bug for c-style for loops, is there also a separate bug
> for for-of style loops?

From skimming Jason's patch it looks like he was fixing it for both for-in and for-of in this bug.

Dave

Comment 17

[Marco Bonardo [:mak]]

This is still causing a bunch of subtle bugs in our code (I just hit it twice while working on a patch, and luckily I had tests, many instances are likely unknown). Any hope to get this fixed?

Comment 18

[Jeff Walden [:Waldo]]

I think there are some dups in flight, because I was planning on fixing this for both for-in and for-of pretty soon.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Now that bug 589199, this bug seems like the last big blocker to exposing let to unversioned JS. We really want to be able to do that for WebExtensions. Any progress here?

Comment 20

[Jeff Walden [:Waldo]]

Now that bug 1052139 is done, this is pretty much my highest priority.  Once my right hand finishes healing, this should happen in short order.

Comment 21

[Jeff Walden [:Waldo]]

Working on a patch now.  The substantial trickiness of this:

  for (let x in x);

and ensuring that's a TDZ violation has made this snowball a bit.  Given that, the ultimate patch here is extremely likely to fix for-in bindings, for-of bindings, and permit use of computed property names in variable declarations appearing after |for (|.  (Regrettably it's also going to be a lot bigger.  Not sure there was much I could have done to split this up harder, alas.  At least it'll be a lot more readable, I hope.)

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Tentative rollup for fuzz/smoke-testing

This seems to work for relatively basic cases, in my manual testing of stuff.  Before I assume too hard that it's reviewable, do the fuzzers put the insta-kibosh on this?

This rollup should apply atop efc63f7cf9951adcfa8603346df60e2c7df0d8f2.

Comment 24

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8704431 [details] [diff] [review]
Tentative rollup for fuzz/smoke-testing

for (var a in b) {}

js> for (var a in b) {}
Assertion failure: binding->isOp(JSOP_SETGNAME) || binding->isOp(JSOP_STRICTSETGNAME), at /Users/fuzz2/trees/mozilla-central/js/src/frontend/BytecodeEmitter.cpp:5670
Segmentation fault: 11

Comment 25

[Jason Orendorff [:jorendorff]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #24)
> js> for (var a in b) {}
> Assertion failure: binding->isOp(JSOP_SETGNAME) ||
> binding->isOp(JSOP_STRICTSETGNAME), at
> /Users/fuzz2/trees/mozilla-central/js/src/frontend/BytecodeEmitter.cpp:5670
> Segmentation fault: 11

LOLOLOLOL

Comment 26

[Christian Holler (:decoder)]

Comment on attachment 8704431 [details] [diff] [review]
Tentative rollup for fuzz/smoke-testing

Please renew the feedback? once the bug in comment 24 is fixed.

Comment 27

[Tom S [:evilpie]]

What is the status here? This is the last part of having compliant let/const bindings.

Comment 32

[Jan de Mooij [:jandem]]

Every other day we have people reporting this on IRC/bugzilla. Do we have a plan or timeline?

Comment 33

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jan de Mooij [:jandem] from comment #32)
> Every other day we have people reporting this on IRC/bugzilla. Do we have a
> plan or timeline?

The most recent status report I've seen is from waldo earlier this week in bug 1278150, comment 2.

Comment 34

[Jeff Walden [:Waldo]]

I spent most of yesterday resurrecting (i.e. mostly paging it back into my memory) the for-in portion of this in my local patch queue.  Resurrecting for-of shouldn't be too much harder.

The big problem is making the per-iteration assignments target the right things, in certain edge cases.  That was a problem with the patchwork the last time I touched it.  It will remain a problem with it now.  I had no good answer for it then; I'm not sure I have any better answer now.

(The fundamental problem is that in certain cases, the assignment *will not* target a for-in/of loop declaration if there is one.  With current def-use chain architecture, this *requires* a separate assignment target, from the one found in the declaration.  But that has very significant issues, that have made the current/previous approach to a separate assignment target have sec-critical security issues.  *Really* this wants a separation of binding representation from def-use tracking, and mapping of assignments to upvars and the like.  But possibly there's something stupid to hack together, that isn't actually gigantically security-bad.)

But maybe some big flash of inspiration can make it not a problem any more, somehow, if I'm really lucky or something.

Comment 35

[Jeff Walden [:Waldo]]

I have a PR against the Github repo/branch where bug 1263355 is being worked on, that I believe fixes this.  Don't anybody bother testing it, tho, because there are known cases (e.g. any for-in/of loop with lexical declaration whose body captures one of those lexical declarations) where the PR falls down, because the underlying branch isn't fully completed yet.

https://github.com/syg/gecko-dev/pull/1

Comment 37

[Shu-yu Guo [:shu]]

Fixed by bug 1263355.

Comment 41

[Marco Bonardo [:mak]]

Is the test here unrelated to this fix? How can it pass if this was fixed?
http://searchfox.org/mozilla-central/source/js/src/tests/ecma_6/Comprehensions/scope.js#10

Comment 42

[Jeff Walden [:Waldo]]

(In reply to Marco Bonardo [::mak] from comment #41)
> Is the test here unrelated to this fix? How can it pass if this was fixed?
> http://searchfox.org/mozilla-central/source/js/src/tests/ecma_6/
> Comprehensions/scope.js#10

Array and generator comprehensions are non-standard and are going to be removed.  It would have been a bit of extra work to "fix" their semantics.  Not worth it given the non-standardness.  So as a practical matter, the comment there is never going to be addressed, short of that test being deleted when comprehensions are removed.