I think introspection needs to recognize (or be hinted about) the very common pattern of boolean-return-plus-error

(In reply to comment #1)
> I think introspection needs to recognize (or be hinted about) the very common
> pattern of boolean-return-plus-error

Maybe just use the existing (skip) annotation on params/return-val  to indicate that bindings should ignore it? That way I can just do this

 Returns: (skip): %TRUE if the operation succeeded, %FALSE if @error is set.

(In reply to comment #1)
> I think introspection needs to recognize (or be hinted about) the very common
> pattern of boolean-return-plus-error

I'd say bindings should ignore gboolean return values for functions that throw, yes.  So in this case the bug should be on gjs.

(In reply to comment #3)
> (In reply to comment #1)
> > I think introspection needs to recognize (or be hinted about) the very common
> > pattern of boolean-return-plus-error
> 
> I'd say bindings should ignore gboolean return values for functions that throw,
> yes.  So in this case the bug should be on gjs.

Sounds good to me. But it looks like pygi is also suffering from this problem (I didn't try Seed but I suspect it also suffers)... I'd feel more comfy if we made it explicit in the .gir and .typelib file that the return value should be ignored.

Looks like GLib is already suffering from this problem, see

 #!/usr/bin/python
 from gi.repository import GLib
 ret = GLib.file_get_contents('/etc/hosts')
 print ret

running it gives this

 $ python ./glib-test.py 
 (True, '127.0.0.1\tx61\tlocalhost.localdomain\tlocalhost\n::1\tx61\tlocalhost6.localdomain6\tlocalhost6\tx61-rawhide\n', 101L)

where the first element in the returned tupple is useless.

The concrete proposal here is

 - Make it possible to add (skip) to parameters and return values
 - Represent this in the Gir (what should it look like?)
 - Represent this in the typelibs (what should it look like?)
 - Add support in the gobject-introspection libraries
 - Add support to gjs, pygi, seed etc. to use this new flag

We could retroactively use (skip) on existing methods (such as g_file_get_contents() above) with the caveat that it breaks Introspection ABI.

<davidz> mclasen: are you saying that check_if_reactor_is_melting(GError **error); is not something we should support? E.g. it should be "gboolean check_if_reactor_is_melting(gboolean *out_is_melting, GError *error);" ?

I am really thinking "yes, it should be".  It's violating the GError convention for C.

So I'd still say that basically just the bindings should encode the rule "ignore gboolean return values for functions that throw=1".

Alternative plan proposed by Ben on IRC:

 - annotation should be called (dont-skip)
 - by default we skip any gboolean return value when the method/function throws
 - users can use (dont-skip) to change this

Note: when this is implemented, then the ABI of existing typelib suddenly changes

(In reply to comment #8)
> Alternative plan proposed by Ben on IRC:
> 
>  - annotation should be called (dont-skip)
>  - by default we skip any gboolean return value when the method/function throws
>  - users can use (dont-skip) to change this
> 
> Note: when this is implemented, then the ABI of existing typelib suddenly
> changes

For example it suddenly changes GLib.file_get_contents() to not return the useless (in Python) gboolean. Either way, I think this is not a break - it's a bugfix.

We discussed this a long time ago and it was decided for one reason or another
that this wasn't going to get fixed so PyGObject wrote overrides.  While I
agree this should be done, in order to keep ABI and not break PyGObject this
change needs to be distinct so we can choose whether or not to ignore it for
older bindings.

In other words, even if we allow you to annotate (skip) for the return, this
should still return the boolean on older PyGObject releases.

(In reply to comment #10)
> We discussed this a long time ago and it was decided for one reason or another
> that this wasn't going to get fixed so PyGObject wrote overrides.  While I
> agree this should be done, in order to keep ABI and not break PyGObject this
> change needs to be distinct so we can choose whether or not to ignore it for
> older bindings.

Well last time this came up was in the context of gtk_tree_model_iter_next() I believe, which *doesn't* throw a GError.  So that seems to me to be a distinct case. 
 
> In other words, even if we allow you to annotate (skip) for the return, this
> should still return the boolean on older PyGObject releases.

Right, that's the core problem.  If pygobject 3.0 is considering its ABI fixed, then we can't add an annotation.

Hm - or what do you mean "older bindings"?  Like pygobject 4 could change to ignore it?

I mean it doesn't change behaviour in the current stable bindings.  We can fix PyGObject going forward but we don't want to be dependent on specific versions of Gtk+.  PyGObject isn't centered around Gtk anymore.  Even if we can break ABI in PyGObject 3, PyGObject 2 still has to run with the latest Gtk3 library.  I am however leaning towards not breaking ABI in PyGObject 3.

Basically I want an app that targets PyGObject 2, to be able to be run unmodified in PyGObject 3 without suddenly breaking in PyGObject 2 because we updated the annotations.  

I know this isn't going to be 100% true since there are annotation bugs that get fixed that might break some obscure API but, when an API change covers a number of APIs and we have the ability to make it backwards compatible, we should.  e.g. we should make the new annotation change something that the older bindings will just pass through.

Now, since you said this is only for gboolean/gerror API, it isn't clear if any of the overrides we supply actually override those APIs.  In any case, I think gsuccess should be for all interfaces, but it should be up to the bindings to allow or ignore the bool.

(In reply to comment #11)
> Right, that's the core problem.  If pygobject 3.0 is considering its ABI fixed,
> then we can't add an annotation.

But we can, however, add support for a "(skip)" annotation like I originally proposed. Then new API can start using it right away... and existing API can start using it the next time they bump ABI.

Additionally, "(skip)" might be useful in other contexts too (can't think of any right now though but I'm not thinking very hard).

Agreed, (skip) should work as long as it actually annotates the return as skipped instead of hiding it from the bindings.

Created attachment 187781 [details] [review]
Proposed patch

Here's a patch that does this. We'll still need to add support to gjs, pygi and other bindings.

Review of attachment 187781 [details] [review]:

::: giscanner/girwriter.py
@@ +198,3 @@
             attrs.append(('transfer-ownership', return_.transfer))
+        if return_.skip or not return_.introspectable:
+            attrs.append(('introspectable', '0'))

Ugh.  Would really prefer to call the XML attribute here "skip"; the intent of "introspectable=0" was for entire *functions* to mean "omit this from the .gir".

While I see some vague symmetry in reusing the annotation (skip) for what you're doing - I don't have a better word - I think it makes sense to say "skip" here in the .gir instead of "introspectable=0".

This implies a new attribute for the Parameter class instead of piggybacking on the "introspectable" property of Node too.

Created attachment 187783 [details] [review]
Updated patch

Thanks for the review. Here's an updated patch.

Review of attachment 187783 [details] [review]:

Looks fine now, thanks!

Excellent, thanks for the review - I've updated https://live.gnome.org/GObjectIntrospection/Annotations with this annotation. I'll file bugs for gjs, pygi and seed - will follow up with the references here. Thanks!

Also, gdbus-codegen(1) is now writing out that annotation, see

 http://git.gnome.org/browse/glib/commit/?id=ae10eca1162310b615db3bdce6791efcfc337c29

Reassigning this bug to gobject-introspection (not that it matters, I'm just a control freak).

OK, filed bug 650133 for gjs, bug 650134 for seed and bug 650135 for pygi.

Created attachment 187796 [details] [review]
Move tests to Regress

This is so e.g. bindings can use this to check that they implement (skip) properly. OK to commit? Thanks!

Review of attachment 187796 [details] [review]:

LGTM

Comment on attachment 187796 [details] [review]
Move tests to Regress

Committed, thanks!

OK, to ensure that bindings actually handle this robustly, we need a bit more coverage. I committed a patch to do that:

 http://git.gnome.org/browse/gobject-introspection/commit/?id=36d4260baa5e6b03c14b8abdcffedf9b940bedb3

Thanks,
David

(In reply to comment #7)
> <davidz> mclasen: are you saying that check_if_reactor_is_melting(GError
> **error); is not something we should support? E.g. it should be "gboolean
> check_if_reactor_is_melting(gboolean *out_is_melting, GError *error);" ?
> 
> I am really thinking "yes, it should be".  It's violating the GError convention
> for C.

(FTR, there *are* a handful of existing methods that violate this rule. Eg, g_key_file_has_key() returns FALSE with no error if you give it a valid group name and non-existent key name, but returns FALSE with an error if you give it a non-existent group name.)

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

(In reply to comment #27)
> (In reply to comment #7)
> > <davidz> mclasen: are you saying that check_if_reactor_is_melting(GError
> > **error); is not something we should support? E.g. it should be "gboolean
> > check_if_reactor_is_melting(gboolean *out_is_melting, GError *error);" ?
> > 
> > I am really thinking "yes, it should be".  It's violating the GError convention
> > for C.
> 
> (FTR, there *are* a handful of existing methods that violate this rule. Eg,
> g_key_file_has_key() returns FALSE with no error if you give it a valid group
> name and non-existent key name, but returns FALSE with an error if you give it
> a non-existent group name.)

Okay then, let's fix it:
https://bugzilla.gnome.org/show_bug.cgi?id=650345

Any others?

(In reply to comment #29)
> Any others?

Probably, but I don't know what they are. (At the time I looked, I only cared if the answer was 0 or not-0, so I stopped looking after I found the first one.)

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

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