There are multiple components here.  The first question is - what is the typelib API that you need?  The current API relating to GErrors is:

gint                   g_type_info_get_n_error_domains (GITypeInfo *info);
GIErrorDomainInfo     *g_type_info_get_error_domain    (GITypeInfo *info,
							gint       n);

/* GIErrorDomainInfo */

const gchar *          g_error_domain_info_get_quark   (GIErrorDomainInfo *info);
GIInterfaceInfo *           g_error_domain_info_get_codes (GIErrorDomainInfo *info);

It appears the way this was designed is that from a GError * parameter, you can determine the error domains it throws, and from there, get an enum for the error codes.  Except that in practice, we're never writing out the error domains.  The typelib compiler accepts GError<domain1,domain2>, in the parser, but that's not written by the scanner, and I don't see any annotations.

I'm guessing the API that's desired here is more like:

GIInterfaceInfo *  g_error_get_codes (GQuark domain);


Now...the scanner code right now just does:

    def _resolve_quarks(self):
        for node in self._error_quark_functions:
            short = node.symbol[:-len('_quark')]
            enum = self._uscore_type_names.get(short)
            if enum is not None:
                enum.error_quark = node.symbol

We need some sort of annotation for this, because we're not scanning e.g. the Gio one correctly right now.

We could then just write out the error-quark function to the typelib as a member  of the EnumBlob, and when implementing the above function, scan through all enums in the namespace for ones that have matching error-quarks.

I just want to be able to identify whether or not a particular error is in a particular domain. Eg, I want to do the equivalent of this C code:

    if (error->domain == G_SPAWN_ERROR &&
        error->code == G_SPAWN_ERROR_NOENT) {
        /* handle this case */
    }

But this is not possible; given the information currently in the typelib, there's no way for a binding to figure out the value of G_SPAWN_ERROR, so there's nothing I can compare error.domain against. (Well, in this particular case, I could call GLib.spawn_error_quark(), but that only works because of bug 602512.)

However, adding the error-quark function name to the EnumBlob is still the right fix. The binding could then find all the error-quark functions in the repo, call them, and create a hash table mapping quarks to enum types. And then it can do something appropriate from there. (The minimal mapping would be to just define constants like GLib.SPAWN_ERROR with the values of the quarks, which the user can then compare against the quarks in the errors. A slightly more intricate mapping would involve translating the domains from quarks to type name strings when wrapping them, so you could do "if (error.domain == 'GLib.SpawnError')" etc. Various other things are also possible.)

Created attachment 149482 [details] [review]
[dumper] Handle glib:error-quark

Add glib:error-quark to EnumBlob and add g_enum_info_get_error_quark()
to retrieve it. This probably makes all of the "error domain" stuff
completely redundant.

(In reply to comment #3)
> This probably makes all of the "error domain" stuff
> completely redundant.

"all the error domain stuff *that isn't currently being used at all*" that is. The dumper generates ErrorDomainBlobs based on the contents of the .gir file, but the scanner never generates <errordomain> nodes.

The patch looks good to me, but Colin should probably take a look at it as well.

(In reply to comment #2)
> The binding could then find all the error-quark functions in the
> repo, call them, and create a hash table mapping quarks to enum types. And then
> it can do something appropriate from there.

Hm... except that means calling g_irepository_get_info() on every object in the typelib, which is probably not the best idea ever.

Two possibilities:

1. g_irepository_get_n_error_domains/_get_error_domain(), which would return GIErrorDomain objects, which g-ir-compiler would have generated based on the error enums, rather than having explicit <errordomain> nodes. Plus g_error_domain_info_get_enum_info(). Then the binding can iterate just the error domains, and continue as suggested above.

2. Instead of storing the name of the error-quark-returning method in glib:error-quark, store the re-stringified value of the quark itself instead, and have g_irepository_find_by_error_quark() or whatever. Then the binding doesn't need to "pre-load" the error domains, it can just deal with them as it sees them in GError values. (This would mean that the value of the error quarks would become part of the ABI though, which is a change.)

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

(In reply to comment #6)
> (In reply to comment #2)
> > The binding could then find all the error-quark functions in the
> > repo, call them, and create a hash table mapping quarks to enum types. And then
> > it can do something appropriate from there.
> 
> Hm... except that means calling g_irepository_get_info() on every object in the
> typelib, which is probably not the best idea ever.

The stack-allocated GIBaseInfo work that Colin did to implement g_callable_info_load_return_type(), etc, would help here a bit - it makes it possible to have a non-allocating g_irepository_load_info(). And the blob type is in the directory entry, so you don't have to page in parts of the type lib for non-enumeration types. Still:

 - You do have to load every enumeration type
 - You have to call every get_error_quark() function, paging in parts of the library

> Two possibilities:
> 
> 1. g_irepository_get_n_error_domains/_get_error_domain(), which would return
> GIErrorDomain objects, which g-ir-compiler would have generated based on the
> error enums, rather than having explicit <errordomain> nodes. Plus
> g_error_domain_info_get_enum_info(). Then the binding can iterate just the
> error domains, and continue as suggested above.

I think I'm in favor of separate GIErrorDomain objects, rather than piggybacking off of Enum blobs. (One side advantage of this is that it allows the error domain to be Gio.Error, even if the enumeration type sticks as Gio.ErrorEnum. Though maybe we should fix the enumeration type as well in introspection.) Using separate domain objects allows cleanly iterating just those objects, and also avoids putting fields onto Enum blobs that make sense only for a tiny subset.

But if we go this way, then I think we should do it at .gir generation time rather than in the compiler; the less the .gir format and the .typelib format are structurally different, then better.

> 2. Instead of storing the name of the error-quark-returning method in
> glib:error-quark, store the re-stringified value of the quark itself instead,
> and have g_irepository_find_by_error_quark() or whatever. Then the binding
> doesn't need to "pre-load" the error domains, it can just deal with them as it
> sees them in GError values. (This would mean that the value of the error quarks
> would become part of the ABI though, which is a change.)

This sounds like a good idea to me. I was initially worried about the hardcoding-the-quark-value part, especially considering that GLib has some broken quark values (g-exec-error-quark for g_spawn_error_quark()), but then I realized that we're considering the typelib and the library to be a unit, and gir-repository just a temporary artifact ... so you are free to change the quark value as long as you distribute a new typelib with the new library.

If we do this, then I don't see any reason to provide the function name in addition to the quark string - you can as easily and efficiently call g_quark_from_static_string() on the string value as dlsym() the function and call that.

Created attachment 150263 [details] [review]
Generate <errordomain/> nodes in gir and typelib

Instead of writing out glib:error-quark attributes for enumerations,
generate <errordomain/> nodes and ErrorDomain blobs as partly
implemented, but not finished. Fix parsing and generation code to
consistently use 'get-quark' attribute rather than 'getquark'.

Created attachment 150264 [details] [review]
Switch to storing string form of error quarks

Instead of storing the name of the function to call to get the
error quark, store the string form of the error quark, which
we derive from the introspection binary during scanning.

This will allow determining a back-mapping from error quark
to error domain without having to dlsym() and call all the
known error quark functions.

The patches I attached get the error domain quark string stored in the typelib; the remaining piece of the puzzle is to do the reverse mapping from quark to error domain, something along the lines of Walters:

 GIInterfaceInfo *  g_error_get_codes (GQuark domain);

(or g_irepository_get_error_codes()?)

One way of implementing that would be, when each typelib is registered with the repository, scan its directory for ErrorDomain blobs, then build a hash table of const char * => enumeration reference. That means paging in the entire directory for every typelib, but we do that anyways as soon as we need to resolve a symbol, so it probably isn't that bad, and I don't see any way to make it usefully lazy. You'll have to do all that work as soon as you first hit an error quark.

(A possibility for laziness would be to try and map back from the string form of the quark to the particular typelib it is in - to extract the prefix. But unless that's deterministically always successful, it doesn't really make things fundamentally better.)

The original intent may have been to use the ErrorType blob information for this - if you know that only that a few specific domains are possible then it's easy to find the right one - but a) we don't have that annotated b) the ability to map arbitrary GErrors seems useful.

A final open question is what we want to do with GioErrorEnum exactly.

Created attachment 150737 [details] [review]
Switch to storing string form of error quarks

This fixes a minor problem in generating the .C file for
the dumper - error quark functions weren't getting correctly
referenced.

(In reply to comment #8)
> > sees them in GError values. (This would mean that the value of the error quarks
> > would become part of the ABI though, which is a change.)
> 
> This sounds like a good idea to me. I was initially worried about the
> hardcoding-the-quark-value part, especially considering that GLib has some
> broken quark values (g-exec-error-quark for g_spawn_error_quark()), but then I
> realized that we're considering the typelib and the library to be a unit, and
> gir-repository just a temporary artifact ... so you are free to change the
> quark value as long as you distribute a new typelib with the new library.

that works for dynamic bindings, but statically-generated ones might want to be able to extract the value at binding-generation time. (Do the existing statically-generated bindings use the typelib at all at runtime? Using pre-generated ffi stubs would probably be faster than making libgirepository calls, so the bindings that already do that might want to keep doing it.)

But anyway, there might not really be any *problem* with declaring the quark values stable (and the g_spawn_error_quark() example seems to show that some people were already considering them stable), we'd just need to make sure people realized this.

Review of attachment 150737 [details] [review]:

Comments follow:

::: girepository/gdump.c
@@ +94,3 @@
+{
+invoke_error_quark (GModule *self, const char *symbol, GError **error)
+static GQuark

This should be 0, not G_TYPE_INVALID.

@@ +407,1 @@
+      if (strncmp (line, "get-type:", strlen ("get-type:")) == 0)

g_str_has_prefix (line, "get-type:")

@@ +428,3 @@
 	}
+	{
+      else if (strncmp (line, "error-quark:", strlen ("error-quark:")) == 0)

Ditto

::: girepository/ginfo.c
@@ +1112,3 @@
 const gchar *
 /* GIErrorDomainInfo functions */
+g_error_domain_info_get_domain (GIErrorDomainInfo *info)

It's probably OK to just nuke the old function, no one could have really been using it, but I'd like to be more explicit about this somehow.

Should we be bump the soname?

Created attachment 153953 [details] [review]
Delete ErrorDomain blob, replace with g_irepository_get_errordomain_codes

The previous ErrorDomain blob was never actually scanned or used, and
it was kind of a lame API conceptually.

Our new API allows you to look up the enumeration corresponding
to a GQuark from a GError.

To keep some compatibility, rather than removing the enumeration
values, rename them to _INVALID, and don't bump the typelib version.
This should in theory allow a new libgirepository to read an old
typelib.

Comment on attachment 153953 [details] [review]
Delete ErrorDomain blob, replace with g_irepository_get_errordomain_codes

needs test cases :)

>@@ -221,7 +221,7 @@ g_base_info_get_name (GIBaseInfo *info)
>-    case GI_INFO_TYPE_ERROR_DOMAIN:
>+    case GI_INFO_TYPE_INVALID_0:

maybe should go to the default: g_assert_not_reached() case?

>+          entry = g_typelib_get_dir_entry (typelib, i);
>+          if (entry->blob_type == BLOB_TYPE_ENUM)

shouldn't that be "!=" ?

>+          if (strcmp (error_quark, iface_data->error_quark) == 0)
>+            {
>+	          index = i;
>+              break;
>+	        }

space/tab confusion there


Note that this is only half the solution; it lets bindings interpret GErrors, but it doesn't let them generate them. We need g_enum_info_get_error_domain() too.

(In reply to comment #16)
> (From update of attachment 153953 [details] [review])
> needs test cases :)
> 
> >@@ -221,7 +221,7 @@ g_base_info_get_name (GIBaseInfo *info)
> >-    case GI_INFO_TYPE_ERROR_DOMAIN:
> >+    case GI_INFO_TYPE_INVALID_0:
> 
> maybe should go to the default: g_assert_not_reached() case?

That would mean we would fail on old typelibs.  Though practically speaking I should probably just bite the bullet and hard fail.
 
> >+          entry = g_typelib_get_dir_entry (typelib, i);
> >+          if (entry->blob_type == BLOB_TYPE_ENUM)
> 
> shouldn't that be "!=" ?

Yes, clearly I needed to actually test this as you say =)  I was working on a gjs patch.

> >+          if (strcmp (error_quark, iface_data->error_quark) == 0)
> >+            {
> >+	          index = i;
> >+              break;
> >+	        }
> 
> space/tab confusion there

g-i has this problem persistently unfortunately, it's mainly my fault.

> 
> Note that this is only half the solution; it lets bindings interpret GErrors,
> but it doesn't let them generate them. We need g_enum_info_get_error_domain()
> too.

Good point, will do.

(In reply to comment #17)
> > maybe should go to the default: g_assert_not_reached() case?
> 
> That would mean we would fail on old typelibs.

There are old typelibs with ERROR_DOMAIN nodes? (I was assuming that there weren't, and thus if you thought you saw one, it could only mean that the file was corrupt. Although it really ought to cleanly bail out in that case, not abort the program... but that's a separate bug.)

> > space/tab confusion there
> 
> g-i has this problem persistently unfortunately, it's mainly my fault.

to be clear, I was not complaining about the abstract problem, i was complaining specifically that the mixing of spaces and tabs on those particular lines results in crazy misalignment under 8-space tabs.

Created attachment 157186 [details] [review]
Switch to storing string form of error quarks

Rebase to current HEAD, old review comments would still apply

Created attachment 182414 [details] [review]
Switch to storing string form of error quarks

Rebase to current HEAD

Created attachment 182416 [details] [review]
Handle error_domain

(Based on Dan's patch)

Created attachment 182435 [details] [review]
Switch to storing string form of error quarks

OK... this bug has turned into a mess, with 4 people writing patches and no one wanting to obsolete anyone else's since we haven't decided on the right approach...

I think there's general agreement that we want to store the string form of the quark, not the getter function.

Owen suggested using ErrorDomain nodes to store the info rather than shoehorning it into Enum nodes, which seemed like a good idea to me at first, but I think it's actually a problem, because it means that we'd have an ErrorDomain node and an Enum node with the same name, and so, eg, g_irepository_find_by_name() could end up returning the one you don't want.

So I'm thinking it makes more sense to just ditch ErrorDomain and keep the data on Enum.

So that means we want:
    - Maxim's first patch, plus the parts of my original patch that he missed
      (EnumBlob gtk-docs,tests/scanner/foo-1.0-expected.tgir, and
      tools/generate.c), modified for s/quark/domain/
    - Maxim's second patch (formerly known as Owen's first patch), except
      rebased to work with enum nodes rather than errordomain nodes
    - Colin's patch, pretty much as is, although I don't know that I
      love "get_errordomain_codes" as the name... How about just
      g_irepository_get_error_domain(), and document that it returns
      a GIEnumerationInfo or NULL?

Created attachment 188180 [details] [review]
Deprecate ErrorDomain

The previous ErrorDomain blob was never actually scanned or used, and
it was kind of a lame API conceptually.

To keep some compatibility, rather than removing the enumeration
values, rename them to _INVALID, and don't bump the typelib version.
This should in theory allow a new libgirepository to read an old
typelib.

Based on a patch from Colin Walters

Created attachment 188181 [details] [review]
Switch to storing string form of error quarks

Instead of storing the name of the function to call to get the
error quark, store the string form of the error quark, which
we derive from the introspection binary during scanning.

Update EnumBlob and GIEnumInfo to include the new information.

This will allow determining a back-mapping from error quark
to error domain without having to dlsym() and call all the
known error quark functions.

Based on earlier patches from Owen Taylor and Maxim Ermilov.

Created attachment 188182 [details] [review]
Add g_irepository_find_by_error_domain()

Add a method to look up a GIEnumInfo given its associated error quark.

Based on a patch from Colin Walters.

Review of attachment 188180 [details] [review]:

Looks correct to me.

Review of attachment 188181 [details] [review]:

This looks good.

Review of attachment 188182 [details] [review]:

Minor doc bits, otherwise looks good.

::: girepository/girepository.c
@@ +713,3 @@
+ * Searches for the enum type corresponding to the given #GError
+ * domain. Before calling this function for a particular namespace,
+ * you must call #g_irepository_require once to load the namespace, or

Should be g_irepository_require() right?

@@ +717,3 @@
+ *
+ * Returns: (transfer full): #GIEnumInfo representing metadata about
+ * @domain's enum type, or %NULL

Leading @ will confuse gtk-doc and g-ir-scanner possibly; move it up a line?

@@ +718,3 @@
+ * Returns: (transfer full): #GIEnumInfo representing metadata about
+ * @domain's enum type, or %NULL
+ */

Can you add Since: 1.29.17 ?

pushed, along with a patch to fix the #g_irepository_require mistake
in all the other doc comments

Attachment 188180 [details] pushed as 4dabe20 - Deprecate ErrorDomain
Attachment 188181 [details] pushed as 5fd2fa5 - Switch to storing string form of error quarks
Attachment 188182 [details] pushed as c4c1de6 - Add g_irepository_find_by_error_domain()

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]