1245877 - Include links to external documentation where possible in JS error messages.

Status: RESOLVED DUPLICATE of bug 1179876

(Core :: JavaScript: Standard Library, defect)

Description

[Morgan Phillips [:mrrrgn]]

When a user sees an error message they don't understand they are forced to hunt down documentation. We should aid users by curating this process; including links to relevant MDN or Mozilla Wiki articles in messages (where possible).

Comment 1

[Jason Orendorff [:jorendorff]]

Spoke with jlongster on IRC. The plan is to add a .errorId property to all Error objects generated by the JS engine itself.

That alone is great, but not quite enough, because the devtools need to know *before* displaying an error message whether or not to display the link to more info.

So for now, I think we'll put a whitelist into the devtools source code: ids for which we have known-good content at some URL. When the console displays an error message for one of those ids, it'll display a link.

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: errid.diff

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: errid.diff

Hmm, so this gets the error ids added and attached to error objects. One thing that troubles me is whether or not this should be attached to the ErrorObject's prototype by default like lineNumber is. For now, it's not.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8721710 [details] [diff] [review]
errid.diff

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

Last we spoke, I was willing to just punch it through and put a nonstandard property on Error objects. But this isn't the done thing, so let's do it the right way: add a Debugger method to expose it. It could be a method on Debugger.Object objects.

Unfortunately this will make it harder to get DOMException working (we can't just throw the same-named property on those), but we'll just have to worry about that later.

::: js/src/js.msg
@@ +47,5 @@
>   */
>  
> +MSG_DEF(JSMSG_NOT_AN_ERROR,             0,  JSEXN_NONE,  "<Error #0 is reserved>", 100)
> +MSG_DEF(JSMSG_NOT_DEFINED,              1,  JSEXN_REFERENCEERR,  "{0} is not defined", 110)
> +MSG_DEF(JSMSG_MORE_ARGS_NEEDED,         3,  JSEXN_TYPEERR,  "{0} requires more than {1} argument{2}", 120)

Continuing to bikeshed on this setup...

It'll eventually get hard to verify by eyeball that these are unique - it's 500+ lines. 
And it's a likely mistake: copy and paste a line, change the message, forget to update the <ID> field.
Is it worth adding some code to check_spidermonkey_style.py (or somewhere) that checks them?

(We already found that <ARGUMENT_COUNT> is very easy to forget about, so in debug builds there's a startup assertion that checks all the argument counts. Maybe that's better than check_spidermonkey_style. 500x500/2 is kind of a lot, but it'll go fast.)

    for (size_t i = 0; i < 

I don't think counting by ten solves the problem you're trying to solve: error messages added at the end of the first section will quickly use up all the numbers between 770 and 780. How about counting by 1s, plus a comment:

    // The next available error id is: 379.  If you use it, remember to update this line.

This way we also wouldn't accidentally reuse an error-id after it gets deleted.

Comment 5

[Morgan Phillips [:mrrrgn]]

In the next patch I'll be using the message names themselves as unique identifiers, along with a debug build assertion that guarantees uniqueness.

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: errornames.diff

I've sat and thought about whether or not I should have named the debugger method getErrorId instead. In the end I decided to go with something that seems more accurate.

Comment 7

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8724295 [details] [diff] [review]
errornames.diff

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

Arg, wrong patch.

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: errornames.diff

Okay, this is my real patch.

Comment 9

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8724299 [details] [diff] [review]
errornames.diff

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

::: js/src/jit-test/tests/debug/Object-getErrorMessageName.js
@@ +21,5 @@
> +    } catch (ex) {
> +        assertEq(name, gw.getErrorMessageName(ex));
> +    }
> +}
> +

*doh* I left out a single test.

assertEq(gw.getErrorMessageName(new SyntaxError()), undefined);

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8724299 [details] [diff] [review]
errornames.diff

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

\o/

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3281396564c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Morgan Phillips [:mrrrgn]]

This needs one more patch before completion.