Closed Bug 1046964 Opened 7 years ago Closed 7 years ago

Lots of JS warnings printed at startup


(Core :: JavaScript Engine, defect)

Not set





(Reporter: billm, Assigned: billm)




(6 files)

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.
Attached file warning output
This is what I get when starting up and then immediately shutting down.
This gets rid of the "missing return statement" warning, which seems pretty useless.
Assignee: nobody → wmccloskey
Attachment #8465969 - Flags: review?(jwalden+bmo)
Attached patch prologue-fixSplinter Review
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.
Attachment #8465971 - Flags: review?(jwalden+bmo)
Attached patch js-warn-fixesSplinter Review
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.
Attachment #8465972 - Flags: review?(ttaubert)
Comment on attachment 8465969 [details] [diff] [review]

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

Attachment #8465969 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8468126 - Flags: review?(n.nethercote)
Comment on attachment 8465971 [details] [diff] [review]

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

I'd rather see us do the right thing here.  Better patch posted.
Attachment #8465971 - Flags: review?(jwalden+bmo) → review-
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.
> + *
> + * Contributor:
> + *   Jeff Walden <>

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?
Attachment #8468126 - Flags: review?(n.nethercote) → review+
(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.

> 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.
I think you could have removed funHasReturnExpr and funHasReturnVoid.
(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.
Here's a very common example where this doesn't work well. We have a receiveMessage method like this:

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

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.
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.
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)
Flags: needinfo?(wmccloskey)
Attached patch dead-codeSplinter Review
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?
Attachment #8471265 - Flags: review?(jwalden+bmo)
Flags: needinfo?(wmccloskey)
Attachment #8471265 - Flags: review?(jwalden+bmo) → review+
Mentor: manishearth
Duplicate of this bug: 1052515
Hmm, dup'ing a bug apparently passes the mentor field to the bug which has marked as the original bug. Sorry for the noise.
Mentor: manishearth
Comment on attachment 8465972 [details] [diff] [review]

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;
>"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.
Attachment #8465972 - Flags: review?(ttaubert) → review+
Jeff landed his patch a while ago. For some reason it wasn't listed here at all:
Keywords: leave-open
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.