1005110 - Warn about unreachable code after semicolon-less return statement

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Till Schneidereit [:till]]

The pattern
`return\n
someValueIMeantToReturn;`
is a common stumbling block. We should emit a warning in the devtools console whenever we detect it.

The warning for __proto__ that was added in bug 948227 is probably a good example for how to implement this.

Comment 1

[Scott West]

For this type of bug it seems to require a completed parse tree. Is there a proper place to analyze the completed parse tree?

I suppose it could also be done during parsing but it'd be nice to keep the semantic and syntactic analysis separate.

Comment 2

[Kengo Ide]

I have a presumably working patch for the bug, and I'd like to ask one thing.
Considering this function:

function f() {
  return
  function g() {}
}

In this example, the author may have intended 'g' as a inner function declaration, but it's also possible to interpret it as a wrongly indented return statement. I still haven't got a clear thought about whether or not to show a warning in this case. Should it be warned?

Comment 3

[Tooru Fujisawa [:arai]]

I guess it's not so difficult to disable that warning later, if needed.
So, how about showing warning for that case, and find out how many indented/unintended case happen in practice?

Till, how do you think?

Comment 4

[Tooru Fujisawa [:arai]]

I forgot to add needinfo

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Warn about unreachable code after semicolon-less return.

This patch checks expression-like statement after semicolon-less return statement, and warn on that case.

Here is an example (js shell with -w option):

  [Code]
  function f() {
    return
    1 + 2;
  }
  f();

  [Result]
  test.js:3:2 warning: unreachable code after semicolon-less return statement:
  test.js:3:2 warning:   1 + 2;
  test.js:3:2 warning: ..^

Currently, it outputs warning message when following statement found:
  * expression statement
  * let-expression without block
  * function declaration
  * block statement

It checks only when parsing with FullParseHandler, to get correct position of the beginning of next statement. So the warning message won't be shown if the function is not actually called.


Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cec244e2a5d1


Btw, this patch hits following warning, I'll file a bug for it :D
> SyntaxError: unreachable code after semicolon-less return statement nsContextMenu.js:1529:6

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> Btw, this patch hits following warning, I'll file a bug for it :D
> > SyntaxError: unreachable code after semicolon-less return statement nsContextMenu.js:1529:6
filed as bug 1140725.

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8574301 [details] [diff] [review]
Warn about unreachable code after semicolon-less return.

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

Thank you for doing this! I feel terrible for letting this sit for so long :(

And now, all I can usefully do is forward the review: I don't feel entirely confident in the parser, so I'd rather someone review this who has worked on the parser recently and extensively.

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8574301 [details] [diff] [review]
Warn about unreachable code after semicolon-less return.

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

Oh, god. This is good, but very ugly. It's totally not your fault, but a shame that we can't confine this to Parser::statements because of the switch stuff. r=me f? jorendorff to see if he has any better ideas.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8574301 [details] [diff] [review]
Warn about unreachable code after semicolon-less return.

When claiming to request feedback from someone, it behooves one to set the feedback? flag.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8574301 [details] [diff] [review]
Warn about unreachable code after semicolon-less return.

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

Arai, I agree with Eric: this is a great feature to have, but the patch touches too much code.

There are two different categories of error that this patch can detect:

    1. code that was intended to say `return X;` but unintentionally says `return; X;`
       because the author put a newline between `return` and `X`.

    2. unreachable code generally

Suppose we give up on detecting error 2 and focus entirely on error 1.

In Parser::returnStatement, case TOK_EOF:, peek ahead at the next token, using tokenStream.peekToken(&next, TokenStream::Operand).

If the next token is any token that can be the start of an expression, including TOK_LP, then we should warn, right? Will that catch all the instances of error 1 that we care about?

Clearing the f? request.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Warn about unreachable expression after semicolon-less return.

Thank you for reviewing and feedback :)

I applied the change, now it looks simpler :D
Found similar method nextTokenEndsExpr, so added nextTokenStartsExpr there.
Then, I rephrased the warning message to "unreachable expression after semicolon-less return statement", since this check only expression-like thing, and doesn't show that warning for almost all other statements after semicolon-less return statement.


This patch works almost samely as previous one, except one case that label statement just after semicolon-less return statement, because peekToken returns TOK_NAME for that case.

> return
> a: 1;

But I think it's not so troublesome difference, since it's still unreachable code, and JavaScript doesn't have 'goto' to reach it (am I correct? or is there any trick to reach the label?).
Is it okay to leave it, or should it be better to check it by peeking one more token?


Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46992691feb7

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8585972 [details] [diff] [review]
Warn about unreachable expression after semicolon-less return.

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

Much cleaner. :)

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8585972 [details] [diff] [review]
Warn about unreachable expression after semicolon-less return.

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

::: js/src/frontend/TokenStream.cpp
@@ +338,5 @@
> +    isExprStarting[TOK_TRUE]             = 1;
> +    isExprStarting[TOK_FALSE]            = 1;
> +    isExprStarting[TOK_NULL]             = 1;
> +    isExprStarting[TOK_THIS]             = 1;
> +    isExprStarting[TOK_FUNCTION]         = 1;

Is the consequence that code like this would end up warning?

  function f()
  {
    g();
    return
    function g() { effect(); }
  }

That seems undesirable.

And in any event -- function *does not* start an expression, in this context.  The next thing is either a statement or the end of a StatementList.  So what this records, is that the token is the start of a statement *that is an expression*.  But function in statement context starts a function declaration, not a statement that's an expression that's a function expression.

So remove this line, and both the original problem and the semantic mixup should go away.

::: js/src/jit-test/tests/basic/semicolon-less-return.js
@@ +216,5 @@
> +// TOK_FUNCTION
> +testWarn(`
> +function f() {
> +  return
> +    function() {};

So this test is sort of an example of a case where considering TOK_FUNCTION as meeting the requirements of this extra warning, is a bit misleading.  Without the warning, this is an unconditional SyntaxError.  With the warning, and with werror that you set, it's also a syntax error -- but your test passes only because we consider the warning-issue before parsing enough to realize that |function(| at the start of a statement is not valid syntax, and so we use the warning-issue error and not the not-a-statement error.

Comment 15

[Tooru Fujisawa [:arai]]

Thank you for pointint that out :D
Removed TOK_FUNCTION from the list locally.

I'll land this later today.

Comment 16

[Tooru Fujisawa [:arai]]

Landed :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/404be5f66b2a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/404be5f66b2a

Comment 18

[Tooru Fujisawa [:arai]]

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

Comment 19

[Florian Scholz (Open Web Docs)]

Docs reviewed, Thanks, :arai! :)

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 21

[Tooru Fujisawa [:arai]]

oh, this style is widely used? :O

4) warn only if javascript.options.strict is true ?

Comment 22

[Feross Aboukhadijeh]

> oh, this style is widely used? :O

Yes. Relying on ASI is pervasive in real-world code. See:

- http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding
- http://inimino.org/~inimino/blog/javascript_semicolons
- https://github.com/maxogden/messages/issues/18
- https://github.com/feross/standard

This style of javascript is very common:

script.onreadystatechange = function () {
    if (this.readyState != 'complete' && this.readyState != 'loaded') return
    callback(null, true)
}

Showing tons of warnings in the console for third-party library code isn't going to help anyone. Please seriously consider removing the warning.

Comment 23

[Jeff Walden [:Waldo]]

(In reply to Feross Aboukhadijeh from comment #22)
> This style of javascript is very common:
> 
> script.onreadystatechange = function () {
>     if (this.readyState != 'complete' && this.readyState != 'loaded') return
>     callback(null, true)
> }

That doesn't warn any more.  A warning now occurs if we're parsing:

  * a list of statements,
  * one of which is a return statement,
  * and that return statement is followed by at least one statement other than a
    * throw statement,
    * break statement,
    * var statement, or a
    * function statement

Your example doesn't trigger this, because the |return| isn't a member of a statement list -- it's a direct child of an if-statement.  The return statement and expression statements aren't siblings of each other at all, to trigger anything.  And of course that's how it should be, because the return clearly doesn't always prevent the expression statement from executing.

> Showing tons of warnings in the console for third-party library code isn't
> going to help anyone. Please seriously consider removing the warning.

I'm pretty sure the refinement to statement lists means this doesn't show tons of warnings for third-party library code any more.