Closed Bug 449811 Opened 16 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51

People

(Reporter: myk, Assigned: Waldo)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
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
Assignee: general → brendan
I'll give this one a try.

Dave
Assignee: brendan → 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
Puh-lease do away with it in the let case.
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
> 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
(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
> > 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
Summary: redefine ForInBinding for each iteration of ForStatement loop → Create a fresh ForInBinding for each iteration of ForStatement loop
Assignee: dherman → jorendorff
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #591670 - Flags: review?(luke)
Blocks: es6
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?
Attachment #591670 - Flags: review?(luke) → review+
Blocks: 802397
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.
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.
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! ;)
Blocks: 1101653
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.)
(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
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?
No longer blocks: es6
I think there are some dups in flight, because I was planning on fixing this for both for-in and for-of pretty soon.
Assignee: jorendorff → jwalden+bmo
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?
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.
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.)
Depends on: 1227677
Depends on: 1232674
Depends on: 1233249
Summary: Create a fresh ForInBinding for each iteration of ForStatement loop → Create a fresh lexical binding for each iteration of for-in/for-of loops
Blocks: 1094995
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.
Attachment #591670 - Attachment is obsolete: true
Attachment #8704431 - Flags: feedback?(gary)
Blocks: 1230148
Depends on: 1237449
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
Attachment #8704431 - Flags: feedback?(gary) → feedback-
(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 on attachment 8704431 [details] [diff] [review]
Tentative rollup for fuzz/smoke-testing

Please renew the feedback? once the bug in comment 24 is fixed.
Attachment #8704431 - Flags: feedback?(choller)
What is the status here? This is the last part of having compliant let/const bindings.
Flags: needinfo?(jwalden+bmo)
No longer blocks: 1268812
Every other day we have people reporting this on IRC/bugzilla. Do we have a plan or timeline?
(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.
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.
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
Flags: needinfo?(jwalden+bmo)
Fixed by bug 1263355.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Blocks: 1323685
Flags: needinfo?(shu)
Target Milestone: --- → mozilla51
(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.
Flags: needinfo?(shu)
You need to log in before you can comment on or make changes to this bug.