Array comprehension [1] and generator comprehension [2] are both now non-standard , including both legacy syntax |[x for (x of y)] and newer syntax |[for (x of y) x]|. [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Array_comprehensions [2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Generator_comprehensions

Newer |[for (x of y) x]| comprehensions are non-standard still, true, but I would be hesitant to declare them a never-will-happen by removing them just yet. They were on track for ES6 until people got cold feet about the composability of the syntax at the last minute, then they were punted to ES7. I wouldn't quite dismiss them yet. Sticking to killing off the old syntax is a solid plan, and I'd defer the second part a little bit longer just in case it makes a comeback.

I see. We should track the SpiderMonkey support separately for them, and only legacy comprehension for now :)

Updated the site compat doc accordingly: https://www.fxsitecompat.com/en-US/docs/2015/legacy-array-generator-comprehension-syntaxes-will-be-removed/

Script used to find what js files have genexprs or comprehensions.

Comment on attachment 8702780 [details] [diff] [review] Remove chrome code uses of genexprs and legacy comprehensions. Review of attachment 8702780 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/PersistentDataBlock.jsm @@ +68,2 @@ > } else if (typeof data === "array") { > + hexString = data.map(toHexChar).join(""); FWIW, typeof data === "array" will always be false (typeof will return "object" for arrays). Maybe file a b2g bug?

Comment on attachment 8702404 [details] [diff] [review] Remove legacy generator comprehensions. Review of attachment 8702404 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +8399,2 @@ > > + MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_IN_PAREN); This might be foldable into comprehension(), in a separate patch/bug, I'd bet. ::: js/src/jsversion.h @@ +21,5 @@ > #define JS_HAS_FOR_EACH_IN 1 /* has for each (lhs in iterable) */ > #define JS_HAS_GENERATORS 1 /* (no longer used) */ > #define JS_HAS_BLOCK_SCOPE 1 /* (no longer used) */ > #define JS_HAS_DESTRUCTURING 2 /* (no longer used) */ > +#define JS_HAS_GENERATOR_EXPRS 1 /* (no longer used) */ Never seen this "(no longer used)" thing before. Surely we can just get rid of this nonsense in a separate bug/patch, rather than leaving all these tombstones for no purpose?

Comment on attachment 8702405 [details] [diff] [review] Remove legacy array comprehensions. Review of attachment 8702405 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +7793,4 @@ > * [for (V of OBJ) if (COND) EXPR] // ES6-era array comprehension > * (for (V of OBJ) if (COND) EXPR) // ES6-era generator expression > * > * (The last two flavors are called "ES6-era" because they were in ES6 draft s/The last two/These/

Comment on attachment 8702406 [details] [diff] [review] Update and remove obsolete jit-tests. Review of attachment 8702406 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/basic/bug791465.js @@ +50,5 @@ > "use strict" > (); > }, > ...([]), > + ...(binary_ops.map((op) => Function("'use strict'\n " + op + " 'not'"))), Nor are parens needed around op here. @@ +65,5 @@ > "<<=", ">>=", ">>>=", > "*=", "/=", "%=", > ]; > > +var invalid_strict_funs_referror = assignment_ops.map((op) => ("'use strict'\n " + op + " 'not'")); Or here. ::: js/src/jit-test/tests/collections/Map-constructor-generator-3.js @@ -1,4 @@ > -// The argument to Map may be a generator-iterator that produces no values. > - > -var m = new Map(x for (x of [])); > -assertEq(m.size, 0); Remove just these two lines, please (and declare |m| further down). ::: js/src/jit-test/tests/collections/Set-iterator-remove-1.js @@ +3,5 @@ > function test(letters, toRemove) { > var set = new Set(letters); > toRemove = new Set(toRemove); > > + var leftovers = [...set].filter((x) => !toRemove.has(x)).join(""); (x) parens, again ::: js/src/jit-test/tests/collections/WeakMap-constructor-generator-3.js @@ -1,3 @@ > -// The argument to WeakMap may be a generator-iterator that produces no values. > - > -new WeakMap(x for (x of [])); Looks like you can/should remove just this line here, and leave the rest of the test still in existence, until legacy generator functions are removed.

Comment on attachment 8702780 [details] [diff] [review] Remove chrome code uses of genexprs and legacy comprehensions. Review of attachment 8702780 [details] [diff] [review]: ----------------------------------------------------------------- Now we know. Copies of toHexString in mozilla-central: 7. ::: browser/components/customizableui/CustomizableUI.jsm @@ +3359,5 @@ > * This array is created for you, so is modifiable without CustomizableUI > * being affected. > */ > get areas() { > + return gAreas.map(([area, props]) => area); Don't think this is correct. gAreas is a Map, which doesn't have a map method. @@ +3761,5 @@ > let buildAreas = gBuildAreas.get(area); > if (!buildAreas) { > return []; > } > + return buildAreas.map((node) => this.forWindow(node.ownerDocument.defaultView)); This one appears to be a Set: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#1004 It also doesn't have a map method. @@ +3872,5 @@ > return areaProps && areaProps.get("type"); > }); > > this.__defineGetter__("instances", function() { > + return gBuildWindows.map(([win,]) => this.forWindow(win)); This one is also a Set. ::: browser/components/sessionstore/SessionStore.jsm @@ +1906,5 @@ > // Remove old closed windows > this._cleanupOldData([this._closedWindows]); > > // Remove closed tabs of closed windows > + this._cleanupOldData(this._closedWindows.map((winData) => winData._closedTabs)); This one is a WeakMap. Also no map method :-). ::: toolkit/components/osfile/modules/osfile_async_worker.js @@ +92,5 @@ > * Return a list of all open resources i.e. the ones still present in > * ResourceTracker's _map. > */ > listOpenedResources: function listOpenedResources() { > + return this._map.map(([id, resource]) => resource.info.path); This one appears to be a Map, so no map method. ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js @@ +271,5 @@ > // Testing nextBatch() > iterator = new OS.File.DirectoryIterator(parent); > + let allentries = []; > + for (let x in iterator) { > + allentries.push(x); Array.from?

(In reply to Bill McCloskey (:billm) from comment #15) > Comment on attachment 8702780 [details] [diff] [review] > Remove chrome code uses of genexprs and legacy comprehensions. > > Review of attachment 8702780 [details] [diff] [review]: > ----------------------------------------------------------------- > > Now we know. Copies of toHexString in mozilla-central: 7. > Yikes, thanks for the catches! > ::: browser/components/sessionstore/SessionStore.jsm > @@ +1906,5 @@ > > // Remove old closed windows > > this._cleanupOldData([this._closedWindows]); > > > > // Remove closed tabs of closed windows > > + this._cleanupOldData(this._closedWindows.map((winData) => winData._closedTabs)); > > This one is a WeakMap. Also no map method :-). > this._closedWindows looks like a plain array to me. this._closedTabs and _closedWindowTabs are WeakMaps though. > > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js > @@ +271,5 @@ > > // Testing nextBatch() > > iterator = new OS.File.DirectoryIterator(parent); > > + let allentries = []; > > + for (let x in iterator) { > > + allentries.push(x); > > Array.from? How do I use Array.from with for-in iteration?

> this._closedWindows looks like a plain array to me. this._closedTabs and _closedWindowTabs > are WeakMaps though. Sorry, you're right. > How do I use Array.from with for-in iteration? Can you do "let allentries = Array.from(iterator);"? The docs claim it works for iterators.

(In reply to Bill McCloskey (:billm) from comment #17) > Can you do "let allentries = Array.from(iterator);"? The docs claim it works > for iterators. For new style iterators that work with for-of iteration, I thought. For for-in iteration I think you're stuck with doing manual for-in iteration statement. Tom also suggested this gem in #jsapi: 15:54 < evilpie> Array.from(function* () { for (let x in {a: 1}) yield x; }())

It turns out I missed some files due to an error in my script, as well as a XUL file.

(In reply to Jan de Mooij [:jandem] from comment #11) > Comment on attachment 8702780 [details] [diff] [review] > Remove chrome code uses of genexprs and legacy comprehensions. > > Review of attachment 8702780 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/components/PersistentDataBlock.jsm > @@ +68,2 @@ > > } else if (typeof data === "array") { > > + hexString = data.map(toHexChar).join(""); > > FWIW, typeof data === "array" will always be false (typeof will return > "object" for arrays). Maybe file a b2g bug? Filed bug 1235990.

Comment on attachment 8702407 [details] [diff] [review] Update and remove obsolete JS reftests. Review of attachment 8702407 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/js1_8/genexps/regress-347739.js @@ -4,5 @@ > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > - > -//----------------------------------------------------------------------------- > -var BUGNUMBER = 347739; > -var summary = 'generator_instance.close readonly and immune'; Hmm. I don't actually see anywhere in this test that depends on legacy comprehension syntax. Shouldn't this be left around til legacy generators are removed? ::: js/src/tests/js1_8/genexps/regress-349012-01.js @@ -4,5 @@ > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > - > -//----------------------------------------------------------------------------- > -var BUGNUMBER = 349012; > -var summary = 'closing a generator fails to report error if yield during close is ignored'; This also looks like legacy generators, not comprehensions, and shouldn't be removed. ::: js/src/tests/js1_8/genexps/regress-349326.js @@ -4,5 @@ > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > - > -//----------------------------------------------------------------------------- > -var BUGNUMBER = 349326; > -var summary = 'closing generators'; Same here, don't remove this. ::: js/src/tests/js1_8/genexps/regress-384991.js @@ -4,5 @@ > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > - > -//----------------------------------------------------------------------------- > -var BUGNUMBER = 384991; > -var summary = ' w(yield) should not cause "yield expression must be parenthesized" syntax error'; This also looks legacy generator-related and shouldn't be removed. ::: js/src/tests/js1_8/genexps/regress-665286.js @@ -5,5 @@ > - > - > -//----------------------------------------------------------------------------- > -var BUGNUMBER = 665286; > -var summary = 'yield in arguments list'; This looks like legacy generators, not legacy comprehensions -- don't remove. ::: js/src/tests/js1_8/genexps/regress-683738.js @@ -5,5 @@ > - > - > -//----------------------------------------------------------------------------- > -var BUGNUMBER = 683738; > -var summary = 'return with argument and lazy generator detection'; This test also looks like it's about legacy generator functions, not comprehensions, and shouldn't be removed. ::: js/src/tests/js1_8_1/regress/regress-452498-082.js @@ -47,5 @@ > -// ===== > - > - try > - { > - (function(){new (function ({}, x) { yield (x(1e-81) for (x4 in undefined)) })()})(); Remove this section, not the overall test. @@ -56,5 @@ > -// ===== > - > - try > - { > - (function(){[(function ([y]) { })() for each (x in [])];})(); And also this section. @@ -111,5 @@ > - eval( > - 'for(let x;' + > - ' ([,,,]' + > - ' .toExponential(new Function(), (function(){}))); [] = {})' + > - ' for(var [x, x] = * in this.__defineSetter__("", function(){}));' This looks like old E4X syntax (the |* in ...| bit) and can be removed, even if most of the rest of the test stays. Doesn't *hurt* to swallow a syntax error, but what's the point? ::: js/src/tests/js1_8_1/regress/regress-452498-102.js @@ -67,5 @@ > -// ===== > - > - try > - { > - {x} ((x=[] for (x in []))); x; Just remove this bit of the test, please, and leave the rest. ::: js/src/tests/js1_8_1/regress/regress-452498-117.js @@ -55,5 @@ > -// Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2022 > -// ===== > - try > - { > - (function(){([]) ((function(q) { return q; })for (each in [1,2]))})(); Remove just this bit, please. @@ -66,5 @@ > -// ===== > - > - try > - { > - eval("((x1) > [(x)(function() { x;}) for each (x in x)])()"); And this. @@ -90,5 @@ > -// Assertion failure: op == JSOP_GETLOCAL, at ../jsemit.cpp:4557 > -// ===== > - try > - { > - (eval("(function(){let x , x = (x for (x in null))});"))(); And this. ::: js/src/tests/js1_8_1/regress/regress-452498-135.js @@ -37,5 @@ > -// === > - > - try > - { > - (x for each (c in [])) Just remove this test-bit, too. @@ -48,5 @@ > -// Assertion failure: ss->printer->pcstack, at ../jsopcode.cpp:909 > -// === > - try > - { > - (function(){for(; (this); ((window for (x in [])) for (y in []))) 0}); And this test-bit. @@ -56,5 @@ > - } > -// Assertion failure: level >= tc->staticLevel, at ../jsparse.cpp:5773 > -// === > - eval(uneval( function(){ > - ((function()y)() for each (x in this)) And. ::: js/src/tests/js1_8_5/extensions/clone-forge.js @@ +14,5 @@ > throw new TypeError("Assertion failed: " + f + " did not throw as expected"); > } > > function byteArray(str) { > + return str.split('').map((c) => c.charCodeAt(0)); No need for parens around the |c| as parameter. ::: js/src/tests/js1_8_5/extensions/clone-v1-typed-array.js @@ +122,5 @@ > var s = "captured[" + i + "] = "; > if (captured[i] instanceof Error) { > print(s + captured[i].toSource() + ";"); > } else { > + data = captured[i].clonebuffer.split('').map((c) => c.charCodeAt(0)); No parens around c, again. ::: js/src/tests/js1_8_5/extensions/regress-627859.js @@ -11,5 @@ > - // comprehension expression 'x' into the scope of the 'for' loop, > - // it must not bring the placeholder definition node for the > - // assignment to x above along with it. If it does, x won't appear > - // in b's lexdeps, we'll never find out that the assignment refers > - // to a's x, and we'll generate an assignment to the global x. *reads comment* Horrors. ::: js/src/tests/js1_8_5/reflect-parse/comprehensions.js @@ +2,5 @@ > function test() { > > +// Transform the legacy comprehensions to less legacy comprehensions and test > +// them. > +function assertFormerlyES6ArrayComp(expr, body, blocks, filter) { Taking these changes on faith, too. And still "less legacy" lol. ::: js/src/tests/js1_8_5/reflect-parse/generatorExpressions.js @@ +1,4 @@ > // |reftest| skip-if(!xulRuntime.shell) > function test() { > > +// Translate legacy genexprs into less legacy genexprs and test them. "less legacy", lol I decided not to look at this file's changes, gonna assume they were sensible.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee34d6a0db79 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f7a477db20 https://hg.mozilla.org/integration/mozilla-inbound/rev/727fa7056a9c https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c905de74c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/733d3f5c1619

https://hg.mozilla.org/mozilla-central/rev/ee34d6a0db79 https://hg.mozilla.org/mozilla-central/rev/d3f7a477db20 https://hg.mozilla.org/mozilla-central/rev/727fa7056a9c https://hg.mozilla.org/mozilla-central/rev/f7c905de74c8 https://hg.mozilla.org/mozilla-central/rev/733d3f5c1619

Posted the site compat doc: https://www.fxsitecompat.com/en-US/docs/2016/legacy-array-generator-comprehension-syntaxes-have-been-removed/

Since this is breaking some addons, should we start showing console warnings on aurora/beta? That gives them more time to update.

My Addons Manager itself is busted and useless. Have to revert to yesterday's nightly.

(In reply to Gary [:streetwolf] from comment #26) > > Offending line of code: return [toHexString(hash.charCodeAt(i)) for (i in > hash)].join(""); This unfortunately comes from a MDN nsICryptoHash example, https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICryptoHash#Computing_the_Hash_of_a_File fairly quoted around the web and probably copy-pasted in a few add-ons :(

(In reply to Giorgio Maone from comment #29) > (In reply to Gary [:streetwolf] from comment #26) > > > > > Offending line of code: return [toHexString(hash.charCodeAt(i)) for (i in > > hash)].join(""); > > This unfortunately comes from a MDN nsICryptoHash example, > > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/ > Interface/nsICryptoHash#Computing_the_Hash_of_a_File > > fairly quoted around the web and probably copy-pasted in a few add-ons :( There is an old revision of the article without the array comprehension. Diff view: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICryptoHash$compare?locale=en-US&to=102128&from=102127 Maybe someone can fix the documentation, at least for future uses of this function. ;)

(In reply to Shu-yu Guo [:shu] (PTO until 1/18) from comment #10) > Created attachment 8702781 [details] > find-comps.js > > Script used to find what js files have genexprs or comprehensions. How does one easily run this script over a codebase?

(In reply to Magnus Melin from comment #31) > How does one easily run this script over a codebase? By putting a \n-separated list of all relevant files, with paths relative to the current working directory, into a file called "js-files", and then, from the same directory, executing the script in a SpiderMonkey shell. You can download a current shell build here (see the "jsshell-*" files at the bottom): https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/

Also broke the FoxyProxy add-on from working.

Please file new bugs in the Tech Evangelism::Add-ons component and make them block this one for broken add-ons; thank you!

Could someone sum up which operators (is that the correct word?) were removed here? As it wasn't my impression that 'for (x in y)' was to be removed

(In reply to Jesper Hansen from comment #35) > Could someone sum up which operators (is that the correct word?) were > removed here? The syntax described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Array_comprehensions#Differences_to_the_older_JS1.7JS1.8_comprehensions https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Generator_comprehensions#Differences_to_the_older_JS1.7JS1.8_comprehensions > As it wasn't my impression that 'for (x in y)' was to be removed No, that would be very much impossible. This is about old, non-standard features that were only ever implemented in SpiderMonkey, and only available if you actively opted in to certain versions of JS.

(In reply to Till Schneidereit [:till] from comment #36) > The syntax described here: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/ > Array_comprehensions#Differences_to_the_older_JS1.7JS1.8_comprehensions > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/ > Generator_comprehensions#Differences_to_the_older_JS1.7JS1.8_comprehensions I'm adding these links to the site compat doc.

As an example for me and probably others meeting this bug over the next couple of weeks: Our code fails here: finalHash = [this.toHexString(hash.charCodeAt(i)) for (i in hash)].join(""); An example of (I think?): x for (x in y) This is what I was thinking of was also covered here?

there are several syntax that are removed here legacy array comprehensions: [X for (Y in Z)] [X for each (Y in Z)] [X for (Y of Z)] legacy generator comprehensions: (X for (Y in Z)) (X for each (Y in Z)) (X for (Y of Z)) for each case, there could be extra 'for ...'s and/or trailing 'if ()', like: [X for (Y in Z) for (U in V) for (S in T) if (W)] also, in legacy generator comprehensions, outmost parentheses can be a part of function call, like: f(X for (Y in Z))

The EFF's Privacy Badger add-on broke because it was using the old array comprehension example code from MDN. I submitted an add-on fix upstream: https://github.com/EFForg/privacybadgerfirefox/pull/722

Can we document some refactoring advice for people using the legacy patterns? I have this code which now breaks in Thunderbird daily: items = [new ftvItem(f) for each (f in recent)]; I guess I would have to refactor let items = []; for each (f in recent) { items.push(new ftvItem(f)) }; is this correct? My understanding is that the old syntax created an array and was able to populate from the inner for loop. Note that I was using for..each to remain Postbox 4.0 compatible (it does not support for..of yet and throws a Syntax error even if it doesn't execute the line) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Array_comprehensions should probably be updated with refactoring advice and links to this bug.

(In reply to Axel Grude [:realRaven] from comment #41) > let items = []; > for each (f in recent) { items.push(new ftvItem(f)) }; While that would probably work right now, you'd only be refactoring to the next deprecated thing: "for each" loops. Depending on what kind of object `recent` is, one of these would work: If it's an Array: let items = recent.map(f => new ftvItem(f)); If it's just some object: let items = Object.keys(recent).map(key => new ftvItem(recent[key])); (Soon, you'll also be able to use Object.entries(), but I need to land bug 1232639 first.)

(In reply to Till Schneidereit [:till] from comment #42) > (In reply to Axel Grude [:realRaven] from comment #41) > > let items = []; > > for each (f in recent) { items.push(new ftvItem(f)) }; correction: for each (let f in recent) { items.push(new ftvItem(f)) }; > > While that would probably work right now, you'd only be refactoring to the > next deprecated thing: "for each" loops. Urgh. Let's cross that bridge when we come to it. > Depending on what kind of object > `recent` is, one of these would work: > > If it's an Array: > let items = recent.map(f => new ftvItem(f)); > thanks for the explanation. I am not sure if can actually use the => syntax here because it is part of a "shim" which is aimed at Thunderbird > 12. If I wouldn't support Postbox I would make a clear cut and say only support Platform version > 40, but sadly I can't do this as long as they are lagging behind (AFAIK they still use Gecko 9) I think for supporting these lambda expressions they would need to upgrade their code base to at least to Gecko 22: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions So I am trying to keep up with the changes in the latest Mozilla branches I also have to be mindful of being backwards compatible. And while I am trying to understand the beautiful new Syntaxes of ES7 I also have to make sure that it doesn't pollute any web code that I am writing as it will probably be a long time until the other browser adopt it. So from the perspective of mdn it would be great to document alternative Syntaxes that are ES5 compatible as well.

(In reply to Axel Grude [:realRaven] from comment #43) > > While that would probably work right now, you'd only be refactoring to the > > next deprecated thing: "for each" loops. > > Urgh. Let's cross that bridge when we come to it. I'd heavily recommend against that approach: you'll just have to fix the code a second time, in a few months. And it's not even really simpler. > thanks for the explanation. I am not sure if can actually use the => syntax > here because it is part of a "shim" which is aimed at Thunderbird > 12. Then just use `function(f) { return new ftvItem(f); }` instead. Works exactly the same in this case. > So I am trying to keep up with the changes in the latest Mozilla branches I > also have to be mindful of being backwards compatible. And while I am trying > to understand the beautiful new Syntaxes of ES7 I also have to make sure > that it doesn't pollute any web code that I am writing as it will probably > be a long time until the other browser adopt it. Arrow functions are ES6 and supported in current versions of all major browsers. But yes, it'll be a bit longer until they can be used for general web code, agreed. Still, my examples don't rely on them at all, they're just a tiny bit shorter that way.