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
Created attachment 286703 [details] Patch Patch for review
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
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 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
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 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
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 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
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
Created attachment 286712 [details] Patch Fix tests
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.
(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?
Created attachment 286755 [details] Patch Add more tests
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) { } } }
(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 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.
Patch was landed https://trac.webkit.org/r204895 So task is closed