Bug 160955 - [ES2016] Allow assignment in for-in head in not-strict mode
Summary: [ES2016] Allow assignment in for-in head in not-strict mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 160392
  Show dependency treegraph
 
Reported: 2016-08-17 23:48 PDT by GSkachkov
Modified: 2016-08-24 07:38 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.43 KB, patch)
2016-08-23 05:28 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1007.77 KB, application/zip)
2016-08-23 06:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (815.56 KB, application/zip)
2016-08-23 06:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2 (635.79 KB, application/zip)
2016-08-23 06:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.50 MB, application/zip)
2016-08-23 06:38 PDT, Build Bot
no flags Details
Patch (13.68 KB, patch)
2016-08-23 08:01 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (13.04 KB, patch)
2016-08-23 12:30 PDT, GSkachkov
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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