1083458 - Remove SpiderMonkey support for expression closures (shorthand function syntax)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Closures#Expression_closures

Comment 1

[Florian Scholz (Open Web Docs)]

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

Comment 2

[Kohei Yoshino [:kohei]]

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

Comment 3

[Tooru Fujisawa [:arai]]

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) :/

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: sca.17.1.7.js

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

Comment 5

[Eric Jung [:ericjung]]

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;}

Comment 6

[Chris Peterson [:cpeterson]]

(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

Comment 7

[Juriy "kangax" Zaytsev]

Or `getHelperForLanguage: () => null`

Comment 8

[Ian Nartowicz]

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

Comment 9

[Eric Jung [:ericjung]]

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

Comment 10

[Tooru Fujisawa [:arai]]

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)

Comment 11

[Tooru Fujisawa [:arai]]

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)

Comment 12

[Tooru Fujisawa [:arai]]

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

Comment 13

[Jan de Mooij [:jandem]]

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

Comment 14

[Tooru Fujisawa [:arai]]

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

Comment 16

[Tom S [:evilpie]]

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.

Comment 17

[Tom S [:evilpie]]

Attachment: Remove basic expression closure code

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 18

[Tom S [:evilpie]]

Attachment: Remove expression closure tests

Comment 19

[Jan de Mooij [:jandem]]

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.

Comment 20

[Tom S [:evilpie]]

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

Comment 21

[Tom S [:evilpie]]

Attachment: Remove isExprBody

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; }.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Noemi Erli[:noemi_erli]]

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

Comment 24

[Tom S [:evilpie]]

Attachment: Remove isExprBody

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 25

[Tom S [:evilpie]]

Attachment: Remove expression closure warning

Comment 26

[Jeff Walden [:Waldo]]

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 27

[Jan de Mooij [:jandem]]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7010443cce
Remove expression closure warning. r=jandem

Comment 29

[Tom S [:evilpie]]

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.

Comment 30

[Tom S [:evilpie]]

Attachment: Remove isExpressionClosure from Parser

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

Comment 31

[Tom S [:evilpie]]

Attachment: Remove isExprBody from everything but FunctionBox to keep Reflect.parse working

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.

Comment 32

[Tom S [:evilpie]]

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

Comment 33

[Jeff Walden [:Waldo]]

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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64682e402373
Remove isExpressionClosure from Parser. r=Waldo

Comment 35

[Jeff Walden [: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.

Comment 36

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7d7010443cce

Comment 37

[Tom S [:evilpie]]

(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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a703ca86a96
Remove isExprBody from everything but FunctionBox to keep Reflect.parse working. r=Waldo

Comment 39

[Eliza Balazs [:ebalazs_]]

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