Bug 160955

Summary: [ES2016] Allow assignment in for-in head in not-strict mode
Product: WebKit Reporter: GSkachkov <gskachkov>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, gskachkov, keith_miller, mark.lam, msaboff, rniwa, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 160392    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch
none
Patch saam: review+, saam: commit-queue-

Description GSkachkov 2016-08-17 23:48:12 PDT
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
Comment 1 GSkachkov 2016-08-23 05:28:38 PDT
Created attachment 286703 [details]
Patch

Patch for review
Comment 2 Build Bot 2016-08-23 06:16:39 PDT
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
Comment 3 Build Bot 2016-08-23 06:16:42 PDT
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
Comment 4 Build Bot 2016-08-23 06:18:24 PDT
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
Comment 5 Build Bot 2016-08-23 06:18:27 PDT
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
Comment 6 Build Bot 2016-08-23 06:30:58 PDT
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
Comment 7 Build Bot 2016-08-23 06:31:01 PDT
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
Comment 8 Build Bot 2016-08-23 06:38:21 PDT
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
Comment 9 Build Bot 2016-08-23 06:38:24 PDT
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
Comment 10 GSkachkov 2016-08-23 08:01:22 PDT
Created attachment 286712 [details]
Patch

Fix tests
Comment 11 Saam Barati 2016-08-23 09:29:42 PDT
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.
Comment 12 GSkachkov 2016-08-23 11:43:11 PDT
(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?
Comment 13 GSkachkov 2016-08-23 12:30:53 PDT
Created attachment 286755 [details]
Patch

Add more tests
Comment 14 Saam Barati 2016-08-23 14:14:52 PDT
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) { }
    }
}
Comment 15 Saam Barati 2016-08-23 14:15:21 PDT
(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.
Comment 16 GSkachkov 2016-08-24 07:38:01 PDT
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.
Comment 17 GSkachkov 2016-08-24 07:38:57 PDT
Patch was landed
https://trac.webkit.org/r204895 
So task is closed