941591 - No line numbers for Syntax Errors

Status: VERIFIED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Frederik Braun [:freddy]]

I am testing and debugging JS in Scratchpad, but when I do stupid things like syntax errors I don't get a line number. In this case it was something like |"foo":+ bar|, where the colon should have went into the string.

I took me a while to figure that out. I wouldn't have wasted my that much time if I was any good in JS. But it's also a bug in Scratchpad :P


Bugzilla says Bug 864680 might be related, but I'm not sure.
Screenshot: http://i.imgur.com/Q9XIKgB.png
Experienced both in Aurora (recent) and Nightly (a day or two behind)

Comment 1

[Alan Thomas]

This is a regression: such line numbers used to be displayed.

Comment 3

[Brandon Benvie [:benvie]]

This is happening because of how Scratchpad.writeAsErrorComment interprets the error's "stack" property [1]. Previously, the check was something like:

> if (error.stack) {
>   stack = error.stack;
> } else {
>   // assemble stack from other info like error.lineNumber
> }

The check was changed to `if (typeof error.stack == "string") {`. The problem with that is SyntaxErrors have the empty string as their stack property, so it uses that instead of building a more useful one.

Fixing this just requires changing the check back, and we should add a test for SyntaxError formatting to [2].

[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#847
[2] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/test/browser_scratchpad_display_outputs_errors.js

Comment 4

[Alan Thomas]

With that change I get things like:

/*
Exception: syntax error
@10
*/

It should say something more like "@Scratchpad/1:10".

Comment 5

[Rahid Hasan [:recursive]]

Hi I would like to work on this bug. I'm quite new. Can you please guide me?

Comment 6

[Brandon Benvie [:benvie]]

(In reply to Rahid Hasan [:recursive] from comment #5)
> Hi I would like to work on this bug. I'm quite new. Can you please guide me?

Comment 7

[Brandon Benvie [:benvie]]

(In reply to Alan Thomas from comment #4)
> With that change I get things like:
> 
> /*
> Exception: syntax error
> @10
> */
> 
> It should say something more like "@Scratchpad/1:10".

Oh wow you're right. There's another bug... `typeof error.fileName == "number"`. Instead of string.

Comment 8

[Brandon Benvie [:benvie]]

Just checking to see if you still want to do this. I'll unassign it if not.

Comment 9

[Rahid Hasan [:recursive]]

Yes. I am working on this.

Comment 10

[Rahid Hasan [:recursive]]

Attachment: No line numbers for Syntax Errors; r=benvie

Comment 11

[Brandon Benvie [:benvie]]

Comment on attachment 8364439 [details] [diff] [review]
No line numbers for Syntax Errors; r=benvie

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

We should add a test to confirm this behavior in the future. You should be able to add a syntax error test to [1].

[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/test/browser_scratchpad_display_outputs_errors.js

::: browser/devtools/scratchpad/scratchpad.js
@@ +844,5 @@
>          }
>  
>          // Assemble the best possible stack we can given the properties we have.
>          let stack;
> +        if (error.stack) {

We still also need to check for typeof == "string" here.

Comment 12

[Alan Thomas]

Attachment: Example tests

This is an example of some tests that could be useful.

Comment 13

[Victor Porof [:vporof][:vp]]

Benvie, I think attachment 8364851 [details] [diff] [review] needs to be reviewed maybe?

Comment 14

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #13)
> Benvie, I think attachment 8364851 [details] [diff] [review] needs to be reviewed maybe?

Ah yeah I was looking for recursive to come back, but with the existing reviewed patch and the new one for the tests from Alan, this should be enough.

Comment 15

[Rahid Hasan [:recursive]]

Benvie, I think as I'm quite inexperienced. Better to move with Alan's patch as it seems good :)

Comment 16

[Brandon Benvie [:benvie]]

No worries, I'll take care of it. Thanks!

Comment 17

[Brandon Benvie [:benvie]]

Attachment: no_line_number_error.patch

Incorporate the tests from Alan. https://tbpl.mozilla.org/?tree=Try&rev=3e814af148a3

Comment 18

[Brandon Benvie [:benvie]]

Attachment: no_line_number_error.patch

Adds the type check on error.stack back in.

Comment 19

[Brandon Benvie [:benvie]]

 https://hg.mozilla.org/integration/fx-team/rev/f1156ec42c25

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1156ec42c25

Comment 21

[Adrian Aichner [:anaran]]

/*
 * This is a JavaScript Scratchpad...
 */

4 = 3
/*
Exception: invalid assignment left-hand side
@Scratchpad/1:5
*/
----
Just to confirm this fixed for me in
navigator.userAgent
/*
Mozilla/5.0 (Windows NT 5.1; rv:30.0) Gecko/20100101 Firefox/30.0
*/
navigator.buildID
/*
20140206030201
*/

Comment 22

[Kostas]

Currently Scratchpad displays either 3-number or 2-number line numbers for syntax errors: 
eg.
> @Scratchpad/3:1:1
> @Scratchpad/3:1

As I've seen, the 1st number (3) is always the times Scratchpad has been launched in the current session, 
i.e 3 means that during the current Firefox session, Scratchpad has been launched 3 times, i.e the current is the 3rd
(the rest numbers are the line number and/or column number where the error was found).
-I don't mean Scratchpad instances: just closing+reopening Scratchpad 3 times-.

Is the first number needed?
It's very confusing.

Comment 23

[Adrian Aichner [:anaran]]

(In reply to Kostas from comment #22)
> Currently Scratchpad displays either 3-number or 2-number line numbers for
> syntax errors: 
> eg.
> > @Scratchpad/3:1:1
> > @Scratchpad/3:1
> 
> As I've seen, the 1st number (3) is always the times Scratchpad has been
> launched in the current session, 
> i.e 3 means that during the current Firefox session, Scratchpad has been
> launched 3 times, i.e the current is the 3rd
> (the rest numbers are the line number and/or column number where the error
> was found).
> -I don't mean Scratchpad instances: just closing+reopening Scratchpad 3
> times-.
> 
> Is the first number needed?
> It's very confusing.

Please present the test case which goes along with above errors, which is the exact text content of that scratchpad.

The first number identifies the individual Scratchpad.

Scratchpads can be restored after a restart of firefox using the same profile.

Use File->Exit so that firefox will restore all tabs upon next launch.

> > @Scratchpad/3:1:1
tells me you have an error at column 1 of line 1.

> > @Scratchpad/3:1
tells me you have an error at line 1, column number unspecified.

Also, what is your firefox version and platform?

Try pressing Ctrl+J in Scratchpad while an error comment is still selected after evaluation.

It should prefill the jump to line field.

Just press Enter, and there you are, at the exact error location.

----Scratchpad/6----
my bad

/*
Exception: missing ; before statement
@Scratchpad/6:1
*/
--------

----Scratchpad/7----
let it = be

/*
Exception: be is not defined
@Scratchpad/7:1:9
*/
--------

Comment 24

[Adrian Aichner [:anaran]]

(In reply to adrian.aichner from comment #23)
> (In reply to Kostas from comment #22)
> > Currently Scratchpad displays either 3-number or 2-number line numbers for
> > syntax errors: 
> > eg.
> > > @Scratchpad/3:1
> tells me you have an error at column 1 of line 1.
> 
> > > @Scratchpad/3:1
> tells me you have an error at line 1, column number unspecified.
> 
> Also, what is your firefox version and platform?
> 
> Try pressing Ctrl+J in Scratchpad while an error comment is still selected
> after evaluation.
> 
> It should prefill the jump to line field.
> 
> Just press Enter, and there you are, at the exact error location.

Works for me in firefox 33.1.1 already.

Comment 25

[Kostas]

I use Firefox 33.1.1 (in windows 8.1x64)

> Please present the test case which goes along with above errors, which is
> the exact text content of that scratchpad.
There's not a specific test case where I get these two errors.
I just listed that in general the format of line numbers for syntax errors 
can only be either 2-number or 3-number.


> The first number identifies the individual Scratchpad.
That's what I also described in my comment.


> Scratchpads can be restored after a restart of firefox using the same
> profile.
> 
> Use File->Exit so that firefox will restore all tabs upon next launch.
It's great and very useful that Firefox also restores Scratchpads upon next launch.

But, my thinking is this:
the line numbers currently are in either of these two formats:
> @Scratchpad/S:x:y 
> @Scratchpad/S:x
where: 
S identifies the individual Scratchpad
x shows the line number
y shows the column number

i.e in all cases S is visible,
even when y is omitted (if it's unspecified).

But, for me the user, while working with code,
my concern is only the line number (x) and/or column number (y) where the error has occurred.
'S' is of no use to me.
In fact, displaying it all the time (when 'y' is omitted) : 
> @Scratchpad/S:x
confuses the user who might think that the error is:
> @Scratchpad/x:y
instead (in other words, he might think that S=x and x=y)


So, my suggestion is this: S to be omitted
and the line numbers to have the following format:
> @Scratchpad/x:y
> @Scratchpad/x:-  (when column number is omitted)

Comment 26

[Adrian Aichner [:anaran]]

Have you tried Ctrl+J when an evaluation error is selected (e.g. after Ctrl+R)?

Try.

Comment 27

[Kostas]

(In reply to adrian.aichner from comment #26)
> Have you tried Ctrl+J when an evaluation error is selected (e.g. after
> Ctrl+R)?
> 
> Try.
Yes, and it's very useful. Thanks for the tip.

My suggestion still stands though: can't the 'S' be omitted from the displayed error?
Is it necessary for the shortcut Ctrl+J to work?? (but, the "prefilled jump to line field" is in the format x:y or plain x, not S:x:y )
Can't 'S' be converted into an internal variable(not visible to the user)?

Comment 28

[Adrian Aichner [:anaran]]

(In reply to Kostas from comment #27)
> (In reply to adrian.aichner from comment #26)
> > Have you tried Ctrl+J when an evaluation error is selected (e.g. after
> > Ctrl+R)?
> > 
> > Try.
> Yes, and it's very useful. Thanks for the tip.
> 
> My suggestion still stands though: can't the 'S' be omitted from the
> displayed error?
> Is it necessary for the shortcut Ctrl+J to work?? (but, the "prefilled jump
> to line field" is in the format x:y or plain x, not S:x:y )
> Can't 'S' be converted into an internal variable(not visible to the user)?

Consider this:

Two Scratchpads running code against same firefox tab content and logging facts to the console.

How could I figure out from the log in the console I actually ran Scratchpad/3, then Scratchpad/4, then Scratchpad/3 again?

,--- Scratchpad/3
// Run some code, then state:
console.log("I'm here!")
// Who knows what else I am doing?
`---

,--- Scratchpad/4
// Run some *other code*, possibly having side effects on code in Scratchpad/3, then state:
console.log("I'm here!")
`---

,---Console
"I'm here!" Scratchpad/3:1
"I'm here!" Scratchpad/4:1
"I'm here!" Scratchpad/3:1
`---

Pretty easy to figure out how I used the scratchpads from just looking at the console.

Now drop the /N part and things are not at all clear anymore.

Comment 29

[Alan Thomas]

Kostas and Adrian: Your discussion is not relevant to this specific bug report, which is about line numbers not being provided for Scratchpad errors. 

Please file a new bug for your issue, if appropriate, or take your conversation to another venue. Thanks!

Comment 30

[Kostas]

Adrian, ok then. I didn't think of the case of two Scratchpads running simultaneously. 
Thanks for your time explaining this.