MDN content has been moved to a separate page to mark this clearly as non-standard/deprecated. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Expression_closures

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/expression-closure-support-will-be-removed/

current telemetry result CONTENT | Nightly | Aurora | Beta | Release ---+---------+---------+---------+---------- 45 | 55.72k | 150.11k | 1.23M | 1.88M 46 | 47.26k | 110.75k | 1.79M | 1.64M 47 | 54.42k | 136.06k | 2.13M | 2.82M 48 | 61.91k | 148.77k | 2.16M | 6.25M 49 | 45.91k | 107.39k | 4.63M | 20.49M 50 | 45.41k | 215.39k | 13.41M | - 51 | 52.06k | 667.19k | - | - 52 | 450.70k | - | - | - ADDON | Nightly | Aurora | Beta | Release ---+---------+---------+---------+--------- 45 | 93.77k | 195.58k | 170.83k | 97.52k 46 | 56.12k | 208.61k | 94.02k | 63.28k 47 | 55.44k | 200.31k | 78.3k | 231.89k 48 | 77.46k | 158.59k | 175.29k | 280.96k 49 | 60.46k | 209.24k | 126.68k | 3.83M 50 | 59.29k | 162.04k | 1.24M | - 51 | 19.68k | 252.67k | - | - 52 | 88.94k | - | - | - not sure what happens tho, it hits spike on last cycle (that is not specific version)... anyway, we don't observe any reduction after showing warning (bug 995610) :/

I have actually seen this expression closure warning on a real website! The following BBC News page very intermittently logs an expression closure warning, depending on which ads (from Rubicon Project ad network) are loaded. http://www.bbc.com/news/world-us-canada-38656814 JavaScript warning: https://cdn.adsafeprotected.com/sca.17.1.7.js, line 1: expression closures are deprecated The script is using eval() to detect support for expression closures, and other browser features, to fingerprint users. Search for "expressionClosure" in the attached sca.17.1.7.js file. This probably explains why expression closures are so common in JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT telemetry: https://mzl.la/2jaLfZL JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDONS telemetry reports plenty of add-ons still using expression closures, but I think it's worth testing whether we can at least hide expression closures from regular web content. Expression closures are a deprecated language feature we'd like to remove and are actively being used to fingerprint users. I realize that detecting expression closure support does not contribute any meaningful entropy for fingerprinting (because all Firefox users support expression closures and no non-Firefox users do).

I'd like to remove these from my addon. Here is how it's used: getHelperForLanguage: function(l) null Is this just a function returning null? Can I replace that with: getHelperForLanguage: function() {return null;}

(In reply to Eric H. Jung from comment #5) > I'd like to remove these from my addon. Here is how it's used: > > getHelperForLanguage: function(l) null > > Is this just a function returning null? Can I replace that with: > > getHelperForLanguage: function() {return null;} Yes and your change is correct. In your particular example defining a function as part of an object literal, you could also use an ES6's shorter method definition syntax like: getHelperForLanguage() {return null;}, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

Or `getHelperForLanguage: () => null`

Shorthand method definitions introduced in Firefox 33, still not available in IE or whatever it is called now. The important bit related to this bug is placing the curly brackets around whatever expression is to be returned and using the return keyword (with or without a semi-colon).

(In reply to Chris Peterson [:cpeterson] from comment #6) > Yes and your change is correct. > > In your particular example defining a function as part of an object literal, > you could also use an ES6's shorter method definition syntax like: > > getHelperForLanguage() {return null;}, > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/ > Method_definitions Thanks. This will be removed from FoxyProxy Standard in its next release (4.6) and FoxyProxy Basic in its next release (3.6)

telemetry table generated by the script in bug 1083470 comment #6 CONTENT | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 59745 | 343200 | 1391066 | 2034283 46 | 48009 | 112849 | 2025797 | 1729146 47 | 55504 | 138089 | 2421614 | 3319298 48 | 64231 | 158405 | 2641369 | 7590326 49 | 47739 | 111023 | 4958256 | 21556859 50 | 47139 | 221618 | 13979525 | 23663556 51 | 54845 | 719252 | 13876764 | 6371716 52 | 462630 | 620659 | 3829983 | 22 53 | 425658 | 190734 | - | 1 54 | 109505 | - | - | - ADDONS | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 95761 | 197887 | 219781 | 417992 46 | 57778 | 209156 | 199249 | 191959 47 | 55785 | 202172 | 288088 | 1809065 48 | 88436 | 168291 | 462228 | 1958165 49 | 74099 | 221353 | 287324 | 5265871 50 | 82582 | 186774 | 1634315 | 92150325 51 | 42056 | 325700 | 15330684 | 25679298 52 | 144776 | 1684192 | 5038724 | - 53 | 1231626 | 558765 | - | - 54 | 358656 | - | - | - (note: latest versions don't have enough data to compare)

CONTENT | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 63387 | 472713 | 1501298 | 2138490 46 | 48548 | 114212 | 2170640 | 1797932 47 | 55810 | 141089 | 2613280 | 3702689 48 | 67800 | 171864 | 3495916 | 9282126 49 | 49071 | 113332 | 5231487 | 22042456 50 | 48127 | 225680 | 14234711 | 24530855 51 | 56322 | 728323 | 14458774 | 15946811 52 | 468205 | 652239 | 8399492 | 14079882 53 | 440372 | 526532 | 8879646 | 17741299 54 | 222092 | 1018247 | 11810352 | 1045938 55 | 576031 | 415 | 203703 | - 56 | 52932 | - | - | - ADDONS | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 96289 | 198353 | 250751 | 674045 46 | 57781 | 209270 | 249379 | 320672 47 | 56830 | 202477 | 578530 | 3153536 48 | 91775 | 178833 | 779363 | 3881537 49 | 90025 | 229458 | 442268 | 5774554 50 | 85606 | 200705 | 1863232 | 94209611 51 | 51140 | 345110 | 15767022 | 54204632 52 | 154409 | 1785597 | 10208301 | 51481376 53 | 1296902 | 1222045 | 9225468 | 68124841 54 | 673167 | 1844268 | 12196295 | 4912235 55 | 1398464 | 23213 | 518631 | - 56 | 116199 | - | - | - (note: latest versions don't have enough data to compare)

CONTENT | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 65825 | 488304 | 1562563 | 2190818 46 | 48853 | 115012 | 2249984 | 1835235 47 | 56154 | 143167 | 2722575 | 3894112 48 | 69613 | 180443 | 4066094 | 10351494 49 | 49644 | 114778 | 5406076 | 22271369 50 | 48827 | 227295 | 14366430 | 25074950 51 | 57240 | 733307 | 14700508 | 16143939 52 | 470268 | 663610 | 8512646 | 14516458 53 | 446556 | 573104 | 9052273 | 18449700 54 | 224067 | 1043255 | 12846193 | 19416714 55 | 595912 | 7975 | 11642883 | 12437826 56 | 325609 | 14292 | 8394049 | - 57 | 550462 | - | - | - ADDONS | nightly | aurora | beta | release ----+------------+------------+------------+------------ 45 | 96546 | 199130 | 282454 | 771377 46 | 57783 | 209471 | 284954 | 384743 47 | 56980 | 202494 | 687073 | 3871930 48 | 92822 | 180327 | 948996 | 4832679 49 | 96946 | 230072 | 515861 | 5939663 50 | 92087 | 207486 | 1946932 | 94619533 51 | 56986 | 347886 | 15944034 | 54965267 52 | 155165 | 1810108 | 10262028 | 52136309 53 | 1369975 | 1252034 | 9321603 | 70237351 54 | 673297 | 1940356 | 12883840 | 67360509 55 | 1441130 | 62231 | 10422325 | 39430176 56 | 611217 | 25221 | 7184891 | - 57 | 121557 | - | - | -

(In reply to Chris Peterson [:cpeterson] from comment #4) > The script is using eval() to detect support for expression closures, and > other browser features, to fingerprint users. Search for "expressionClosure" > in the attached sca.17.1.7.js file. This probably explains why expression > closures are so common in JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT > telemetry: https://mzl.la/2jaLfZL That makes sense. I think we should try removing support for expression closures on Nightly after the merge next week, then we can see if it breaks anything.

CONTENT | nightly | aurora | beta | release ----+------------+------------+------------+------------ 52 | 470609 | 667523 | 8541451 | 14592467 53 | 448157 | 581558 | 9093863 | 18514282 54 | 224927 | 1047737 | 12939087 | 19548228 55 | 599048 | 8023 | 11787706 | 13714351 56 | 330849 | 14917 | 8908219 | 10469463 57 | 560252 | 28214 | 9478448 | 62 58 | 625411 | 65270 | 88 | 17 ADDONS | nightly | aurora | beta | release ----+------------+------------+------------+------------ 52 | 155313 | 1820362 | 10292035 | 52296698 53 | 1411538 | 1256300 | 9347465 | 70430425 54 | 673350 | 1946785 | 13051554 | 67768977 55 | 1448950 | 62231 | 10535246 | 43112554 56 | 635965 | 26771 | 7735926 | 31036552 57 | 142467 | 4137 | 1109 | 1 58 | 35348 | 13 | - | -

We already disabled expression closures in bug 1426519. We can completely remove the disabled code after this reaches stable (Firefox 60) and everything looks okay.

This mostly just removes the code that was disabled in bug 1426519, but there is more stuff to delete. Especially this: - // If we remove expression closure, we can remove isExprBody flag from - // LazyScript and JSScript.

Comment on attachment 8961064 [details] [diff] [review] Remove basic expression closure code Review of attachment 8961064 [details] [diff] [review]: ----------------------------------------------------------------- \o/ Will you land this in Firefox 61 or 62? Comment 16 suggests 62 but IMO we could land this now.

Thanks for the review. I actually forgot about that comment, we are probably clear to land this now.

This patch isn't finished, because Reflect.parse actually needs isExprBody, but I am not sure what to replace it with. I probably need to look deeper into how we parse arrow functions to fix it myself. This line is wrong: isExpression = type == AST_ARROW_EXPR. Apparently we need/want to tell the difference between () => 1 and () => { return 1; }.

Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dce3a6c9866b Remove basic expression closure code. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/0cac0a8b22db Remove expression closures from tests. r=jandem

https://hg.mozilla.org/mozilla-central/rev/dce3a6c9866b https://hg.mozilla.org/mozilla-central/rev/0cac0a8b22db

So the basic problem here is the "expression" value in Reflect.parse. For () => 1 this should be true, but false for () => { return 1 } it should be false, because the right hand side is a block and not an expression. However for `async () => 1` we build the same StatementList as `async () => { return 1`. I solved this by pattern matching, which is incorrect for that case. I think we could also keep isExprBody on FunctionBox?

Comment on attachment 8964182 [details] [diff] [review] Remove isExprBody Review of attachment 8964182 [details] [diff] [review]: ----------------------------------------------------------------- I'd be happier if this were done in steps. Remove {is,note}ExpressionClosure as one standalone patch and land that. Then, in a next patch, after Searchfox has updated so we can reasonably see what's dead/not-dead/barely-used, start removing the exprbody stuff. I don't think I'm okay with removing exprbody in advance of figuring out a different way to record arrow functions with expression bodies, tho -- which I gather from the async.js changes is what this patch would produce. I'd be inclined to see if we can't figure out a way to split up the is-arrow-function representation into is-arrow-block and is-arrow-expression, but I'm not at all sure of that. Right now there's too much moving parts to this patch for me to be sure I grasp it all well.

Comment on attachment 8964183 [details] [diff] [review] Remove expression closure warning Review of attachment 8964183 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/js.msg @@ -242,4 @@ > MSG_DEF(JSMSG_DECLARATION_AFTER_EXPORT,0, JSEXN_SYNTAXERR, "missing declaration after 'export' keyword") > MSG_DEF(JSMSG_DECLARATION_AFTER_IMPORT,0, JSEXN_SYNTAXERR, "missing declaration after 'import' keyword") > MSG_DEF(JSMSG_DEPRECATED_DELETE_OPERAND, 0, JSEXN_SYNTAXERR, "applying the 'delete' operator to an unqualified name is deprecated") > -MSG_DEF(JSMSG_DEPRECATED_EXPR_CLOSURE, 0, JSEXN_WARN, "expression closures are deprecated") I think we can also remove: * JSMSG_DEPRECATED_BLOCK_SCOPE_FUN_REDECL * JSMSG_DEPRECATED_FOR_EACH Also, JSMSG_DEPRECATED_EXPR_CLOSURE is used here: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/server/actors/errordocs.js#56 Not sure what the process is for removing from that list...

Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7010443cce Remove expression closure warning. r=jandem

Comment on attachment 8964182 [details] [diff] [review] Remove isExprBody Splitting up the isExpressionClosure part makes sense. I think after that it's pretty clear to see that isExprBody is only used by Reflect.parse though.

We know that isExpressionClosure in FullParseHandler always returns false, because isExprBody && !isArrow can't be true anymore without the shorthand function syntax.

I am pretty sure Reflect.parse only ever sees non-lazy functions, so we should be totally fine with removing this flag from everything but the FunctionBox.

In the asm.js case we call ParseFunction first, which looks for the "function" token, so it can't be an arrow function.

Comment on attachment 8964527 [details] [diff] [review] Remove isExpressionClosure from Parser Review of attachment 8964527 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/SyntaxParseHandler.h @@ +137,4 @@ > }; > > bool isNonArrowFunctionExpression(Node node) const { > + return node == NodeFunctionExpressionBlockBody; We should rename that to just NodeFunctionExpression now (or in some patch here).

Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/64682e402373 Remove isExpressionClosure from Parser. r=Waldo

Comment on attachment 8964534 [details] [diff] [review] Remove isExprBody from everything but FunctionBox to keep Reflect.parse working Review of attachment 8964534 [details] [diff] [review]: ----------------------------------------------------------------- Huh, so we were XDRing isExprBody in all this stuff for...no actual reason? Weird.

(In reply to Jeff Walden [:Waldo] from comment #35) > Comment on attachment 8964534 [details] [diff] [review] > Remove isExprBody from everything but FunctionBox to keep Reflect.parse > working > > Review of attachment 8964534 [details] [diff] [review]: > ----------------------------------------------------------------- > > Huh, so we were XDRing isExprBody in all this stuff for...no actual reason? > Weird. Thanks for the review. The only important use of isExprBody was removed in the first patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/dce3a6c9866b#l2.16

Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a703ca86a96 Remove isExprBody from everything but FunctionBox to keep Reflect.parse working. r=Waldo

https://hg.mozilla.org/mozilla-central/rev/64682e402373 https://hg.mozilla.org/mozilla-central/rev/8a703ca86a96