995610 - Add console warnings for expression closures (shorthand function syntax)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: shorthand

I think luckily almost nobody knows about this!
var x = function() 1;

Comment 1

[Tom S [:evilpie]]

Attachment: shorthand

Starting Firefox with this patch show quite a few warnings mostly because of code like:
{
  get foo() "bar"
}

Comment 2

[Chris Peterson [:cpeterson]]

Tom: is this shorthand function syntax a SpiderMonkey-specific language extension? Is it JS version-gated?

Comment 3

[Tom S [:evilpie]]

1) Yes 2) No

Comment 4

[Chris Peterson [:cpeterson]]

If we add telemetry for language extensions in bug 988386, we could track shorthand function syntax, too. Perhaps someday we could remove this feature. :)

Comment 5

[Tom S [:evilpie]]

I haven't landed this yet, because I was busy and we probably should fix some in-tree problems first.

Comment 6

[Tom S [:evilpie]]

Attachment: log

Just the warnings form starting a debug browser build.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Can this warning be #ifndef RELEASE_BUILD? I think the value of the warning is for developers on Nightly and looking on tbpl to see new warnings that might be introduced, but there's no need to tell end users about internal issues like this..  Specially as there's add-on SDK code using the shorthand syntax, which means it may take a while to totally get rid of it from add-ons.

Comment 8

[Chris Peterson [:cpeterson]]

evilpie: do you plan to land this patch? To address Felipe's concern, perhaps you could limit the warnings to JS version < JSVERSION_1_8? Do chrome and add-on JS code default to JSVERSION_1_8?

Comment 9

[Florian Scholz (Open Web Docs)]

Is this about "expression closures" introduced in JS 1.8/ Firefox 3 (bug 381113)?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Closures#Expression_closures
I think the docs need updating to say it's non-standard/deprecated then.

Comment 10

[Jeff Walden [:Waldo]]

Limiting to < 1.8 is mostly unuseful, because <script> in XUL defaults to latest version, so ~nobody ever explicitly sets a version.  I'd limit this warning to once per script in order to land it, using a bit in JSScript to track.

(In reply to Florian Scholz [:fscholz] (elchi3) from comment #9)
> Is this about "expression closures" introduced in JS 1.8/ Firefox 3 (bug
> 381113)?

Yes.

Comment 11

[Tom S [:evilpie]]

I still plan on doing this, maybe I will work on this today. Fixing all those warnings and getting them reviewed is just not a lot of fun.

Comment 12

[Chris Peterson [:cpeterson]]

btw, I landed patches (in bug 1054630) to collect JS parser telemetry about the use of deprecated language extensions (in web content, not add-ons or chrome JS), including expression closures.

Comment 13

[Tom S [:evilpie]]

Attachment: browser fixes

This fixes a few hundred of these warnings in the browser.

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 8481537 [details] [diff] [review]
browser fixes

Review of attachment 8481537 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/source/lib/sdk/io/fs.js
@@ +248,5 @@
>      let file = new nsILocalFile(path);
>      if (!file.exists()) throw FSError("stat", "ENOENT", 34, path);
>      nsIFile(this, file);
>    },
> +  isDirectory: function() { return nsIFile(this).isDirectory(); },

Why not use ES6 arrow functions? They're even more concise than the expression closures you're replacing. :)

  isDirectory: () => nsIFile(this).isDirectory(),

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Chris Peterson (:cpeterson) from comment #14)
> > +  isDirectory: function() { return nsIFile(this).isDirectory(); },
> 
> Why not use ES6 arrow functions? They're even more concise than the
> expression closures you're replacing. :)
> 
>   isDirectory: () => nsIFile(this).isDirectory(),

Concisely wrong.

Arrow functions capture |this| from their enclosing environment.  That's not the same as whatever object literal contains this function.  It's not the same as any object that contains this object literal on its prototype chain.  (I don't know which of these is the case, seems one or the other, and I'm too lazy to look.)  If |this| is to refer to |obj| in |obj.isDirectory()|, then you need a normal function definition.

This confusion is one of the reasons I think it's twenty years too late to add another kind of function syntax to JS, at least if it's going to have semantics any different at all from traditional functions.  People are going to make mistakes like this when there are two forms with different semantics.  Particularly when the shorter form is held out as the "right" form for short functions.  It usually works.  But not always.  TIMTOWTDI is going to lead to more confusion than just ensuring people know about the inheriting gotcha and working around it will.

But whatever, people seem insistent on it as "fixing" stuff even tho it doesn't really, and I can't be bothered to care that much.

</cantankering>

Comment 16

[Till Schneidereit [:till]]

While I think that for most of these use cases, arrow functions will work just splendidly, there are also ES6 method definitions, which make this only four characters longer:

isDirectory() { return nsIFile(this).isDirectory(); },

Comment 17

[Tom S [:evilpie]]

I tried to be very conservative with using arrow functions to make it easier to review.
1. If there was enough space I usually just inserted "{ return ... }"
2. If we are tight on space and there is no |this| usage, I sometimes picked arrow functions
3. If this is used in a function that was previously defined like (function() blub).bind(this) or called by map etc. like .map(function() foo, this) -> arrow
4. In all other cases with |this| usage I kept normal functions.

Comment 18

[Tom S [:evilpie]]

Chris, would you mind taking over this bug?

Comment 19

[Chris Peterson [:cpeterson]]

Sure. Unfortunately, removing support for expression closures might not be practical. Based on the JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT telemetry probe, expression closures are one of the more popular SpiderMonkey language extensions (~18M page views from Beta 34 users):

https://telemetry.mozilla.org/#filter=beta%2F34%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT

Comment 20

[Tom S [:evilpie]]

Thanks Chris.

We should really try to look at some corpus of the top sites and figure out why that is. As far as I know, no other browser actually supports this. My theory is that this is actually caused by some buggy script that gets truncated or similar. It's just used so much more than any other non-standard feature.

Comment 21

[Kohei Yoshino]

Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/37/Site_Compatibility

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: (draft) Show deprecated warning for expression closure.

Bug 1219837 is removing remaining last a few use.  This bug could be fixed after that.

As expression closure could be used a lot in Add-ons, showing warning for every occurrence will flood error console.  So this patch shows one warning per compartment.

Then, I overlooked about the non-release-only restriction in discussion.  I'll implement it shortly (now searching a way to test it).

Green on try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f390a6b6850f

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add release property to getBuildConfiguration().

Added getBuildConfiguration().release property that is true when RELEASE_BUILD is defined.  This can be used to test non-release-only thing in shell test.

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Show deprecated warning for expression closure.

Comment 25

[Tom S [:evilpie]]

Comment on attachment 8681110 [details] [diff] [review]
Part 1: Show deprecated warning for expression closure.

Review of attachment 8681110 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +9573,5 @@
> +    if (!cx)
> +        return true;
> +
> +    // Report only if error reporter is non-null to report once and only once.
> +    if (!cx->compartment()->warnedAboutExprClosure && cx->runtime()->errorReporter) {

Why check cx->runtime()->errorReporter at all? I am not aware that we do this anywhere else.

::: js/src/js.msg
@@ +243,5 @@
>  MSG_DEF(JSMSG_CURLY_IN_COMPOUND,       0, JSEXN_SYNTAXERR, "missing } in compound statement")
>  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_NONE, "expression closure is deprecated")

I think "expression closures are deprecated" is a bit nicer.

Comment 26

[Tooru Fujisawa [:arai]]

Thank you :)

(In reply to Tom Schuster [:evilpie] from comment #25)
> ::: js/src/frontend/Parser.cpp
> @@ +9573,5 @@
> > +    if (!cx)
> > +        return true;
> > +
> > +    // Report only if error reporter is non-null to report once and only once.
> > +    if (!cx->compartment()->warnedAboutExprClosure && cx->runtime()->errorReporter) {
> 
> Why check cx->runtime()->errorReporter at all? I am not aware that we do
> this anywhere else.

oh, sorry I should've noted that.  JS_BufferIsCompilableUnit sets errorReporter to nullptr (it's called in js shell prompt).  So, if we want to report warning once and only once, at lease we should avoid setting warnedAboutExprClosure to true if it's nullptr, otherwise the message will be lost.
Maybe we could just ignore JS_BufferIsCompilableUnit's case?

Comment 27

[Tom S [:evilpie]]

Comment on attachment 8681110 [details] [diff] [review]
Part 1: Show deprecated warning for expression closure.

Review of attachment 8681110 [details] [diff] [review]:
-----------------------------------------------------------------

Just remove it. The shell is not that important.

Comment 28

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc2c1f35e9c5ced31fb6459866772c1d008e246
Bug 995610 - Part 0: Add release property to getBuildConfiguration(). r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/40b7242a0deb74bc3512f31bd1734b7943dfcc0e
Bug 995610 - Part 1: Show deprecated warning for expression closure. r=evilpie

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bcc2c1f35e9c
https://hg.mozilla.org/mozilla-central/rev/40b7242a0deb

Comment 30

[Daniel Holbert [:dholbert]]

Should this bug have assignee=arai?

(Looks like it's set to cpeterson right now, since he was going to finish it off at some point, but arai ended up taking care of it.)

Comment 31

[Tooru Fujisawa [:arai]]

thank you :)

Comment 32

[Chris Peterson [:cpeterson]]

Thanks, arai! :)

Comment 33

[Kohei Yoshino]

Posted the site compatibility doc again. I forgot why I pushed the doc for Firefox 37... (Comment 21)

https://www.fxsitecompat.com/en-US/docs/2015/expression-closures-are-now-deprecated/

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bcc2c1f35e9c
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/40b7242a0deb

Comment 35

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion