1046964 - Lots of JS warnings printed at startup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm seeing a ton of warnings about anonymous functions not returning a value as well as other extraWarnings stuff. I think this is a regression from bug 940305. I'd like to disable the return value warning and fix the other stuff.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: warning output

This is what I get when starting up and then immediately shutting down.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: kill-retval-warning

This gets rid of the "missing return statement" warning, which seems pretty useless.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: prologue-fix

This fixes the "useless expression" warning so it never warns about "use asm" or "use strict". People are stitching together lots of .js files using #include and sticking "use strict" at the top of all of them. I guess ideally we'd continue to warn if the expression is not in the context of a correct "use strict" directive, but I don't know how to do that. I'm happy to code it up if you know how Jeff.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: js-warn-fixes

This patch fixes a ton of JS warnings. Hope you don't mind reviewing Tim. Most of these are straightforward. The interesting ones are:

In FxAccountsCommon, we're doing EXPORT_SYMBOLS = Object.keys(this). That's bad because we end up exporting Cu and Ci and other random stuff. Then when other people import, their Cu and Ci get overwritten, which triggers a warning if they've been defined using const. I tightened this up by putting everything we want to export into an |exports| object. However, a more conservative change would be to |delete Cu| and |delete Ci| before the Object.keys business.

Also, there were two places where we have an xpcom-shutdown observer that does:
  for (var i in this) this[i] = null;
This triggers a warning for properties on the object that have getters and not setters. I changed the code to:
  for (var i in this) delete this[i];
I don't think there's anything wrong with this.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8465969 [details] [diff] [review]
kill-retval-warning

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

\o/

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Warn when encountering a string expression statement, that would be a directive but isn't in the right position, when respecting the non-directive would change semantics

I'm not okay with just losing the warning when we have a perfectly reasonable idea what semantics would be better, and we just want to be lazy about not doing the right thing.

Technically the useAsm part of this isn't necessary -- there's already a warning for asm.js directives out of position.  But if we're going to do this, we should do it such that all directives get equal treatment.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8465971 [details] [diff] [review]
prologue-fix

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

I'd rather see us do the right thing here.  Better patch posted.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8468126 [details] [diff] [review]
Warn when encountering a string expression statement, that would be a directive but isn't in the right position, when respecting the non-directive would change semantics

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

r=me, though I'm not particularly familiar with this code.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5552,5 @@
> +    } else if (pn->isDirectivePrologueMember()) {
> +        // Don't complain about directive prologue members; just don't emit
> +        // their code.
> +    } else {
> +        if (JSAtom *atom = pn->isStringExprStatement()) {

Nit: could this if/else be merged into the parent if/else chain?

::: js/src/jsfun.cpp
@@ -59,5 @@
> -/* NB: no sentinels at ends -- use ArrayLength to bound loops.
> - * Properties censored into [[ThrowTypeError]] in strict mode. */
> -static const uint16_t poisonPillProps[] = {
> -    NAME_OFFSET(arguments),
> -    NAME_OFFSET(caller),

The changes in this function seem unrelated. Are they?

::: js/src/tests/ecma_5/extensions/misplaced-inconsistent-directive.js
@@ +2,5 @@
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/
> + * Contributor:
> + *   Jeff Walden <[e-mail]>

We still bother with contributor names in files like this? Huh. Removing them from all the .cpp/.h files was a win, IMO...

@@ +48,5 @@
> +
> +// No errors expected -- useless non-directives, but not contrary to used
> +// semantics.
> +evaluateNoRval("'use strict'; function f4() {} 'use strict'; function f5() {}");
> +evaluateNoRval("'use strict'; function f6() { var z; 'use strict' }");

You have good coverage for 'use strict', but none for 'use asm'. Add some?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The missing return value patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a470d0cbe3fa

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for jsreftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1388026f8a

https://tbpl.mozilla.org/php/getParsedLog.php?id=45578873&tree=Mozilla-Inbound

Comment 11

[[e-mail]]

(In reply to Bill McCloskey from comment #2)
> This gets rid of the "missing return statement" warning, which seems pretty
> useless.

Why is it useless? We wouldn't tolerate it in C++ for instance.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d400055c9999

> Why is it useless? We wouldn't tolerate it in C++ for instance.

C++ functions have return types, so it's a lot easier to generate useful warnings there.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb3cfc3ef3f3

Comment 14

[Tom S [:evilpie]]

I think you could have removed funHasReturnExpr and funHasReturnVoid.

Comment 15

[Chris Peterson [:cpeterson]]

(In reply to Bill McCloskey (:billm) from comment #12)
> > Why is it useless? We wouldn't tolerate it in C++ for instance.
> 
> C++ functions have return types, so it's a lot easier to generate useful
> warnings there.

Wasn't the "missing return statement" warning only emitted when a function returns a value on some code paths but not others? That seems like a genuinely useful warning about a programmer oversight.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here's a very common example where this doesn't work well. We have a receiveMessage method like this:

receiveMessage: function(msg) {
  switch (msg.name) {
    case "AsyncMsg1": ... break;
    case "AsyncMsg2": ... break;
    case "SyncMsg":
      return computedResult();
    default:
      print_error();
      break;
  }
}

In this case, only the "SyncMsg" branch returns a value, which is perfectly fine. The return value is ignored for async messages. We have hundreds of these sorts of methods all over the codebase. It just doesn't make sense to me to force people to add "return null" to the end of each one.

These sorts of static analyses have to be weighed carefully. The other day we had a serious regression in the code for XHR because someone was trying to fix a Coverity warning. The original code had been correct.

In the case of the return value warning, I think the false positive rate is way too high. Just because it might identify bugs doesn't justify its existence.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d400055c9999

Comment 18

[Jeff Walden [:Waldo]]

The doesn't-always-return analysis was also pretty spectacularly dumb.  It just looks at the last parse node in the function and complains if it's not a return.  Putting a function at the end of the code confuses it (bug 425827).  I think (but haven't investigated, only observed a very-confusing warning) every asm.js function has a non-return statement at the end (in the parse tree, not in actual syntax), so asm.js functions trigger it.  Functions ending in loops that never break, but exit by returning, I believe also confused it.  Without an actual principled dominator analysis, this was always rather broken.

Comment 19

[Jan de Mooij [:jandem]]

Clang is warning about unused HasFinalReturn functions:

Parser.cpp:655:1: warning: function 'HasFinalReturn' is not needed and will not be emitted [-Wunneeded-internal-declaration]
HasFinalReturn(ParseNode *pn)
^
Parser.cpp:770:1: warning: unused function 'HasFinalReturn' [-Wunused-function]
HasFinalReturn(SyntaxParseHandler::Node pn)
^

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: dead-code

This removes the dead code Jan pointed out. It looks like we still use funHasReturnExpr for some stupid generator error, so I left that alone.

Jeff, would it be okay to land your patch?

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Hmm, dup'ing a bug apparently passes the mentor field to the bug which has marked as the original bug. Sorry for the noise.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bde3aa7e317

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bde3aa7e317

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8465972 [details] [diff] [review]
js-warn-fixes

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

::: browser/components/nsBrowserGlue.js
@@ +1423,5 @@
>              PlacesUtils.getFolderContents(PlacesUtils.toolbarFolderId).root;
>            let toolbarChildCount = toolbarFolder.childCount;
>            toolbarFolder.containerOpen = false;
>            return toolbarChildCount;
>          }

Nit: missing semicolon

::: services/fxaccounts/FxAccounts.jsm
@@ +65,2 @@
>    this.fxaInternal = fxaInternal;
>  };

Nit: no semicolon

::: services/healthreport/providers.jsm
@@ +880,5 @@
>          return;
>        }
>  
>        let now = new Date();
> +      addons = this.getMeasurement("addons", 2);

Re-using the argument name isn't great. It might be easier here to rename the argument to allAddons or something?

::: toolkit/components/crashes/CrashManager.jsm
@@ +391,5 @@
>    },
>  
>    _addSubmissionAsCrash: function (store, processType, crashType, succeeded,
>                                     id, date) {
> +    id = id + "-" + this.PROCESS_TYPE_SUBMISSION;

Nit: id += "-" + this.PROCESS_TYPE_SUBMISSION;

::: toolkit/modules/Sqlite.jsm
@@ +730,5 @@
>    let identifier = basename + "#" + number;
>  
>    log.info("Opening database: " + path + " (" + identifier + ")");
>    let deferred = Promise.defer();
> +  options = null;

Re-using the argument name here is a little ugly but alright. Better might be to rename it to dbOptions or connOptions.

::: uriloader/exthandler/nsHandlerService.js
@@ +568,5 @@
>        let objpath = this._getValue(aHandlerAppID, NC_OBJPATH);
>        if (!objpath)
>          return null;
>        
> +      let interface_ = this._getValue(aHandlerAppID, NC_INTERFACE);

Looks like this was fixed in the meantime.

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b62a942fd622

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jeff landed his patch a while ago. For some reason it wasn't listed here at all:
http://hg.mozilla.org/mozilla-central/rev/cb3cfc3ef3f3

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b62a942fd622