Lots of JS warnings printed at startup

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8465723 [details]
warning output

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

Comment 2

3 years ago
Created attachment 8465969 [details] [diff] [review]
kill-retval-warning

This gets rid of the "missing return statement" warning, which seems pretty useless.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8465969 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

3 years ago
Created attachment 8465971 [details] [diff] [review]
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.
Attachment #8465971 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

3 years ago
Created attachment 8465972 [details] [diff] [review]
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.
Attachment #8465972 - Flags: review?(ttaubert)
Comment on attachment 8465969 [details] [diff] [review]
kill-retval-warning

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

\o/
Attachment #8465969 - Flags: review?(jwalden+bmo) → review+
Created 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

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]
prologue-fix

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.
> + * http://creativecommons.org/licenses/publicdomain/
> + * Contributor:
> + *   Jeff Walden <jwalden+code@mit.edu>

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

Comment 9

3 years ago
The missing return value patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a470d0cbe3fa
Keywords: leave-open
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
(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.
(Assignee)

Comment 12

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/cb3cfc3ef3f3
Flags: in-testsuite+
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.
(Assignee)

Comment 16

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/d400055c9999
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)
(Assignee)

Comment 20

3 years ago
Created attachment 8471265 [details] [diff] [review]
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?
Attachment #8471265 - Flags: review?(jwalden+bmo)
Flags: needinfo?(wmccloskey)
Attachment #8471265 - Flags: review?(jwalden+bmo) → review+
Blocks: 1053061
Mentor: manishearth@gmail.com
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@gmail.com
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bde3aa7e317
https://hg.mozilla.org/mozilla-central/rev/6bde3aa7e317
Depends on: 1055190
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.
Attachment #8465972 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 26

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b62a942fd622
(Assignee)

Comment 27

3 years ago
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
Keywords: leave-open

Updated

3 years ago
Blocks: 1055906
https://hg.mozilla.org/mozilla-central/rev/b62a942fd622
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 925150
You need to log in before you can comment on or make changes to this bug.