1216623 - ES6 scoping: In `for(let x=EXPR;;)`, x is not visible in EXPR

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jeff Walden [:Waldo]]

Uh, I'd prefer to take this and deal with all the for-loop stuff at once.  Do you mind?  The monolith is not easily split up, and everything would benefit from correcter handling of for-loop scoping.

Comment 2

[André Bargull [:anba]]

bug 1069480 contains this and other examples where for and for-in/of loop scoping does not match ES2015. See the "testForRhs()" function in bug 1069480.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: In `for (let ...)` loops, evaluate initializers in the scope of the variables being initialized

Comment 4

[Jason Orendorff [:jorendorff]]

This patch may not make any sense without the epic refactoring of BytecodeEmitter::emitVariables() in bug 1217001.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Uh, I'd prefer to take this and deal with all the for-loop stuff at once. 
> Do you mind?  The monolith is not easily split up, and everything would
> benefit from correcter handling of for-loop scoping.

I didn't see this until I had already uploaded the patch -- honest!

The way I split it was to fix the scoping in this bug, across for(;;), for-in, and for-of loops.

I left TDZ enforcement (bug 1069480) and fresh bindings in for-in/of loops (bug 449811) for another day.

Comment 5

[Jason Orendorff [:jorendorff]]

    $ hg diff -r .^ frontend | diffstat
     8 files changed, 96 insertions(+), 150 deletions(-)

I sure hope this patch doesn't break everything, because I'm awfully happy with how it turned out. Scope freshening is now entirely in the emitter. The parser doesn't know about it. Wrapping the whole for-loop in a LEXICALSCOPE node just worked.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8677044 [details] [diff] [review]
In `for (let ...)` loops, evaluate initializers in the scope of the variables being initialized

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

This is surprisingly not bad, and tracking the stuff I'd already been doing pretty closely.  Nice!

::: js/src/builtin/ReflectParse.cpp
@@ +2641,5 @@
>          RootedValue stmt(cx);
>          if (!statement(pn->pn_right, &stmt))
>              return false;
>  
> +        if (head->isKind(PNK_FORIN) || head->isKind(PNK_FOROF)) {

Wow, this all is, like, actually *readable* now.  :-)

::: js/src/frontend/Parser.cpp
@@ +5304,5 @@
> +    // letStmt is the BLOCK StmtInfo for the implicit block.
> +    //
> +    // Caution: `letStmt.emplace()` creates some Rooted objects. Rooteds must
> +    // be created/destroyed in FIFO order. Therefore adding a Rooted between
> +    // this point and the .emplace() call below would trip assertions.

Add something about "at top level in this function".

@@ +5454,4 @@
>          if (isForDecl) {
>              /*
>               * pn2 is part of a declaration. Make a copy that can be passed to
> +             * EmitAssignment.

BytecodeEmitter::emitAssignment, presumably.

::: js/src/jit-test/tests/basic/bug646968-3.js
@@ +1,1 @@
> +var s, v = 9;

s/9/"NOPE"/ or something more glaringly wrong.

::: js/src/jit-test/tests/basic/bug646968-6.js
@@ +3,5 @@
> +
> +load(libdir + "asserts.js");
> +
> +assertThrowsInstanceOf(() => {
> +    for (let x = x; x < 3; x++) {}

Change the test/update parts to null.foo = 5 or something that'll throw not-a-ReferenceError.

@@ +7,5 @@
> +    for (let x = x; x < 3; x++) {}
> +}, ReferenceError);
> +
> +assertThrowsInstanceOf(() => {
> +    for (let x = eval('x'); x < 3; x++) {}

Likewise.

@@ +11,5 @@
> +    for (let x = eval('x'); x < 3; x++) {}
> +}, ReferenceError);
> +
> +assertThrowsInstanceOf(() => {
> +    for (let x = function () { with ({}) return x; }(); x < 3; x++) {}

.

Comment 7

[Jason Orendorff [:jorendorff]]

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

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Rename some loop variables to avoid conflicts with ES6 scoping rules

Comment 9

[Jason Orendorff [:jorendorff]]

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

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: rollup of everything in this bug and prerequisites (for shu)

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8679117 [details] [diff] [review]
Part 1: Rename some loop variables to avoid conflicts with ES6 scoping rules

Requesting reviews from:
  fitzgen for CensusUtils.js
  rhelmer for UITour.jsm
  gps for dataprovider.jsm
  ttaubert for SessionStore.jsm

Comment 12

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

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Comment on attachment 8679117 [details] [diff] [review]
> Part 1: Rename some loop variables to avoid conflicts with ES6 scoping rules
> 
> Requesting reviews from:
>   fitzgen for CensusUtils.js
>   rhelmer for UITour.jsm
>   gps for dataprovider.jsm
>   ttaubert for SessionStore.jsm

(You didn't flag anyone but me for review).

Comment 13

[Jason Orendorff [:jorendorff]]

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

Comment 14

[Robert Helmer [:rhelmer]]

Comment on attachment 8679117 [details] [diff] [review]
Part 1: Rename some loop variables to avoid conflicts with ES6 scoping rules

UITour.jsm changes LGTM but I've barely touched that code, would be good to get someone who is more familiar to confirm.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8679117 [details] [diff] [review]
Part 1: Rename some loop variables to avoid conflicts with ES6 scoping rules

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

UITour.jsm is fine

Comment 16

[Jason Orendorff [:jorendorff]]

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

Comment 17

[Jason Orendorff [:jorendorff]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02922ca741fd7f1cb3a742b9afcc15c1f5e49083
Bug 1216623 - Part 1: Rename some loop variables to avoid conflicts with ES6 scoping rules. r=fitzgen, r=ttaubert, r=MattN, r=gps.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0741b494a70915a451fda9378a20816319ee3c
Bug 1216623 - Part 2: In `for (let ...)` loops, evaluate initializers in the scope of the variables being initialized. r=Waldo.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02922ca741fd
https://hg.mozilla.org/mozilla-central/rev/ce0741b494a7

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/02922ca741fd
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ce0741b494a7