New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dropping for-in initializers was sufficiently incompatible as to be reverted by browsers #260

Closed
saurik opened this Issue Dec 16, 2015 · 8 comments

Comments

Projects
None yet
7 participants
@saurik

saurik commented Dec 16, 2015

I was having a discussion with @wycats, and he said that if the spec said to do something that all the major browsers refused to do, that it should be filed as a bug against the spec; he encouraged me to actually file this one as a pull request, but I did not feel entirely qualified to decide the next step.

All of V8, JavaScriptCore, and SpiderMonkey have found themselves unable to honor the ECMAScript 2015 requirement that "for (var x = 0 in []);" is a SyntaxError (which is documented in Annex E 13.7). Here is what I've been able to dig up about this situation, with links to online bug trackers as appropriate.

To begin with, the text "The value of that expression was always discarded." is actually incorrect: the code has in fact always been executed as a side effect until a few months ago, and even the value was quite visible in that it "initialized" the variable: note the behavior with respect to iterating an empty array.

V8 version 2.2.23
> for (var i = 0 in []); i
0

Meanwhile, when this was discussed for compatibility impact a while ago, running a regex against a web crawl showed that the syntax was being used by a number of websites... and the regular expression was flawed in a way (checking only for one character variable names) that underestimates the usage.

https://bugzilla.mozilla.org/show_bug.cgi?id=748550

Finally, when this was actually deployed in real-world browsers, it failed: people complained about code not working any longer, and the change was reverted, first in JavaScriptCore, and then in SpiderMonkey. I am not certain if V8 ever shipped the change, but the code currently includes a workaround for this.

https://bugs.webkit.org/show_bug.cgi?id=130269
https://bugzilla.mozilla.org/show_bug.cgi?id=1164741

https://chromium.googlesource.com/v8/v8.git/+/0e052bb8346d23627d1cfed43b4dc8bdf1a734d2/src/parsing/parser.cc#3740

FWIW, my personal idea would be to include the following extra production, which adds the most minimal amount of extra support for this feature to the grammar as is possible in order to make it work. I also believe that the result should not be discarded, as otherwise it breaks old initialization logic.

IterationStatement[Yield,Return]:
    for ( var BindingIdentifier[?Yield] Initializer[?Yield] in Expression[In,?Yield] ) Statement[?Yield, ?Return]
@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Dec 16, 2015

Contributor

I also believe that the result should not be discarded, as otherwise it breaks old initialization logic.

The initialiser expression is ignored in JavaScriptCore and SpiderMonkey without breaking legacy web-compat, so initialisation logic doesn't seem to depend on this.

Contributor

anba commented Dec 16, 2015

I also believe that the result should not be discarded, as otherwise it breaks old initialization logic.

The initialiser expression is ignored in JavaScriptCore and SpiderMonkey without breaking legacy web-compat, so initialisation logic doesn't seem to depend on this.

@saurik

This comment has been minimized.

Show comment
Hide comment
@saurik

saurik Dec 16, 2015

Within about ten minutes I was able to find someone who wrote code that seems to actually be relying on this behavior (that the variable is left at 0 at the end of the loop). They clearly don't know what they are doing (like, "lol"), but in all honestly, that describes 99% of people who program in JavaScript, as it is effectively "the programming language of the masses" and often "one's first and only language".

http://stackoverflow.com/questions/28843561/appending-multiple-div-using-javascript-timer

Regardless, I guess the level considered as "breaking legacy web-compat" is "do a ton of people who are developer savvy enough to know how to file bug reports with a browser vendor scream in pain within a few months of release"? If so: I guess, "fair enough :(". But that still leaves the issue that currently the syntax is specified as removed entirely... it only affects "var" and is trivial to implement.

saurik commented Dec 16, 2015

Within about ten minutes I was able to find someone who wrote code that seems to actually be relying on this behavior (that the variable is left at 0 at the end of the loop). They clearly don't know what they are doing (like, "lol"), but in all honestly, that describes 99% of people who program in JavaScript, as it is effectively "the programming language of the masses" and often "one's first and only language".

http://stackoverflow.com/questions/28843561/appending-multiple-div-using-javascript-timer

Regardless, I guess the level considered as "breaking legacy web-compat" is "do a ton of people who are developer savvy enough to know how to file bug reports with a browser vendor scream in pain within a few months of release"? If so: I guess, "fair enough :(". But that still leaves the issue that currently the syntax is specified as removed entirely... it only affects "var" and is trivial to implement.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 16, 2015

Member

Seconded! A spec change to reflect reality here would be great. V8 does not ship this change, and we implement the old semantics for for-in with an initializer. We do use the initializer expression correctly.

I like your restricted grammar (only 'var' and 'in'). I'd extend it with a static semantics rule to prohibit it in strict mode (which is how V8 and SpiderMonkey currently implement it).

Member

littledan commented Dec 16, 2015

Seconded! A spec change to reflect reality here would be great. V8 does not ship this change, and we implement the old semantics for for-in with an initializer. We do use the initializer expression correctly.

I like your restricted grammar (only 'var' and 'in'). I'd extend it with a static semantics rule to prohibit it in strict mode (which is how V8 and SpiderMonkey currently implement it).

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Dec 16, 2015

Member

There are meeting notes on this topic that you should lookup.

Member

allenwb commented Dec 16, 2015

There are meeting notes on this topic that you should lookup.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Dec 16, 2015

Contributor

@allenwb the meeting notes are at https://github.com/rwaldron/tc39-notes/blob/aad8937063ab32eb33ec2a5b40325b1d9f171180/es6/2014-04/apr-10.md#revisiting-initializer-in-for-in.

WH: In favor of taking it out and seeing if we can get away with it. We can put it back in Annex B. The work to take it out is not wasted.
BT: If all three browsers continue supporting, we agree to put it in the spec, at least in Annex B


Conclusion/Resolution
Mozilla willing to ship a patch in nightly for experimentation

In other words "let's see if we can get away with it". @saurik and @littledan are correctly pointing out that we could not.

Contributor

wycats commented Dec 16, 2015

@allenwb the meeting notes are at https://github.com/rwaldron/tc39-notes/blob/aad8937063ab32eb33ec2a5b40325b1d9f171180/es6/2014-04/apr-10.md#revisiting-initializer-in-for-in.

WH: In favor of taking it out and seeing if we can get away with it. We can put it back in Annex B. The work to take it out is not wasted.
BT: If all three browsers continue supporting, we agree to put it in the spec, at least in Annex B


Conclusion/Resolution
Mozilla willing to ship a patch in nightly for experimentation

In other words "let's see if we can get away with it". @saurik and @littledan are correctly pointing out that we could not.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Dec 18, 2015

Member

We also do not get away with this. I will present this at the next TC39 to get consensus for what goes in Annex B. Essentially it seems like we do have evidence that we could get away with ignoring this syntax if we wanted, but it's not clear that's what we want if that's all we can do.

Member

bterlson commented Dec 18, 2015

We also do not get away with this. I will present this at the next TC39 to get consensus for what goes in Annex B. Essentially it seems like we do have evidence that we could get away with ignoring this syntax if we wanted, but it's not clear that's what we want if that's all we can do.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan May 22, 2016

Member

V8 has finally collected some data on this question: According to our (admittedly impefect) counters, 0.011% of document loads use the for (var x = y in z) pattern. See the data at https://www.chromestatus.com/metrics/feature/timeline/popularity/1238 (the graph increased over time as the counter rolled out).

Member

littledan commented May 22, 2016

V8 has finally collected some data on this question: According to our (admittedly impefect) counters, 0.011% of document loads use the for (var x = y in z) pattern. See the data at https://www.chromestatus.com/metrics/feature/timeline/popularity/1238 (the graph increased over time as the counter rolled out).

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan May 26, 2016

Member

After TC39, some of us had an informal conversation about this issue. It seems that there is interest in adding this feature back. I will follow up with a PR.

Member

littledan commented May 26, 2016

After TC39, some of us had an informal conversation about this issue. It seems that there is interest in adding this feature back. I will follow up with a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment