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)
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: jason, Assigned: arai)
References
(Depends on 1 open bug)
Details
(Keywords: regression)
Attachments
(4 files)
4.39 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
131 bytes,
text/html
|
Details | |
4.45 KB,
patch
|
arai
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(mozilla-beta) Do not show warning about unreachable empty statement after return statement. r=Waldo
4.51 KB,
patch
|
arai
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 2•9 years ago
|
||
[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
tracking-firefox40:
--- → ?
Ever confirmed: true
Flags: needinfo?(arai.unmht)
Keywords: regression
Assignee | ||
Comment 3•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
Yeah, empty statement is the simple thing. Catch me in the next hour or so and I'll stamp quickly.
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Comment 7•9 years ago
|
||
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.)
Assignee | ||
Comment 8•9 years ago
|
||
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
Comment 9•9 years ago
|
||
(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.
https://hg.mozilla.org/mozilla-central/rev/52f3d7379370
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 12•9 years ago
|
||
Tracking for 40,41- does this mean that we will want to uplift a fix to 40 eventually?
Comment 13•9 years ago
|
||
Yes, I think this should be uplifted to both 41 and 40, assuming it's safe.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 14•9 years ago
|
||
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?
Assignee | ||
Comment 15•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox39:
--- → unaffected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
Comment 16•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8630306 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c52ee49106a8
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•