Bug 160955 – [ES2016] Allow assignment in for-in head in not-strict mode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

Links to spec https://tc39.github.io/ecma262/#sec-initializers-in-forin-statement-heads

var f = function () {  
    for (var i = 0 in {}) {}
    return i === 0;
};
f(); // False -> should be true

[GSkachkov]

Created attachment 286703 [details]
Patch

Patch for review

[Build Bot]

Comment on attachment 286703 [details]
Patch

Attachment 286703 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1927500

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 286705 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286703 [details]
Patch

Attachment 286703 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1927496

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 286706 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 286703 [details]
Patch

Attachment 286703 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1927521

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 286707 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 286703 [details]
Patch

Attachment 286703 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1927511

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 286709 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 286712 [details]
Patch

Fix tests

[Saam Barati]

Comment on attachment 286712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286712&action=review

r=me with one request:

Can you see what should be done for the following type of programs:
```
function foo() {
    {
        let x = 50;
        for (var x = 25 in {}) { }
        // what is x here?
    }
    // and what is x here?
}
foo();

and

function foo() {
    {
        const x = 50;
        for (var x = 25 in {}) { }
       // Should we throw a const assignment error?
    }
}
foo();
```
Currently, both Firefox and Chrome throw syntax errors for these programs. However, I suspect that's not the correct behavior as defined by the spec.
If it is, we should also throw a syntax error. If it isn't, we should make sure we do the right thing. I suspect that in the first example,
in the "let x" scope, x should be 25. In the "var" scope of the function, I suspect x should be undefined. But who knows.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2560
> +void ForInNode::emitResolveVariable(BytecodeGenerator& generator, RegisterID* propertyName, const Identifier& ident)

Suggestion: An alternative to defining a method is you can just use a local lambda inside the ::emitLoopHeader that does this.

[GSkachkov]

(In reply to comment #11)
> Comment on attachment 286712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286712&action=review
> 
> r=me with one request:
> 
> Can you see what should be done for the following type of programs:
> ```
> function foo() {
>     {
>         let x = 50;
>         for (var x = 25 in {}) { }
>         // what is x here?
>     }
>     // and what is x here?
> }
> foo();
> 
> and
> 
> function foo() {
>     {
>         const x = 50;
>         for (var x = 25 in {}) { }
>        // Should we throw a const assignment error?
>     }
> }
> foo();
> ```
> Currently, both Firefox and Chrome throw syntax errors for these programs.
> However, I suspect that's not the correct behavior as defined by the spec.
> If it is, we should also throw a syntax error. If it isn't, we should make
> sure we do the right thing. I suspect that in the first example,
> in the "let x" scope, x should be 25. In the "var" scope of the function, I
> suspect x should be undefined. But who knows.
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2560
> > +void ForInNode::emitResolveVariable(BytecodeGenerator& generator, RegisterID* propertyName, const Identifier& ident)
> 
> Suggestion: An alternative to defining a method is you can just use a local
> lambda inside the ::emitLoopHeader that does this.

Hmm, it is good question.
I think it should behave in the same way as work without for..in, and this annex to spec should not modify this behavior so it currently work in this way
function foo() {
    {
         let x = 50;
         var x = 25;
         debug(x); // 25
     }
     debug(x); // undefined
}
foo();
//////////////////////
function foo() {
    {
         const x = 50;
         var x = 25;
         debug(x); // Syntax Error
     }
     debug(x); // undefined
}
foo();
with for..in it works in the same way.
Should I add tests for this cases?

[GSkachkov]

Created attachment 286755 [details]
Patch

Add more tests

[Saam Barati]

Comment on attachment 286755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286755&action=review

> JSTests/stress/for-in-tests.js:161
> +    var foo = function(a, b, first) {
> +        {   
> +            let p = 'some-value';
> +            for (var p = b in a) {}
> +            if (first)
> +                return p;
> +        }
> +        return p;
> +    };

can you also add this test to make sure if throws a runtime exception when we write to the const variable:
foo = function(a, b) {
    {
        const p = 'some-value';
        for (var p = b in a) { }
    }
}

[Saam Barati]

(In reply to comment #14)
> Comment on attachment 286755 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286755&action=review
> 
> > JSTests/stress/for-in-tests.js:161
> > +    var foo = function(a, b, first) {
> > +        {   
> > +            let p = 'some-value';
> > +            for (var p = b in a) {}
> > +            if (first)
> > +                return p;
> > +        }
> > +        return p;
> > +    };
> 
> can you also add this test to make sure if throws a runtime exception when
> we write to the const variable:
> foo = function(a, b) {
>     {
>         const p = 'some-value';
>         for (var p = b in a) { }
>     }
> }
Correction: *when we try to write*, we should not successfully write to it.

[GSkachkov]

Comment on attachment 286755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286755&action=review

>>> JSTests/stress/for-in-tests.js:161
>>> +    };
>> 
>> can you also add this test to make sure if throws a runtime exception when we write to the const variable:
>> foo = function(a, b) {
>>     {
>>         const p = 'some-value';
>>         for (var p = b in a) { }
>>     }
>> }
> 
> Correction: *when we try to write*, we should not successfully write to it.

It is already there. See line 205, but there will be Syntax  error: 'Cannot declare a var variable that shadows a let/const/class variable: '. 
I'll land patch, if there is need another test I'll add as new patch.

[GSkachkov]

Patch was landed
https://trac.webkit.org/r204895 
So task is closed