Last Comment Bug 449811 - Create a fresh lexical binding for each iteration of for-in/for-of loops
: Create a fresh lexical binding for each iteration of for-in/for-of loops
Status: NEW
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement with 13 votes (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 1233089 1268812 1273865 1279251 1279735 (view as bug list)
Depends on: 1237452 1135167 1227677 1232674 1233249 1237449
Blocks: es6:let 1094995 1101653 802397 1230148
  Show dependency treegraph
 
Reported: 2008-08-08 12:12 PDT by Myk Melez [:myk] [@mykmelez]
Modified: 2016-06-22 09:57 PDT (History)
55 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (23.06 KB, patch)
2012-01-25 17:50 PST, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Review
Tentative rollup for fuzz/smoke-testing (55.64 KB, patch)
2016-01-05 17:17 PST, Jeff Walden [:Waldo] (remove +bmo to email)
gary: feedback-
Details | Diff | Review

Description Myk Melez [:myk] [@mykmelez] 2008-08-08 12:12:40 PDT
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] 2008-08-08 15:04:41 PDT
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] 2010-07-02 17:10:39 PDT
I'll give this one a try.

Dave
Comment 3 Dave Herman [:dherman] 2010-07-12 17:10:47 PDT
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] 2010-07-27 11:13:35 PDT
Puh-lease do away with it in the let case.
Comment 5 Brendan Eich [:brendan] 2010-07-27 18:13:56 PDT
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] 2010-07-27 21:16:42 PDT
> 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] 2010-07-28 00:36:27 PDT
(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] 2010-07-28 04:45:45 PDT
> > 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] 2012-01-25 17:50:13 PST
Created attachment 591670 [details] [diff] [review]
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] 2012-01-30 10:25:20 PST
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] 2012-10-21 23:53:53 PDT
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] 2012-10-22 18:18:45 PDT
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 13 Tom Schuster [:evilpie] 2013-03-22 16:02:30 PDT
*** Bug 854037 has been marked as a duplicate of this bug. ***
Comment 14 Kyle Simpson 2013-03-22 16:06:03 PDT
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] [⏰PDT; UTC-7] 2015-03-10 14:21:40 PDT
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] 2015-03-12 08:20:39 PDT
(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] 2015-04-24 01:17:31 PDT
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] (remove +bmo to email) 2015-08-11 13:28:52 PDT
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 (:billm) 2015-10-18 08:27:45 PDT
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] (remove +bmo to email) 2015-10-18 14:21:39 PDT
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] (remove +bmo to email) 2015-11-12 21:42:25 PST
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 22 Tom Schuster [:evilpie] 2015-12-16 09:07:58 PST
*** Bug 1233089 has been marked as a duplicate of this bug. ***
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2016-01-05 17:17:52 PST
Created attachment 8704431 [details] [diff] [review]
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] 2016-01-06 15:46:21 PST
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] 2016-01-21 12:23:35 PST
(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) 2016-03-07 07:14:50 PST
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 Schuster [:evilpie] 2016-03-11 05:49:58 PST
What is the status here? This is the last part of having compliant let/const bindings.
Comment 28 Tom Schuster [:evilpie] 2016-04-29 09:18:57 PDT
*** Bug 1268812 has been marked as a duplicate of this bug. ***
Comment 29 Ben Kelly [:bkelly] 2016-05-18 09:11:48 PDT
*** Bug 1273865 has been marked as a duplicate of this bug. ***
Comment 30 Jeff Walden [:Waldo] (remove +bmo to email) 2016-06-09 11:04:37 PDT
*** Bug 1279251 has been marked as a duplicate of this bug. ***
Comment 31 Tom Schuster [:evilpie] 2016-06-11 14:12:35 PDT
*** Bug 1279735 has been marked as a duplicate of this bug. ***
Comment 32 Jan de Mooij [:jandem] 2016-06-11 14:44:57 PDT
Every other day we have people reporting this on IRC/bugzilla. Do we have a plan or timeline?
Comment 33 Myk Melez [:myk] [@mykmelez] 2016-06-11 15:23:57 PDT
(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] (remove +bmo to email) 2016-06-11 15:43:23 PDT
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] (remove +bmo to email) 2016-06-22 09:57:37 PDT
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

Note You need to log in before you can comment on or make changes to this bug.