Closed Bug 1177941 Opened 9 years ago Closed 9 years ago

"Syntax error unreachable code after return statement" is hard to debug in generated code

Categories

(Core :: JavaScript Engine, defect)

40 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 + fixed
firefox41 + fixed
firefox42 --- fixed

People

(Reporter: jason, Assigned: arai)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150626004006

Steps to reproduce:

I'm using the numeric.js library (http://www.numericjs.com/) to do linear algebra in javascript. It does a bunch of upfront code generation to create efficient broadcasting versions of binary operators so that you can do things like add two matrices or two vectors.

Newer versions of firefox log a lot of warnings to the console upon loading this library:

```
SyntaxError: unreachable code after return statement numeric-1.2.6.js:3:54
SyntaxError: unreachable code after return statement numeric-1.2.6.js:5:26
SyntaxError: unreachable code after return statement numeric-1.2.6.js:3:51
SyntaxError: unreachable code after return statement numeric-1.2.6.js:5:31
SyntaxError: unreachable code after return statement numeric-1.2.6.js:3:69
SyntaxError: unreachable code after return statement numeric-1.2.6.js:3:52
SyntaxError: unreachable code after return statement numeric-1.2.6.js:3:86
SyntaxError: unreachable code after return statement numeric-1.2.6.js:5:39
SyntaxError: unreachable code after return statement numeric-1.2.6.js:4:47
SyntaxError: unreachable code after return statement numeric-1.2.6.js:6:39
SyntaxError: unreachable code after return statement numeric-1.2.6.js:4:32
```

The issue is difficult to debug, because the line and column numbers appear to refer to generated code rather than the original source.

It would be much easier to debug this issue if there was a way to turn these warnings into exceptions so that I could see a call stack, and hopefully examine the actual generated code that is offending the javascript engine.

For convenience, here's a jsbin that does nothing but load up numeric.js.

http://output.jsbin.com/qubolo

Check the firefox console to see the logged errors on that page.

I have also opened this as an issue in numeric.js's public repo

https://github.com/sloisel/numeric/issues/65

although it appears that the library may no longer be actively maintained


Actual results:

Lots of difficult to debug warnings in the console.


Expected results:

Either the warnings should not be shown at all, or they should be easy to turn off, or there should be some way to set a breakpoint where they are generated.
Honestly, I think this whole feature should be reconsidered. It could be cool for the firefox dev tools to have a built in linter, but I think that should be a first class feature with its own separate UI. Bolting on 1% of jshint as noisy console warnings seems like a dead end.
[Tracking Requested - why for this release]: Warnings that perhaps should not be there.

Looks like this warning was added in bug 1151931.

Thank you for the link to the jsbin.  Looks like in this case the relevant code is being passed to a "new Function" call, and looks something like this (as a C string):

"var x = this;\n\nif(x.y) {  return new numeric.T(x.x,numeric.neg(x.y));;\n}\nreturn new numeric.T(x.x);;\n"

Note that the thing after return is an empty statement.  We should probably just not warn on that no matter what....

I poked at a few more occurences of this warning on this page and they all seem to look like that so far.
Blocks: 1151931
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(arai.unmht)
Keywords: regression
We could allow empty statements after return statement.
Currently following declarations and statements are allowed after return statement:
https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/FullParseHandler.h#703
  * function declaration
  * var declaration
  * break statement
  * throw statement
Former 2 are hoisted, so the place doesn't matter.  Latter 2 are actually unreachable, but "break" is placed inside switch statement in many cases, like:
  switch (cond) {
    case FOO:
      return 42;
      break;
  }
and "throw" sometimes placed as a sentinel (see bug 1151931 comment #14 and #15).
Allowing empty statement won't hurt more than break/throw cases.


Or perhaps we could hide the warning behind javascript.options.strict pref, just like bare-assignment in if-condition (if (a=1) ...), so the warning won't be shown for clean profile.


About debugging, the underlying issue is that there's no way to see the eval/new Function code for warning.  So, fixing the debugging issue only for this warning won't be a nice solution.
in js shell, there's werror option for testing. not sure it can be exposed to browser tho...
https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp#699


for now, I'll prepare a patch which allows empty statement.
Flags: needinfo?(arai.unmht)
Yeah, empty statement is the simple thing.  Catch me in the next hour or so and I'll stamp quickly.
Added empty statement to isStatementPermittedAfterReturnStatement.
Then, the test for empty statement was wrong, it didn't generate PNK_SEMI node, fixed it also.

the test is passed locally.
try is running now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9129720fa931
Assignee: nobody → arai.unmht
Attachment #8627525 - Flags: review?(jwalden+bmo)
Comment on attachment 8627525 [details] [diff] [review]
Do not show warning about unreachable empty statement after return statement.

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

::: js/src/frontend/SyntaxParseHandler.h
@@ +44,5 @@
>          NodeReturn,
>          NodeHoistableDeclaration,
>          NodeBreak,
>          NodeThrow,
> +        NodeEmpty,

NodeEmptyStatement, please.  "empty" alone isn't clear to me.

::: js/src/jit-test/tests/basic/statement-after-return.js
@@ +255,5 @@
>  
>  // empty statement
>  testPass(`
>  function f() {
> +  return;;

Preference for

  return;
  ; // empty statement

to be clearer than a (conceivable, if unlikely in context) possible typo.
Attachment #8627525 - Flags: review?(jwalden+bmo) → review+
Note that there's a separate issue here, which I think the initial reporter was more concerned about: the location for the warning is utterly wrong. Attached is a reduced test case for that, which logs "test.html:3:4" as the location for me when warning about the unreachable code.

The location seems to be relative to the function created by `new Function`, with an extra line for the function declaration. Ideally, it'd be relative to the generated code and clicking on the location would show that in the debugger. (Though the generated function itself doesn't show up in the debugger at all, for some reason.)
Thank you Waldo for quick review, and thank you till for the investigation :D

for me, the testcase shows warning at attachment.cgi:3:2, so it's the line/column number based on the script for the function.

var f = new Function(` <- line 1
  return 1;            <- line 2
  alert('2');          <- line 3
`);

So, line/column is correct, but the filename is somehow wrong.

It seems that currently we're printing error/warning with "filename > line X Function:LINE:COLUMN" format for runtime error, and "filename:LINE:COLUMN" format for syntax error/warning, am I correct?
Then, former style cannot be handled correctly in console.  clicking that url opens "File not found" page.  Also, latter style doesn't point the right position.
So I guess the required work to fix the view-source/debugger issue is following:
  1. make it possible to open the "filename > line X Function" style URL, or add dedicated data format to handle it more precisely?
  2. show syntax error/warning in eval/Function same way as runtime error

I guess bug 1109612 is related to 1.
See Also: → 1109612
(In reply to Tooru Fujisawa [:arai] from comment #8)
> Thank you Waldo for quick review, and thank you till for the investigation :D
> 
> for me, the testcase shows warning at attachment.cgi:3:2, so it's the
> line/column number based on the script for the function.

Right, the file is obviously different in the attachment. Good point on the line number, though: line 1 is of course right there in the template string.

> It seems that currently we're printing error/warning with "filename > line X
> Function:LINE:COLUMN" format for runtime error, and "filename:LINE:COLUMN"
> format for syntax error/warning, am I correct?

Correct.

> Then, former style cannot be handled correctly in console.  clicking that
> url opens "File not found" page.  Also, latter style doesn't point the right
> position.
> So I guess the required work to fix the view-source/debugger issue is
> following:
>   1. make it possible to open the "filename > line X Function" style URL, or
> add dedicated data format to handle it more precisely?

Right. The thing is, that usually works. We've had debugging of eval'd code for the better part of a year now, and it works with locations printed in error logs, too. However, here the eval'd code doesn't show up in the debugger at all. Filed bug 1178687 about that.

>   2. show syntax error/warning in eval/Function same way as runtime error

Agreed, that's what should happen.
Depends on: 1178687
https://hg.mozilla.org/mozilla-central/rev/52f3d7379370
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Tracking for 40,41- does this mean that we will want to uplift a fix to 40 eventually?
Flags: needinfo?(bzbarsky)
Yes, I think this should be uplifted to both 41 and 40, assuming it's safe.
Flags: needinfo?(bzbarsky)
Same patch is applicable to mozilla-aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1151931
[User impact if declined]: Almost useless warning is shown in console
[Describe test coverage new/current, TreeHerder]: testcase in jittest is updated in this patch, and passed on mozilla-central, which has exactly same change
[Risks and why]: low, it only reduces the target case of warning
[String/UUID change made/needed]: none
Attachment #8630298 - Flags: review+
Attachment #8630298 - Flags: approval-mozilla-aurora?
Almost same patch is applicable to mozilla-beta (difference only in adjacent unrelated code)

Approval Request Comment
[Feature/regressing bug #]: bug 1151931
[User impact if declined]: Almost useless warning is shown in console
[Describe test coverage new/current, TreeHerder]: testcase in jittest is updated in this patch, and passed on mozilla-central, which has almost same change
[Risks and why]: low, it only reduces the target case of warning
[String/UUID change made/needed]: none
Attachment #8630306 - Flags: review+
Attachment #8630306 - Flags: approval-mozilla-beta?
Comment on attachment 8630298 [details] [diff] [review]
(mozilla-aurora) Do not show warning about unreachable empty statement after return statement. r=Waldo

This fix has been on m-c for a week. Let's move it up to Aurora and Beta so that we don't ship this regression.
Attachment #8630298 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8630306 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.