The default bug view has changed. See this FAQ.

Warn about unreachable code after semicolon-less return statement

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: till, Assigned: arai, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

Trunk
mozilla40
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [DocArea=JS][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

3 years ago
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.
Mentor: till@tillschneidereit.net
Whiteboard: [mentor=till][lang=c++] → [lang=c++]
Assignee: general → nobody

Comment 2

3 years ago
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?
Blocks: 622261
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?
I forgot to add needinfo
Flags: needinfo?(till)
Flags: needinfo?(till)
Duplicate of this bug: 1140596
Created attachment 8574301 [details] [diff] [review]
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
Attachment #8574301 - Flags: review?(till)
(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.
(Reporter)

Comment 8

2 years ago
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.
Attachment #8574301 - Flags: review?(till) → review?(efaustbmo)

Comment 9

2 years ago
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.
Attachment #8574301 - Flags: review?(efaustbmo) → review+
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.
Attachment #8574301 - Flags: feedback?(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.
Attachment #8574301 - Flags: feedback?(jorendorff)
Created attachment 8585972 [details] [diff] [review]
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
Attachment #8574301 - Attachment is obsolete: true
Attachment #8585972 - Flags: review?(efaustbmo)
Comment on attachment 8585972 [details] [diff] [review]
Warn about unreachable expression after semicolon-less return.

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

Much cleaner. :)
Attachment #8585972 - Flags: review?(efaustbmo) → review+
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.
Thank you for pointint that out :D
Removed TOK_FUNCTION from the list locally.

I'll land this later today.
Landed :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/404be5f66b2a
https://hg.mozilla.org/mozilla-central/rev/404be5f66b2a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

2 years ago
Keywords: dev-doc-needed, site-compat
Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/return
Whiteboard: [lang=c++] → [DocArea=JS][lang=c++]
Docs reviewed, Thanks, :arai! :)
Keywords: dev-doc-needed → dev-doc-complete
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
oh, this style is widely used? :O

4) warn only if javascript.options.strict is true ?
Depends on: 1151931
Assignee: nobody → arai.unmht
Depends on: 1153656

Comment 22

2 years ago
> 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.
(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.
You need to log in before you can comment on or make changes to this bug.