854037 - fresh binding per iteration of C-style for-let

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kyle Simpson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20130320 Firefox/21.0
Build ID: 20130320042012

Steps to reproduce:

I tried this: https://gist.github.com/getify/5225304

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! ;)


Actual results:

// 5
// 5
// 5
// 5
// 5


Expected results:

according to TC39 sometime in the last year:

// 0
// 1
// 2
// 3
// 4

Comment 2

[Dave Herman [:dherman]]

Reopening. This bug is about the scope of C-style for loops using let, which is a separate question from for-of and for-in loops. TC39 shifted in January 2012 to move to a semantics more like Dart, where each loop iteration gets a fresh binding that inherits as its initial value the final value of the previous iteration. See the bottom of:

https://mail.mozilla.org/pipermail/es-discuss/2012-January/019784.html

The semantics of this is not yet in the draft ES6 spec. There are definitely some subtleties.

Dave

Comment 3

[Kyle Simpson]

bump.

Comment 5

[Allen Wirfs-Brock]

This will be in the Rev 23 ES6 draft.  You can see a preview of the for(let;;) spec at https://twitter.com/awbjs/status/433645470431186944/photo/1

Comment 6

[Shu-yu Guo [:shu]]

Is this still the case? This isn't what I'm understanding from reading http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-for-statement-runtime-semantics-labelledevaluation

Comment 7

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Shu-yu Guo [:shu] from comment #6)
> Is this still the case? This isn't what I'm understanding from reading
> http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-for-
> statement-runtime-semantics-labelledevaluation

See comment #5. Changes are not present in the 22nd draft.

Comment 8

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Shu-yu Guo [:shu] from comment #6)
> Is this still the case? This isn't what I'm understanding from reading
> http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-for-
> statement-runtime-semantics-labelledevaluation

This is the current URL: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-for-statement-runtime-semantics-labelledevaluation

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Michał Gołębiowski [:m_gol] from comment #8)
> (In reply to Shu-yu Guo [:shu] from comment #6)
> > Is this still the case? This isn't what I'm understanding from reading
> > http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-for-
> > statement-runtime-semantics-labelledevaluation
> 
> This is the current URL:
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-for-statement-
> runtime-semantics-labelledevaluation

Ack, thanks.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Make lexical declarations in the initializing component of a for(;;) loop create a fresh binding for each iteration of the loop

This doesn't change for (let x = x; ; ); scoping to deal with the TDZ.  It only handles creating new bindings each iteration.  I'm looking into doing the TDZ thing atop this -- it might (I'm not certain) be possible to refactor the current thing into a block with a let-declaration and a modified for-loop in it.  More as I learn it.

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8582823 [details] [diff] [review]
Make lexical declarations in the initializing component of a for(;;) loop create a fresh binding for each iteration of the loop

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

Certified fresh. My fears for how complicated this was going to be were unfounded.

It would be ideal to desugar into a non-binary let block, even as part of this bug. Could that be part 2?

::: js/src/frontend/Parser.cpp
@@ +4754,5 @@
> +    //   assertEq(funcs[0](), 0);
> +    //   assertEq(funcs[1](), 1);
> +    //
> +    // These semantics are implemented by "freshening" the implicit block each
> +    // iteration, before evaluating the "update" in for(;;) loops.  (We don't

Could use a parenthetical on what "freshening" actually does (clone).

@@ +4760,5 @@
> +    // No freshening occurs in for (const ...;;) -- there's no point (you can't
> +    // reassign consts), and the spec requires it (which fact is exposed by the
> +    // debugger API, if not in-language).
> +    //
> +    // Setting aside our inconsistencies with the spec.  If the for-loop head

"Setting aside out inconsistencies with the spec" doesn't seem like a useful sentence in a comment.

@@ +4951,5 @@
> +            //   for (let INIT; TEST; UPDATE) STMT
> +            //
> +            // into
> +            //
> +            //   let (INIT) { for (; TEST; UPDATE) STMT }

I want binary let blocks gone. Would it be difficult, while you're here, to desugar into

{ let INIT; for (; TEST; UPDATE) STMT }

instead?

@@ +4962,5 @@
>  
>              forLetDecl = pn1;
> +
> +            // The above transformation isn't enough to implement INIT scoping,
> +            // because each loop iteration must see separate |INIT|.  We handle

separate |INIT| -> separate binding of INIT.

@@ +4968,5 @@
> +            // each iteration.  We supply a special PNK_FRESHENBLOCK node as
> +            // the init node for for(let A;;) loop heads to distinguish such
> +            // nodes from an *actual*, non-desugared use of the above syntax.
> +            // (We don't do this for PNK_CONST nodes because the spec says no
> +            // freshening happens -- and this is observable with the debugger

nit: capitalize Debugger

::: js/src/jit/BaselineCompiler.cpp
@@ +3003,5 @@
> +
> +bool
> +BaselineCompiler::emit_JSOP_FRESHENBLOCKSCOPE()
> +{
> +    // Call a stub to freshen the block on the block chain.

Style nit: I'd remove this comment. Code is pretty self-explanatory.

::: js/src/jit/BaselineFrame-inl.h
@@ +43,5 @@
>      popOffScopeChain();
>  }
>  
> +inline void
> +BaselineFrame::replaceScopeChain(ScopeObject &scope)

Given what it does, perhaps this is clearer as "replaceInnermostScope"

@@ +73,5 @@
>  
> +inline bool
> +BaselineFrame::freshenBlock(JSContext *cx)
> +{
> +    MOZ_ASSERT(scopeChain_->is<ClonedBlockObject>());

Superfluous assert, the as<ClonedBlockObject> call below will assert.

::: js/src/tests/ecma_6/LexicalEnvironment/for-loop.js
@@ +44,5 @@
> +isOK("for (let x; ; ) ;");
> +isOK("for (let x = 5, y; ; ) ;");
> +isOK("for (let [z] = [3]; ; ) ;"); // XXX ??? or syntax error?
> +isError("for (let [z, z]; ; ) ;", SyntaxError); // XXX or TypeError?
> +isError("for (let [z, z] = [0, 1]; ; ) ;", TypeError); // XXX or SyntaxError?

What's up with these XXXs?

::: js/src/vm/ScopeObject.cpp
@@ +693,5 @@
> +
> +    copy.setReservedSlot(SCOPE_CHAIN_SLOT, block->getReservedSlot(SCOPE_CHAIN_SLOT));
> +
> +    for (uint32_t i = 0, count = copy.numVariables(); i < count; i++)
> +        copy.setSlot(RESERVED_SLOTS + i, block->getSlot(RESERVED_SLOTS + i));

Should use setVar and var here for readability.

::: js/src/vm/ScopeObject.h
@@ +672,5 @@
> +    /*
> +     * Create a new ClonedBlockObject with the same enclosing scope and
> +     * variable values as this.
> +     */
> +    static ClonedBlockObject * clone(ExclusiveContext *cx, Handle<ClonedBlockObject*> block);

Nit: extra space after *

::: js/src/vm/Stack-inl.h
@@ +207,5 @@
>      scopeChain_ = &scopeChain_->as<ScopeObject>().enclosingScope();
>  }
>  
> +inline void
> +InterpreterFrame::replaceScopeChain(ScopeObject &scope)

Ditto on the name. Maybe replaceInnermostScope.

Comment 12

[Jeff Walden [:Waldo]]

I do want to very quickly follow up converting this to a { let ...; for (; ...; ...) ... } form, yes.  Let-expressions complicate this some; the divergent scoping implementations for for(;;) loops versus for-in/of complicate some more.  But I do plan to fix all of this in probably the next month -- fixing all the rest of for-loop scoping seems likely to be a quarterly goal if I can swing it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out because bug 1146644 was backed out and it wasn't clear to me if this depended on that or not and I didn't want to risk an extended tree closure finding out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f8d63b2cad

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b05e10ed40c4