*** Bug 602449 has been marked as a duplicate of this bug. ***

Dan points out in bug 602449 that the original error message is also interesting, since the stringified version of the exception contains excess noise.

Working through this in some more detail, commenting in chunks because there's no draft saving in Bugzilla.

Preferrably the stringification of the exception would give the domain and code. This is easy enough to do if we want:

 g-shell-error-quark: 0: Quoted text doesn't begin with a quotation mark

But getting to:

 GLib.ShellError.BAD_QUOTING: Quoted text doesn't begin with a quotation mark

is somewhat harder. There's some rudimentary support for enum to error quark mappings in gobject-introspection, but it doesn't seem finished:

 - The glib:error-quark enumeration is only emitted into the output for introspected enumerations; there's no good reason for this, except that glibtransformer.py:_resolve_quarks() works off of the list of GObject registered type names instead of going off all known type names.

 - The glib:error-quark value gives a function name; maybe this is OK, but it requires calling all the error quark functions we know about to try and find a match. (That would call g_quark_from_static_string(), page in program text, allocate hash nodes, etc.) It might be handy to have the atom value in addition. *Or* we could try to derive function name (or type name) from the quark value at run time. But:

There are some consistency problems in the quark names. Looking at GLib and Gio:

 Quark value                 Enum name          Function returning quark

 g-key-file-error-quark      GKeyFileError      g_key_file_error_quark
 g-markup-error-quark        GMarkupError       g_markup_error_quark
 g-bookmark-file-error-quark GBookmarkFileError g_bookmark_file_error_quark
 g-file-error-quark          GFileError         g_file_error_quark
 g-shell-error-quark         GShellError        g_shell_error_quark
 g-regex-error-quark         GRegexError        g_regex_error_quark
 g-io-channel-error-quark    GIOChannelError    g_io_channel_error_quark
 g-resolver-error-quark      GResolverError     g_resolver_error_quark

But:

 g-option-context-error-quark GOptionError   g_option_error_quark
 g_thread_error               GThreadError   g_thread_error_quark
 g_convert_error              GConvertError  g_convert_error_quark
 g-exec-error-quark           GSpawnError    g_spawn_error_quark
 g-io-error-quark             GIOErrorEnum   g_io_error_quark

Wouldn't be that hard to handle that range of enums, and most of them could be fixed over the long run, if nobody is already depending on the quark values (I guess a language binding might be?)

One of the fundamental questions here is what is the style we expect.

 try {
 } catch (e) {
   if (e instanceof GjsError && 
       e.domain = GLib.SHELL_ERROR && e.code == GLib.ShellError.BAD_QUOTING) {
       /* do something */       
   } else {
       throw e;
   }
 }

[ GLib.SHELL_ERROR or GLib.shell_error_quark() aren't defined at the moment in the .gir, not sure why the second isn't there ]

[...]
   if (e.domain == GLib.SHELL_ERROR && e.code == GLib.ShellError.BAD_QUOTING) {
[...]

[...]
   if (e.name == "GLib.ShellError.BAD_QUOTING")
[...]

Or using a JS 1.5 addition:

 try {
 } catch (e if e.name == "GLib.ShellError.BAD_QUOTING")
     /* do something */
 }

The first with the instanceof *and* the checks on domain/code is pretty horrifying, so that makes me feel that we could get away without a subclass. The advantage of not using a subclass is that we easily get filename/linenumber information if we throw an Error, while it's quote hard to subclass and still have that ability.

see bug 602512 (non-gtype-registered error domains don't get marked) and bug 602516 (error domain info doesn't end up in typelib).

another possibility btw is:

    if (e.domain == GLib.ShellError && e.code == GLib.ShellError.BAD_QUOTING)

(ie, error->domain gets translated into the js object that the enum values are stored on). This preserves the "domain is a prefix of code" property that it has in C.

Created attachment 180794 [details] [review]
add code/domain properties to Error for thrown GError

e.domain - error domain string
e.code - error code

(In reply to comment #6)
> Created an attachment (id=180794) [details] [review]
> add code/domain properties to Error for thrown GError
> 
> e.domain - error domain string
> e.code - error code

Hmm, I guess that sort of works, but I don't think it's good enough.

 454	assertEquals(x.domain, "g-io-error-quark");

is clarly an internal detail leaking out not an API. Someone is going to have to sort through the mess in 602516 from last year and figure out what to do so we actually have a back-mapping from the string to the public API.

(In reply to comment #5)
> see bug 602512 (non-gtype-registered error domains don't get marked) and bug
> 602516 (error domain info doesn't end up in typelib).
> 
> another possibility btw is:
> 
>     if (e.domain == GLib.ShellError && e.code == GLib.ShellError.BAD_QUOTING)
> 
> (ie, error->domain gets translated into the js object that the enum values are
> stored on). This preserves the "domain is a prefix of code" property that it
> has in C.We really  

Hmm, I sort of like this, though, Gio.IOErrorEnum is an ugly case. (We really should have de-worked around the clash with G.IOError at GIR generation time rather than leaking that workaround to the GIR API, but I don't know if we have the facility for doing this and it's possibly too late now.)

Note that it doesn't solve the subclassing problem - you can't write 

 except (e if e.domain == GLib.ShellError && e.code == GLib.ShellError.BAD_QUOTING)

Because that will produce warnings in SpiderMonkey strict mode about e.domain being undefined. The e.name thing I suggested is using the fact that name is a standard Javascript property, but is maybe a bit hacky/ugly. 

 except (e if e instanceof GjsError && 
              e.domain == GLib.ShellError && 
              e.code == GLib.ShellError.BAD_QUOTING)

isn't _that_ bad. Dunno.

one more alternative might be to export g_error_matches() in a way, so you could do something like

if (GLib.Error.matches(e, GLib.ShellError, GLib.ShellError.BAD_QUOTING))

or maybe even drop the somewhat redundant prefix:

if (GLib.Error.matches(e, GLib.ShellError.BAD_QUOTING))

(In reply to comment #8)
> one more alternative might be to export g_error_matches() in a way, so you
> could do something like
> 
> if (GLib.Error.matches(e, GLib.ShellError, GLib.ShellError.BAD_QUOTING))
> 
> or maybe even drop the somewhat redundant prefix:
> 
> if (GLib.Error.matches(e, GLib.ShellError.BAD_QUOTING))

GLib.ShellError.BAD_QUOTING is just a number

(In reply to comment #9)
> 
> GLib.ShellError.BAD_QUOTING is just a number

Sure, but I'm not sure it's an absolute requirement. If it makes sense to handle domain and code separately, then sure, but otherwise it could be an object encoding both domain and code into one. But maybe not worth the trouble.

Actually another option I didn't see would be to have each domain mapped to a class, then I think the use could be maybe bit more javascript-like, e.g.

except (e if e instanceof GLib.ShellError)

Created attachment 182415 [details] [review]
add code/domain properties to Error for thrown GError

Allow to do:
    catch (x if x.domain && x.domain == Gio.IOErrorEnum)

(depends on 2 patches from 602516)

Comment on attachment 182415 [details] [review]
add code/domain properties to Error for thrown GError

so in this patch, "e.domain == GLib.IOErrorEnum" only works because you changed toString() on GLib.IOErrorEnum, which seems like a kludge.

also, at some point we may want to be able to throw GErrors as well (not sure if throwing errors from callbacks is supported, but if we ever implement subclassing we'd need to be able to do it).

So I think we should do it like Tommi suggested; when creating an enum object, if it has an error-domain, make the enum object be a subclass of Error, and when converting a GError to an exception, create an error of the appropriate class. And then we can just do

    except (e if (e instanceof GLib.ShellError && 
                  e.code == GLib.ShellError.BAD_QUOTING))

and in the future,

    throw new GLib.ShellError(GLib.ShellError.BAD_QUOTING, _("Blah blah blah"));

Actually, we probably want GLib.Error, which is a subclass of Error, and each error domain is a subclass of GLib.Error, so we can distinguish errors-translated-from-glib from errors-thrown-by-javascript, if need be.

All of this was nice in theory, but never implemented, as it is pretty complex to do (you need the same dynamic subclassing as boxed and object)
As a stop gap for 3.4, for apps that want to handle GErrors right now, can we get this patch merged?

(If overriding toString() is perceived bad, and it probably is, JSExtendedClass has a JSEqualityOp to override the behavior of ==)

Ok, it turned out to be easier than I thought.
The next patch implements the proposed behaviour, with error enums that become classes of which the exception are instances.

I though of supporting vala style "throw new Gio.IOErrorEnum.CANCELLED(message)", but then implemented the simpler "throw new Gio.IOErrorEnum({ code: Gio.IOErrorEnum.CANCELLED, message: message })". Actually, there is little use for throwing GErrors, since they're not yet propagated to C callers.

On the catching side, the usage pattern is
try {
   fail();
} catch(e if e instanceof Gio.IOErrorEnum &&
             e.code == Gio.IOErrorEnum.CANCELLED) {
   print("Operation cancelled");
} catch(e) {
   print("Something else happened: " + e);
}

Finally, such classes don't inherit from Error. Instead, they inherit from GLib.Error, the standard GI boxed class. This means for example that the above can be written as
catch(e if e.matches(Gio.io_error_quark(), Gio.IOErrorEnum.CANCELLED))

Created attachment 208882 [details] [review]
Introduce special marshalling for GErrors

Previously GErrors were transformed into plain Error with a custom
message, which removed the code and domain metadata. This commit
introduces a new class hierarchy (derived from GLib.Error) for each
enumeration representing an error domain, and modifies existing
code to throw instances of that when a function fails.

(In reply to comment #15)
> Finally, such classes don't inherit from Error. Instead, they inherit from
> GLib.Error, the standard GI boxed class.

This means they don't get the "stack" property with a stack trace like "real" errors do... but I think it's pretty easy to generate that

Created attachment 208909 [details] [review]
GError: add stack, fileName and lineNumber

Similar to native Errors(), GLib.Error is extended to provide debug
information in the form of fileName, lineNumber and stack (obtained
using the JS debug API). At the same time, the existing stack logging
facility is modified to be similar in format to the native one, and
logError is modified to avoid iterating object properties (which
gives an undefined order, and does not include prototype properties)

I don't see why we're inheriting from GLib.Error and emulating the JS Error class, rather than just inheriting from the JS Error class.

Because you cannot "inherit" from a JS Error class. What you can do is create a custom class that happens to have .__proto__ = Error.prototype, but that won't give you special properties (they are set in the constructor). BTW, none of the other standard error classes (TypeError, RangeError, SyntaxError, EvalError, ReferenceError, URIError) inherit from Error.prototype, or even share it.
On the other hand, inheriting from GLib.Error (as much as allowed by JS) means that "e instanceof GLib.Error" works, and "e.matches(domain, code)" works.

(In reply to comment #20)
> Because you cannot "inherit" from a JS Error class. What you can do is create a
> custom class that happens to have .__proto__ = Error.prototype

That's inheritance in JS, isn't it?

> but that won't
> give you special properties (they are set in the constructor).

So call the parent constructor in our constructor? (Well, it may be not worth it from C.)

> BTW, none of the
> other standard error classes (TypeError, RangeError, SyntaxError, EvalError,
> ReferenceError, URIError) inherit from Error.prototype, or even share it.

    > TypeError.prototype.__proto__ == Error.prototype
    true

(In reply to comment #21)
> (In reply to comment #20)
> > Because you cannot "inherit" from a JS Error class. What you can do is create a
> > custom class that happens to have .__proto__ = Error.prototype
> 
> That's inheritance in JS, isn't it?

Yeah, but doing it with objects having different JSClass is a call for trouble (just look at GIRepositoryFunction.toString()).

> > but that won't
> > give you special properties (they are set in the constructor).
> 
> So call the parent constructor in our constructor? (Well, it may be not worth
> it from C.)

Uhm... How? Error() returns a new instance, with a different prototype and JSClass.

> > BTW, none of the
> > other standard error classes (TypeError, RangeError, SyntaxError, EvalError,
> > ReferenceError, URIError) inherit from Error.prototype, or even share it.
> 
>     > TypeError.prototype.__proto__ == Error.prototype
>     true

Ugh, you're right... Probably I made a mistake last time I checked.

It's probably worth looking at how Gecko creates custom error subclasses internally.

For JS Errors:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp#812
They call InitErrorClass for each of the derived class, which essentially does a JS_InitClass using the internal API, passing the same constructor as Error(), and redefining special properties.

For XP Connect:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCThrower.cpp#246
As I read it, they just wrap the native object without special code.

Hm, I thought the HTML5 specification (or something else like Web Workers) defined some error subclasses that aren't part of ECMA-262.

If you're referring to DOMException, all I could find is
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMException.cpp
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCException.cpp#269
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCStack.cpp#97

which seems to show they reimplement it at the C++ level and then use XPConnect to bring it back to JS.

Review of attachment 208909 [details] [review]:

I'm not a fan of this, but I guess we have no other real option until the SpiderMonkey developers give us a new API. They said they were going to because they had a need internally in Gecko to create actual Error subclasses.

Review of attachment 208882 [details] [review]:

Mostly good.

::: gi/arg.c
@@ +1343,3 @@
                     !g_type_is_a(gtype, G_TYPE_CLOSURE)) {
+
+                    if (gtype == G_TYPE_ERROR) {

g_type_is_a

::: gi/gerror.c
@@ +67,3 @@
+    GJS_NATIVE_CONSTRUCTOR_PRELUDE(error);
+
+    /* Check early to avoid allocating memory for nothing */

You've already constructed a new object in the prelude, so you're still allocating memory. Do this before the prelude.

@@ +75,3 @@
+    priv = g_slice_new0(Error);
+
+    GJS_INC_COUNTER(boxed);

Define a new counter.

@@ +145,3 @@
+
+static JSBool
+error_getter(JSContext *context, JSObject *obj, jsid id, jsval *vp)

Would make more sense to me to have three difference getters instead of one.

@@ +151,3 @@
+    int tinyid;
+
+    /* Optimization: let JS cache the value */

Not sure I understand this.

@@ +220,3 @@
+
+        if (!gjs_string_from_utf8(context, descr, -1, &v_out)) {
+            g_free(descr);

I'd prefer a standard "goto out;" strategy.

Giovanni: ping?

Sorry, it slipped out of my sight.

(In reply to comment #28)
> @@ +151,3 @@
> +    int tinyid;
> +
> +    /* Optimization: let JS cache the value */
> 
> Not sure I understand this.

After the getter runs, the engine takes the returned jsval and stores it. Since properties don't change dynamically, the stored value is always good, so we can avoid marshalling the same value over it.

Revised patch coming...

Created attachment 215760 [details] [review]
Introduce special marshalling for GErrors

Previously GErrors were transformed into plain Error with a custom
message, which removed the code and domain metadata. This commit
introduces a new class hierarchy (derived from GLib.Error) for each
enumeration representing an error domain, and modifies existing
code to throw instances of that when a function fails.

Review of attachment 215760 [details] [review]:

Looks mostly fine.

::: gi/function.c
@@ +675,3 @@
         g_assert_cmpuint(0, <, c_argc);
 
         if (type == GI_INFO_TYPE_STRUCT || type == GI_INFO_TYPE_BOXED) {

I wonder if we could reuse some of the same code from arg.c here.

@@ +679,3 @@
+
+            /* GError must be special cased */
+            if (gtype == G_TYPE_ERROR)

g_type_is_a

::: gi/value.c
@@ +391,3 @@
             obj = JSVAL_TO_OBJECT(value);
+
+            if (gtype == G_TYPE_ERROR) {

g_type_is_a

::: test/js/testEverythingBasic.js
@@ +528,3 @@
+    } catch (x) {
+	assertTrue(x instanceof Gio.IOErrorEnum);
+	assertTrue(x.matches(Gio.io_error_quark(), Gio.IOErrorEnum.NOT_FOUND));

I don't really like the Gio.io_error_quark() syntax, but sure.

Maybe you could somehow add a Gio.IOErrorEnum.$error_quark so that you could do:

    x.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.NOT_FOUND);

still suboptimal, but until we get an actual class for enumerations, it's the best we can do.

(In reply to comment #32)
> Review of attachment 215760 [details] [review]:
> 
> Looks mostly fine.
> 
> ::: gi/function.c
> @@ +675,3 @@
>          g_assert_cmpuint(0, <, c_argc);
> 
>          if (type == GI_INFO_TYPE_STRUCT || type == GI_INFO_TYPE_BOXED) {
> 
> I wonder if we could reuse some of the same code from arg.c here.

arg.c "public" API is in terms of GIArgInfo, not GITypeInfo, so at least not now. Next patch, maybe.

Created attachment 215858 [details] [review]
Introduce special marshalling for GErrors

Previously GErrors were transformed into plain Error with a custom
message, which removed the code and domain metadata. This commit
introduces a new class hierarchy (derived from GLib.Error) for each
enumeration representing an error domain, and modifies existing
code to throw instances of that when a function fails.

About the GQuark stuff, I didn't want to add more special cases in
arg.c, so I went with Gio.IOErrorEnum.valueOf() returning the GQuark.
Also, I added Error.prototype.matches(), allowing for a final API like:
catch(e if e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)), which is
neat, IMHO

(In reply to comment #34)
> Created an attachment (id=215858) [details] [review]
> Introduce special marshalling for GErrors

> About the GQuark stuff, I didn't want to add more special cases in
> arg.c, so I went with Gio.IOErrorEnum.valueOf() returning the GQuark.

ugh. Clever, but *ugh*.

> Also, I added Error.prototype.matches(), allowing for a final API like:
> catch(e if e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)), which is
> neat, IMHO

What's wrong with g_error_matches() ?

(In reply to comment #35)
> (In reply to comment #34)
> [...]
> 
> What's wrong with g_error_matches() ?

Nothing, but if you get a TypeError or a ReferenceError, you want that to pass through and be logged, and not be replaced by the new TypeError caused by calling a non-existing function.
In fact, my Error.prototype.matches() always returns false. You need the real one to do the actual work.

Oh, gotcha. Again, a bit disgusting.

Review of attachment 215858 [details] [review]:

This looks fine.

Attachment 208909 [details] pushed as 52e921d - GError: add stack, fileName and lineNumber
Attachment 215858 [details] pushed as 2ab1b3f - Introduce special marshalling for GErrors