Closed Bug 995610 Opened 6 years ago Closed 4 years ago

Add console warnings for expression closures (shorthand function syntax)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: evilpie, Assigned: arai)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(6 files, 1 obsolete file)

Attached patch shorthand (obsolete) — Splinter Review
I think luckily almost nobody knows about this!
var x = function() 1;
Attachment #8405765 - Flags: review?(jorendorff)
Attached patch shorthandSplinter Review
Starting Firefox with this patch show quite a few warnings mostly because of code like:
{
  get foo() "bar"
}
Assignee: nobody → evilpies
Attachment #8405765 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8405765 - Flags: review?(jorendorff)
Attachment #8405769 - Flags: review?(jorendorff)
Tom: is this shorthand function syntax a SpiderMonkey-specific language extension? Is it JS version-gated?
Flags: needinfo?(evilpies)
OS: Linux → All
Hardware: x86_64 → All
1) Yes 2) No
Flags: needinfo?(evilpies)
If we add telemetry for language extensions in bug 988386, we could track shorthand function syntax, too. Perhaps someday we could remove this feature. :)
Blocks: 988386
Attachment #8405769 - Flags: review?(jorendorff) → review+
I haven't landed this yet, because I was busy and we probably should fix some in-tree problems first.
Attached file log
Just the warnings form starting a debug browser build.
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.
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?
Flags: needinfo?(evilpies)
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.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
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.
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.
Flags: needinfo?(evilpies)
No longer blocks: 988386
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.
See Also: → 1054630
Attached patch browser fixesSplinter Review
This fixes a few hundred of these warnings in the browser.
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(),
(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>
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(); },
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.
Blocks: 1083453
Summary: Warn about shorthand function syntax → Add console warnings for expression closures (shorthand function syntax)
Chris, would you mind taking over this bug?
Flags: needinfo?(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
Assignee: evilpies → cpeterson
Flags: needinfo?(cpeterson)
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.
Depends on: 1113031
Depends on: 1113032
Keywords: addon-compat
Blocks: 1122724
Blocks: 1083458
No longer blocks: 1083459
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
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.
Attachment #8681109 - Flags: review?(evilpies)
Attachment #8681109 - Flags: review?(evilpies) → review+
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.
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 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.
Attachment #8681110 - Flags: review?(evilpies) → review+
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
https://hg.mozilla.org/mozilla-central/rev/bcc2c1f35e9c
https://hg.mozilla.org/mozilla-central/rev/40b7242a0deb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.)
Flags: needinfo?(arai.unmht)
thank you :)
Assignee: cpeterson → arai.unmht
Flags: needinfo?(arai.unmht)
Thanks, arai! :)
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/
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.