1151931 - Warning for "unreachable expression after semicolon-less return statement" triggers incorrectly (braceless if, ASI)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

derived from bug 1005110.

(In reply to Benjamin Bouvier [:bbouvier] from bug 1005110 comment #20)
> For what it's worth, I'm hitting a lot of these warnings on code that relies
> on ASI *a lot*. Namely, twitter's bootstrap library has return statements
> without semicolons, and they all seem correct (see an instance at [0]).
> Should we
> 
> 1) tweak the heuristics, so that if there's one blank line after the
> carriage return, we don't show the alert?
> 2) ask bootstrap developers / make a pull request to bootstrap so that they
> add semicolons after their empty return statements? (might start a flamewar)
> 3) show the warning only once per compartment / website?
> 
> [0] https://github.com/twbs/bootstrap/blob/master/js/alert.js#L44-L46

Not sure this style (no semicolon at all) is widely used or not though, if it is, 1 won't work.
and I guess 2 is not a good way.
3 might work nicely, but still it hits once for the page.

It may be better to make it strict warning (javascript.options.strict)?
of course some case may be also warned by JSMSG_USELESS_EXPR "useless expression" in the configuration.

Comment 1

[Jeff Walden [:Waldo]]

I don't believe it's widely used.  I think this is mostly the effect of a style quirk of someone I know at Twitter who decided at one time that ASI was one honking good idea and went gonzo with it.  :-)  I'm not sure I've seen such recalcitrance anywhere else.  ;-)

Comment 2

[Jesse Ruderman]

The warning is incorrect in this case. It says "SyntaxError: unreachable expression after semicolon-less return statement", but the expression isn't actually unreachable, because the return is conditional.

> if (0) return
> print(3);

Warns, but 3 is printed anyway.

Comment 3

[Jesse Ruderman]

Warning about "braceless if" or "reliance on ASI" is best left for jshint. If we want to warn about the combination we should have a different message than this one. But we probably just shouldn't warn in this case.

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for the example, yeah, this message is not correct for the case.
There should be better message which describes what we actually want to warn:
the case that author meant to write 'if (0) return print(3);' but unintentionally put newline after 'return', maybe for maximum line width or some code styling reason.
so, if author actually relies on ASI, it's not what we want to warn, I guess.

Comment 5

[Tooru Fujisawa [:arai]]

how about comparing column position of 'return' and the next token?
so, if author actually want to return the value of expression, they will write something like

  return
    f();

so, if the column position of the next token is larger than (or equals to?) 'return', it should be suspicious. and we won't hit the case for 'if' (comment #0 and comment #2), in most coding style.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Do not warn about semicolon-less return if next token starts before the column index of return.

Applied the change in comment #5.
With this change, the case of bug 1140725 still can be warned, and the bootstrap's case won't be warned.

  warned:
    if (...)
      return
        f();

  not warned:
    if (...)
      return
    f();

also, removed "unreachable" from the warning message, since we're not actually checking the reachability now.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Warn about unreachable expression after semicolon-less return statement only in block-like statement.

Updated the patch to use inner-most statement information instead of column.
now it warns only in STMT_BLOCK, STMT_SWITCH, STMT_CATCH, STMT_TRY, STMT_FINALLY, and function top-level (it skips STMT_LABELs)
so it's now unconditionally unreachable.

Comment 8

[Jeff Walden [:Waldo]]

It's unclear to me why it wouldn't be better to check for this an entirely different place: inside Parser::statements.  Before a return statement is encountered, just keep doing as we do now.  Once a return statement is seen, keep a stack boolean recording that.  Then, every iteration of the parse-a-statement loop, if that flag is set, check whether the node is an unreachable statement.  If it's a hoistable declaration (function statement, var statement), do nothing.  Otherwise, emit a warning (once, for the entire Parser::statements call) that subsequent statements are unreachable.  This avoids a bunch of complexity of checking this effectively at the tokenization stage.

This would raise the small concern of people wanting to temporarily (or even permanently) disable a function's code by inserting an unconditional early return.  That's the only demerit I see.  And maybe there's a way to work around that, *or* we can just decide that warning for "temporary hacks" is not unreasonable.

Comment 9

[Tooru Fujisawa [:arai]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> It's unclear to me why it wouldn't be better to check for this an entirely
> different place: inside Parser::statements.  Before a return statement is
> encountered, just keep doing as we do now.  Once a return statement is seen,
> keep a stack boolean recording that.  Then, every iteration of the
> parse-a-statement loop, if that flag is set, check whether the node is an
> unreachable statement.  If it's a hoistable declaration (function statement,
> var statement), do nothing.  Otherwise, emit a warning (once, for the entire
> Parser::statements call) that subsequent statements are unreachable.  This
> avoids a bunch of complexity of checking this effectively at the
> tokenization stage.

It means that warning about unreachable code generally (case 2 in bug 1005110 comment #11), right?
Does it include checking semicolon for return statement? Or more general warning?

Comment 10

[Eric Faust [:efaust]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> This would raise the small concern of people wanting to temporarily (or even
> permanently) disable a function's code by inserting an unconditional early
> return.  That's the only demerit I see.  And maybe there's a way to work
> around that, *or* we can just decide that warning for "temporary hacks" is
> not unreasonable.

As long as we'll still run the script, I don't see a problem with warning. It's true that the code is unreachable. I don't think we need to aspire to the amount of telepathy required to know if they meant it to be or not.

Comment 11

[Eric Faust [:efaust]]

Blech. I hate to keep doing this, but I think Waldo has a point. If we move this logic to statements(), we avoid this case altogether, because statements() will never be called in these small scoped cases. Can we do that instead?

Comment 12

[Tooru Fujisawa [:arai]]

With WIP patch for more generic warning for unreachable code after return statement, it hits second return in following case, it seems to be useful (I hope it's not some hack for code validator) :)

https://dxr.mozilla.org/mozilla-central/source/js/src/tests/shell.js#413
> function BigO(data)
> {
> ...
>   if (2 == origLength - order)
>   {
>     order = Number.POSITIVE_INFINITY;
>   }
>   return order;
>   function LinearRegression(data)
>   {
> ...
>   }
>   function dataDeriv(data)
>   {
> ...
>   }
>   return 0;
> }

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> With WIP patch for more generic warning for unreachable code after return
> statement, it hits second return in following case, it seems to be useful (I
> hope it's not some hack for code validator) :)

I'd bet that's because of our former, misguided warning about a function containing return statements but not ending in one, that we removed because our analysis was totally broken in many cases where the end of the function wasn't actually reachable, such as your example.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Warn about unreachable code after return statement.

Moved logic to statements and switchStatement, and now it doesn't check the existence of semicolon (please tell me if I'm misunderstanding).
If parser finds statement after return statement (skipping all hoistable declarations between them), and it's not a break statement (it seems to be common case that break is placed after return in switch statement), it shows warning.

This patch hits bug 1154228 and bug 1154234, and some more cases seem to exist:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f84634a527da (tested by throwing error instead of warning)

Also, I opened several websites with this patch applied, and hit the warning in https://s.ytimg.com/yts/jsbin/html5player-en_US-vfleI-biQ/html5player.js used by https://blog.mozilla.org/ and https://www.youtube.com/ , there is `throw` after `return`.

> function PQ(a,b,c){var d="key_"+a+":"+b,e=NQ[d];if(void 0===e||0>e)NQ[d]=0,
> OQ[d]=c;else if(0==e)throw Error('Encountered two active delegates with the
> same priority ("'+a+":"+b+'").');}function QQ(a,b){var c=OQ["key_"+a+":"+b];
> c||""==b||(c=OQ["key_"+a+":"]);return c?c:RQ;throw Error('Found no active impl
> for delegate call to "'+a+":"+b+'" (and not allowemptydefault="true").');}
> function RQ(){return""}

Current no other warning found on the web, but I guess there are more.

Comment 15

[Tooru Fujisawa [:arai]]

Maybe similar to html5player's case.

`throw` is placed at the last of the function for the case all of above #ifdef fails and no `return` is extracted there.
https://hg.mozilla.org/mozilla-central/file/53ceefb0e1c8/toolkit/webapps/WebappOSUtils.jsm#l173
>   getInstallPath: function(aApp) {
> #ifdef MOZ_B2G
> ...
> #elifdef MOZ_FENNEC
> ...
> #elifdef MOZ_PHOENIX
> #ifdef XP_WIN
> ...
> #elifdef XP_MACOSX
> ...
> #elifdef XP_UNIX
> ...
> #endif
> #elifdef MOZ_WEBAPP_RUNTIME
> #ifdef XP_WIN
> ...
> #elifdef XP_MACOSX
> ...
> #elifdef XP_UNIX
> ...
> #endif
> #endif
>     // Anything unsupported, like Metro
>     throw new Error("Unsupported apps platform");
>   },

might be better to ignore `throw` as well as `break`?

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8592291 [details] [diff] [review]
Warn about unreachable code after return statement.

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

::: js/src/frontend/Parser.cpp
@@ +2820,5 @@
>      pc->blockNode = pn;
>  
>      bool canHaveDirectives = pc->atBodyLevel();
> +    bool afterReturn = false;
> +    TokenPos pos(0, 0);

Mild preference for this being a uint32_t statementBegin, then have the peekTokenPos call read into a TokenPos scoped to exactly that |if (afterReturn) {}| statement, copying .begin out of it just after the reading.  That makes it a little clearer that we don't care about |pos.end| at all.

@@ +2842,5 @@
>              return null();
>          }
> +        if (afterReturn) {
> +            if (!handler.isHoistableDeclaration(next)) {
> +                if (!handler.isBreak(next)) {

If you want to throw (heh) in throw statements too, I'm not going to be too unhappy about it.  (Although note that some implicit pseudo-requirement that new warnings not fire on web code seems too much a hurdle -- you want them to not fire *often*, some firing is and should be okay.  Not convinced one YouTube script meets that bar at all.)  But at that point you probably should add a better-named handler method to detect it.  Maybe restructure the whole thing to

  if (afterReturn) {
    if (!handler.isStatementPermittedAfterReturnStatement(next)) {
      if (!reportWithOffset(ParseWarning, false, statementBegin, JSMSG_STMT_AFTER_RETURN))
        return false;
      warnedAboutStatementsAfterReturn = true;
    }
  } else if (handler.isReturnStatement(next)) {
    afterReturn = true;
  }

Having to permit |break| even when not inside a switch statement is a little sadmaking.  I suppose we could detect whether we're inside a switch, but the pain probably isn't worth it.

@@ +2847,5 @@
> +                    if (!reportWithOffset(ParseWarning, false, pos.begin,
> +                                          JSMSG_STMT_AFTER_RETURN))
> +                        return null();
> +                }
> +                afterReturn = false;

This seems...neurotic.  Wouldn't the result be that we'd get multiple warnings for this?

  function f()
  {
    return 0;
    1; // not a break, triggers warning -- then sets afterReturn = false
    return 2; // sets afterReturn back to true
    3; // not a break, triggers warning *again*
  }

Seems like we should have a bool recording whether a warning has been reported, then only report if that boolean is false.

@@ +5214,5 @@
>          if (!body)
>              return null();
>  
> +        bool afterReturn = false;
> +        TokenPos pos(0, 0);

All the same comments as on the other code.

::: js/src/frontend/SyntaxParseHandler.h
@@ +240,5 @@
>      bool addCatchBlock(Node catchList, Node letBlock,
>                         Node catchName, Node catchGuard, Node catchBody) { return true; }
>  
>      void setLastFunctionArgumentDefault(Node funcpn, Node pn) {}
> +    Node newFunctionDefinition() { return NodeHoistableDeclaration; }

This is used for function statements and function expressions both, unfortunately, so this is dodgy.  But I can't think of a way it's consequentially dodgy.  :-\

@@ +287,1 @@
>      }

Rather than inserting these here, I'd prefer seeing

    Node newDeclarationList(ParseNodeKind kind) {
        return kind == PNK_VAR ? NodeHoistableDeclaration : NodeGeneric;
    }

and similar so that we're not burdening a general method with behavior for exactly two call sites.  (And add assertions to newList enforcing that it's not called with PNK_VAR, for a double-check.)

::: js/src/jit-test/tests/basic/statement-after-return.js
@@ +8,5 @@
>    try {
>      eval(code);
>    } catch (e) {
>      caught = true;
> +    assertEq(e.message, "unreachable code after return statement", code);

Frankly I'd rather see assertEq(e.constructor, SyntaxError) or whatever instead of an exact error message check.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Avoid warning about unreachable code after return statement in some asm.js tests.

Thank you for the feedback!
addressed the comments :)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> ::: js/src/frontend/SyntaxParseHandler.h
> @@ +240,5 @@
> >      bool addCatchBlock(Node catchList, Node letBlock,
> >                         Node catchName, Node catchGuard, Node catchBody) { return true; }
> >  
> >      void setLastFunctionArgumentDefault(Node funcpn, Node pn) {}
> > +    Node newFunctionDefinition() { return NodeHoistableDeclaration; }
> 
> This is used for function statements and function expressions both,
> unfortunately, so this is dodgy.  But I can't think of a way it's
> consequentially dodgy.  :-\

In the patch in bug 1149333, newly added `newFunction` method receives JSOp parameter, so we can detect whether it's a statement (JSOP_DEFFUN) or not (JSOP_LAMBDA_ARROW/JSOP_LAMBDA), and return appropriate node from SyntaxParseHandler.
So, if bug 1149333 lands prior to this, there should be no problem. if this bug lands first, we could pass temporary parameter FunctionSyntaxKind to newFunctionDefinition and check it (or just leave it as is and fix it in bug 1149333 ?).


In some asm.js tests, warning for return statement is fired before warning for asm.js type failure, so I modified them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca75abafab25 (asm.js tests failed)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a8e743957b0 (fixed asm.js, maybe infra issue on B)

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Warn about unreachable code after return statement.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8594057 [details] [diff] [review]
Part 2: Warn about unreachable code after return statement.

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

Will do the other after I head into the office now.

::: js/src/frontend/Parser.cpp
@@ +2848,5 @@
> +            if (afterReturn) {
> +                if (!handler.isStatementPermittedAfterReturnStatement(next)) {
> +                    if (!reportWithOffset(ParseWarning, false, statementBegin,
> +                                          JSMSG_STMT_AFTER_RETURN))
> +                        return null();

Braces around this, if the condition doesn't fit on one line.

Comment 20

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

Found 4 examples about temporary disabling hack with return.
Might be better to fix Prefetcher.jsm's case before fixing this bug, since it will be visible in browser console.

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/Prefetcher.jsm#104
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/Prefetcher.jsm#115
https://dxr.mozilla.org/mozilla-central/source/intl/locale/tests/unit/test_bug374040.js#36
https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_plaintext_sniff.js#181

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8594055 [details] [diff] [review]
Part 1: Avoid warning about unreachable code after return statement in some asm.js tests.

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

The argument could be made that the existing tests are different from what you've changed them to -- in that with your changes, only actually-reachable returns have consistent type.  But as long as asm.js forbids constant-folding to affect linking and all, good enough, I guess.

Comment 22

[Tooru Fujisawa [:arai]]

Thanks again :)

Ran try with throwing error instead of warning again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b67a521ebeb

and there seems to be only one case which is not test code
https://hg.mozilla.org/mozilla-central/file/a32e3b93c8d8/browser/extensions/pdfjs/content/web/viewer.js#l6815
>   PDFViewerApplication.setTitleUsingUrl(file);
>   PDFViewerApplication.initPassiveLoading();
>   return;
> 
> 
>   if (file) {
>     PDFViewerApplication.open(file, 0);
>   }
> }

after this gets fixed, I guess the patch is ready to land.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8bdbc4b4b58
https://hg.mozilla.org/integration/mozilla-inbound/rev/02520ac48ee3

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8bdbc4b4b58
https://hg.mozilla.org/mozilla-central/rev/02520ac48ee3

Comment 25

[Tooru Fujisawa [:arai]]

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/return

Comment 26

[Feross Aboukhadijeh]

It looks like this patch relaxed the number of places where this warning shows up. But, I still wanted to share my thoughts on the "I don't believe ASI is widely used" sentiment.

I don't think Firefox should be in the business of checking code style. There are numerous excellent tools for that already (e.g. eslint, jshint, jscs, to name a few).

In old nightlies this warning was incorrectly triggering for any code that relied on ASI, which is quite common, not just in Bootstrap. NPM Inc. (the company behind npm) relies on ASI in all their modules. So do many prolific npm authors (mafintosh, maxogden, izs, substack...) This code ends up in the browser via browserify. What about minified code where the minifier is relying on ASI to save bytes? What about third-party code?

Comment 27

[Jeff Walden [:Waldo]]

(In reply to Feross Aboukhadijeh from comment #26)
> I still wanted to share my thoughts on the "I don't believe
> ASI is widely used" sentiment.
> 
> I don't think Firefox should be in the business of checking code style.
> There are numerous excellent tools for that already (e.g. eslint, jshint,
> jscs, to name a few).

It's a fair argument, and it'd simplify our lives a fair bit.  I've certainly seriously considered axing the entire thing (but keep in mind it's not my call to make).  But we've made the opposite choice for close to twenty years now.  And last time the question arose, lots of people working on Mozilla code specifically (in Firefox, in extensions, etc.) clearly said they wanted the warnings.

For better and worse, I don't think this is going to change.

> What about minified code where the minifier is
> relying on ASI to save bytes? What about third-party code?

As regards minifiers particularly, two thoughts.

First, any statement that occurs after a return statement in a statement list, of the sort we warn about now, is a waste of bytes.  Minifiers *should* remove such code in the minification process.  They should *want* this warning as pointing out a missed opportunity to remove bytes.  If the new warning fires for any of them, you should report bugs to them.

Second, ASI happens only at line breaks. (Okay, at ends of blocks/functions/scripts too, but we're not warning in those contexts.)  ASI "inserts" a semicolon that could just as easily have been used *instead of* the linebreak, at no increase in characters.  This ASI can't be used to save bytes.

But in general: the justification for warning about non-error cases is that warnings are opt-in.  If you don't turn on the warnings, you don't see a thing.  Just as if you don't use a linter, you don't see its warnings.

Comment 28

[Günter Zöchbauer]

I get still a lot of these warnings in 40.0.3.

Dart compiled to JS doesn't add semicolons at the end of lines, which makes it quite common for me.

One example 
http://bwu-dart.github.io/bwu_datagrid/example/composite_editor_item_details.html
(here the whole list of my examples of one project https://github.com/bwu-dart/bwu_datagrid/wiki/Examples)

Comment 29

[Marc-Aurèle DARCHE]

For the record (for people landing here after having read the "unreachable code after return statement" warning), some code (like the popular Bluebird Promise library) add some unreachable statements to alter JavaScript engines optimization cf. http://stackoverflow.com/questions/24987896/how-does-bluebirds-util-tofastproperties-function-make-an-objects-properties

And for example when a project that is using the Bluebird code through browserify (with no compression) for client-side JavaScript, some of this unreachable code arrives into Firefox and triggers the warning.

A solution to not have this warning anymore is simply to compress (for example through uglify) the browserified code, this will remove the unreachable statements.