Bug 156673 – We allow assignments to const variables when in a for-in/for-of loop

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

```
const x = 20;
for (x in/of [1,2,3]) ;
// We don't throw an exception even though we should.
```

[Marcus Buffett]

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?

[Saam Barati]

(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()
```
?

[Marcus Buffett]

I did not, confirmed that reproduces the issue. Thank you!

[JF Bastien]

Created attachment 286315 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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.

[Saam Barati]

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.

[JF Bastien]

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.

[JF Bastien]

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).

[Saam Barati]

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.

[Saam Barati]

(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.

[WebKit Commit Bot]

Comment on attachment 286332 [details]
patch

Clearing flags on attachment: 286332

Committed r204586: <http://trac.webkit.org/changeset/204586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.