Last Comment Bug 1220564 - Remove legacy array/generator comprehension.
: Remove legacy array/generator comprehension.
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: Unspecified Unspecified
-- normal with 2 votes (vote)
: mozilla46
Assigned To: Shu-yu Guo [:shu]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1232618 1048636 1220565 1228976 1237602 1237603 1237605 1239753 1240401 1241429 1246378 1255566
Blocks: 1103158 eslint 1238254
  Show dependency treegraph
 
Reported: 2015-11-02 01:20 PST by Tooru Fujisawa [:arai]
Modified: 2016-03-10 12:27 PST (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Remove legacy generator comprehensions. (16.82 KB, patch)
2015-12-28 15:36 PST, Shu-yu Guo [:shu]
jwalden+bmo: review+
Details | Diff | Splinter Review
Remove legacy array comprehensions. (28.32 KB, patch)
2015-12-28 15:37 PST, Shu-yu Guo [:shu]
jwalden+bmo: review+
Details | Diff | Splinter Review
Update and remove obsolete jit-tests. (45.79 KB, patch)
2015-12-28 15:37 PST, Shu-yu Guo [:shu]
jwalden+bmo: review+
Details | Diff | Splinter Review
Update and remove obsolete JS reftests. (122.20 KB, patch)
2015-12-28 15:37 PST, Shu-yu Guo [:shu]
jwalden+bmo: review+
Details | Diff | Splinter Review
Remove chrome code uses of genexprs and legacy comprehensions. (21.78 KB, patch)
2015-12-29 18:59 PST, Shu-yu Guo [:shu]
wmccloskey: review+
Details | Diff | Splinter Review
find-comps.js (723 bytes, application/javascript)
2015-12-29 19:01 PST, Shu-yu Guo [:shu]
no flags Details
Update chrome code uses of genexprs and legacy comprehensions. (45.51 KB, patch)
2015-12-30 18:07 PST, Shu-yu Guo [:shu]
wmccloskey: review+
Details | Diff | Splinter Review

Description User image Tooru Fujisawa [:arai] 2015-11-02 01:20:34 PST
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
Comment 1 User image Kohei Yoshino [:kohei] 2015-11-02 02:04:46 PST Comment hidden (obsolete)
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2015-11-02 11:09:58 PST
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.
Comment 3 User image Tooru Fujisawa [:arai] 2015-11-02 12:06:09 PST
I see.  We should track the SpiderMonkey support separately for them, and only legacy comprehension for now :)
Comment 4 User image Kohei Yoshino [:kohei] 2015-11-02 13:36:12 PST
Updated the site compat doc accordingly: https://www.fxsitecompat.com/en-US/docs/2015/legacy-array-generator-comprehension-syntaxes-will-be-removed/
Comment 5 User image Shu-yu Guo [:shu] 2015-12-28 15:36:59 PST
Created attachment 8702404 [details] [diff] [review]
Remove legacy generator comprehensions.
Comment 6 User image Shu-yu Guo [:shu] 2015-12-28 15:37:05 PST
Created attachment 8702405 [details] [diff] [review]
Remove legacy array comprehensions.
Comment 7 User image Shu-yu Guo [:shu] 2015-12-28 15:37:11 PST
Created attachment 8702406 [details] [diff] [review]
Update and remove obsolete jit-tests.
Comment 8 User image Shu-yu Guo [:shu] 2015-12-28 15:37:18 PST
Created attachment 8702407 [details] [diff] [review]
Update and remove obsolete JS reftests.
Comment 9 User image Shu-yu Guo [:shu] 2015-12-29 18:59:32 PST
Created attachment 8702780 [details] [diff] [review]
Remove chrome code uses of genexprs and legacy comprehensions.
Comment 10 User image Shu-yu Guo [:shu] 2015-12-29 19:01:05 PST
Created attachment 8702781 [details]
find-comps.js

Script used to find what js files have genexprs or comprehensions.
Comment 11 User image Jan de Mooij [:jandem] 2015-12-30 04:19:30 PST
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 12 User image Jeff Walden [:Waldo] (remove +bmo to email) 2015-12-30 11:21:38 PST
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 13 User image Jeff Walden [:Waldo] (remove +bmo to email) 2015-12-30 11:24:18 PST
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 14 User image Jeff Walden [:Waldo] (remove +bmo to email) 2015-12-30 13:26:03 PST
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 15 User image Bill McCloskey (:billm) 2015-12-30 14:28:57 PST
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?
Comment 16 User image Shu-yu Guo [:shu] 2015-12-30 15:41:12 PST
(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?
Comment 17 User image Bill McCloskey (:billm) 2015-12-30 15:44:03 PST
> 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.
Comment 18 User image Shu-yu Guo [:shu] 2015-12-30 16:07:08 PST
(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; }())
Comment 19 User image Shu-yu Guo [:shu] 2015-12-30 18:07:15 PST
Created attachment 8703051 [details] [diff] [review]
Update chrome code uses of genexprs and legacy comprehensions.

It turns out I missed some files due to an error in my script, as well as a XUL
file.
Comment 20 User image Shu-yu Guo [:shu] 2015-12-31 00:15:24 PST
(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 21 User image Jeff Walden [:Waldo] (remove +bmo to email) 2015-12-31 14:15:15 PST
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.
Comment 25 User image Alice0775 White 2016-01-07 05:41:50 PST Comment hidden (obsolete)
Comment 26 User image Gary [:streetwolf] 2016-01-07 05:52:55 PST Comment hidden (obsolete)
Comment 27 User image Jan de Mooij [:jandem] 2016-01-07 07:05:50 PST
Since this is breaking some addons, should we start showing console warnings on aurora/beta? That gives them more time to update.
Comment 28 User image IU 2016-01-07 09:00:02 PST
My Addons Manager itself is busted and useless.  Have to revert to yesterday's nightly.
Comment 29 User image Giorgio Maone [:mao] 2016-01-07 10:25:05 PST
(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 :(
Comment 30 User image Sören Hentzschel 2016-01-07 10:47:39 PST
(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. ;)
Comment 31 User image Magnus Melin 2016-01-07 12:35:31 PST
(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?
Comment 32 User image Till Schneidereit [till] 2016-01-07 16:27:38 PST
(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/
Comment 33 User image Peja Stija 2016-01-07 20:33:56 PST
Also broke the FoxyProxy add-on from working.
Comment 34 User image :Ms2ger (⌚ UTC+1/+2) 2016-01-08 01:18:57 PST
Please file new bugs in the Tech Evangelism::Add-ons component and make them block this one for broken add-ons; thank you!
Comment 35 User image Jesper Hansen 2016-01-08 06:38:14 PST
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
Comment 36 User image Till Schneidereit [till] 2016-01-08 06:58:15 PST
(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.
Comment 37 User image Kohei Yoshino [:kohei] 2016-01-08 07:03:47 PST
(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.
Comment 38 User image Jesper Hansen 2016-01-08 16:47:34 PST
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?
Comment 39 User image Tooru Fujisawa [:arai] 2016-01-08 17:56:39 PST
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))
Comment 40 User image Chris Peterson [:cpeterson] 2016-01-24 22:29:14 PST
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
Comment 41 User image Axel Grude [:realRaven] 2016-01-27 03:23:53 PST
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.
Comment 42 User image Till Schneidereit [till] 2016-01-27 03:34:37 PST
(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.)
Comment 43 User image Axel Grude [:realRaven] 2016-01-27 04:04:44 PST
(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.
Comment 44 User image Till Schneidereit [till] 2016-01-27 09:17:23 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.