After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 649657 - Don't return gboolean for functions that throw
Don't return gboolean for functions that throw
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
: 599698 690904 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-05-07 13:33 UTC by David Zeuthen (not reading bugmail)
Modified: 2015-02-07 16:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (16.72 KB, patch)
2011-05-13 16:23 UTC, David Zeuthen (not reading bugmail)
reviewed Details | Review
Updated patch (16.50 KB, patch)
2011-05-13 17:33 UTC, David Zeuthen (not reading bugmail)
committed Details | Review
Move tests to Regress (11.23 KB, patch)
2011-05-13 22:27 UTC, David Zeuthen (not reading bugmail)
committed Details | Review

Description David Zeuthen (not reading bugmail) 2011-05-07 13:33:35 UTC
Hey,

So one goal of gdbus-codegen(1) is to ensure that the generated code is 100% introspectable so it's easy to use from high-level languages. There is one problem, however - consider a D-Bus method returning a string

 HelloWorld(IN String greeeting, OUT string response)

For this the C function

 /**
  * ...
  * @out_response: (out): Return location for response or %NULL.
  * ...
  */
 gboolean
 foo_bar_call_hello_world_sync (FooBar        *bar,
                                const gchar   *greeting,
                                gchar        **out_response,
                                GCancellable  *cancelable,
                                GError       **error);

is returned (we also generate async versions but let's not focus on that for now). However, this leads to two return values so the usage is

 ret = bar.call_hello_world_sync('hey', null /* cancelable */);
 response = ret[1]

I see two paths here

 - Somehow annotate the return value of foo_bar_call_hello_world_sync()
   with e.g. "(error)" so it will be ignored

 - Rework the code generated by gdbus-codegen(1) so it never returns
   a gboolean. This is inconvenient in C, however, because it's perfectly
   fine to pass NULL for the GError

If we go for the latter, I'll just reassign this to the gdbus component.

Thoughts?

Thanks,
David
Comment 1 Matthias Clasen 2011-05-07 15:53:15 UTC
I think introspection needs to recognize (or be hinted about) the very common pattern of boolean-return-plus-error
Comment 2 David Zeuthen (not reading bugmail) 2011-05-08 15:43:43 UTC
(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.
Comment 3 Colin Walters 2011-05-11 20:14:35 UTC
(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.
Comment 4 David Zeuthen (not reading bugmail) 2011-05-11 22:53:03 UTC
(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.
Comment 5 David Zeuthen (not reading bugmail) 2011-05-12 16:57:02 UTC
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.
Comment 6 David Zeuthen (not reading bugmail) 2011-05-12 16:58:13 UTC
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.
Comment 7 Colin Walters 2011-05-12 17:09:10 UTC
<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".
Comment 8 David Zeuthen (not reading bugmail) 2011-05-12 17:10:34 UTC
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
Comment 9 David Zeuthen (not reading bugmail) 2011-05-12 17:12:49 UTC
(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.
Comment 10 johnp 2011-05-12 17:18:39 UTC
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.
Comment 11 Colin Walters 2011-05-12 17:29:03 UTC
(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?
Comment 12 johnp 2011-05-12 17:41:40 UTC
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.
Comment 13 David Zeuthen (not reading bugmail) 2011-05-12 18:15:52 UTC
(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).
Comment 14 johnp 2011-05-12 18:37:29 UTC
Agreed, (skip) should work as long as it actually annotates the return as skipped instead of hiding it from the bindings.
Comment 15 David Zeuthen (not reading bugmail) 2011-05-13 16:23:41 UTC
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.
Comment 16 Colin Walters 2011-05-13 17:17:13 UTC
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.
Comment 17 David Zeuthen (not reading bugmail) 2011-05-13 17:33:47 UTC
Created attachment 187783 [details] [review]
Updated patch

Thanks for the review. Here's an updated patch.
Comment 18 Colin Walters 2011-05-13 18:15:20 UTC
Review of attachment 187783 [details] [review]:

Looks fine now, thanks!
Comment 19 David Zeuthen (not reading bugmail) 2011-05-13 18:22:27 UTC
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!
Comment 20 David Zeuthen (not reading bugmail) 2011-05-13 18:24:56 UTC
Also, gdbus-codegen(1) is now writing out that annotation, see

 http://git.gnome.org/browse/glib/commit/?id=ae10eca1162310b615db3bdce6791efcfc337c29
Comment 21 David Zeuthen (not reading bugmail) 2011-05-13 18:26:49 UTC
Reassigning this bug to gobject-introspection (not that it matters, I'm just a control freak).
Comment 22 David Zeuthen (not reading bugmail) 2011-05-13 18:30:36 UTC
OK, filed bug 650133 for gjs, bug 650134 for seed and bug 650135 for pygi.
Comment 23 David Zeuthen (not reading bugmail) 2011-05-13 22:27:43 UTC
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!
Comment 24 Colin Walters 2011-05-13 22:32:09 UTC
Review of attachment 187796 [details] [review]:

LGTM
Comment 25 David Zeuthen (not reading bugmail) 2011-05-13 22:36:03 UTC
Comment on attachment 187796 [details] [review]
Move tests to Regress

Committed, thanks!
Comment 26 David Zeuthen (not reading bugmail) 2011-05-14 00:43:27 UTC
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
Comment 27 Dan Winship 2011-05-15 13:57:49 UTC
(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.)
Comment 28 Dan Winship 2011-05-15 13:58:22 UTC
*** Bug 599698 has been marked as a duplicate of this bug. ***
Comment 29 Colin Walters 2011-05-16 19:32:11 UTC
(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?
Comment 30 Dan Winship 2011-05-16 20:05:33 UTC
(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.)
Comment 31 Philip Withnall 2012-12-30 23:31:34 UTC
*** Bug 690904 has been marked as a duplicate of this bug. ***
Comment 32 André Klapper 2015-02-07 16:47:34 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]