``` const x = 20; for (x in/of [1,2,3]) ; // We don't throw an exception even though we should. ```
Tried to reproduce with the following: ``` const x = 20; for (x in [1,2,3]) { } ``` A syntax error was thrown, specifically `TypeError: Assignment to constant variable`. Close?
(In reply to comment #1) > Tried to reproduce with the following: > ``` > const x = 20; > for (x in [1,2,3]) { > } > ``` > A syntax error was thrown, specifically `TypeError: Assignment to constant > variable`. Close? Did you try: ``` function foo() { const x = 20; for (x in [1,2,3]) { } } foo() ``` ?
I did not, confirmed that reproduces the issue. Thank you!
Created attachment 286315 [details] patch
Comment on attachment 286315 [details] patch Attachment 286315 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1888656 New failing tests: webgl/1.0.2/conformance/extensions/webgl-compressed-texture-s3tc.html js/arguments-iterator.html inspector/formatting/formatting-json.html
Created attachment 286318 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286315 [details] patch Looks like you have some test failures in layout tests. Some of our JS test coverage comes from layout tests, especially weird things like the WebGL tests (which have lots of weird JS) and the inspector tests (giant pile of JS). You'll either need to rebase the tests or figure out if maybe you created a legit regression.
Comment on attachment 286315 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=286315&action=review > JSTests/stress/for-in-of-const.js:5 > + print("Executing: " + why); Remove this. > JSTests/stress/for-in-of-const.js:10 > + print("Executing: " + why); remove this.
Comment on attachment 286315 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=286315&action=review >> JSTests/stress/for-in-of-const.js:5 >> + print("Executing: " + why); > > Remove this. Done. >> JSTests/stress/for-in-of-const.js:10 >> + print("Executing: " + why); > > remove this. Done.
Created attachment 286332 [details] patch Updated patch, checks for var.isReadOnly() because isStrictMode otherwise fires and throws when it shouldn't. I'll file a separate bug to refactor that (isReadOnly versus isConst).
Comment on attachment 286332 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=286332&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2562 > + if (var.isReadOnly()) > + generator.emitReadOnlyExceptionIfNeeded(var); I think you want these before emitExpressionInfo(). > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2795 > + if (var.isReadOnly()) > + generator.emitReadOnlyExceptionIfNeeded(var); Ditto.
(In reply to comment #11) > Comment on attachment 286332 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286332&action=review > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2562 > > + if (var.isReadOnly()) > > + generator.emitReadOnlyExceptionIfNeeded(var); > > I think you want these before emitExpressionInfo(). > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2795 > > + if (var.isReadOnly()) > > + generator.emitReadOnlyExceptionIfNeeded(var); > > Ditto. I spoke with JF in person about this. My above comments are wrong. Basically, we either emit code to throw an exception, in which case, this code is correct. Or, we don't emit code to throw an exception, in which case, we have the same bytecode as we had before this patch. So it's OK to put the emitExpressionInfos where they are.
Comment on attachment 286332 [details] patch Clearing flags on attachment: 286332 Committed r204586: <http://trac.webkit.org/changeset/204586>
All reviewed patches have been landed. Closing bug.