Created attachment 162984 [details] [review]
Correctly handle functions with out args and bool return value.

Functions with out args which return a boolean value usually return
false if the out args could not be set.

We handle those by discarding the boolean value if it is True, and by
returning None instead of a tuple if the C function returns False.

Review of attachment 162984 [details] [review]:

You have a number of unrelated patches in here

What unrelated patches? Should I split the python and C parts?

Comment on attachment 162984 [details] [review]
Correctly handle functions with out args and bool return value.

>From 36dc7c1ac9c98975abf39f536f237fcd2229a93d Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Steve=20Fr=C3=A9cinaux?= <code@istique.net>
>Date: Tue, 8 Jun 2010 00:24:30 +0200
>Subject: [PATCH] Correctly handle functions with out args and bool return value.
>
>Functions with out args which return a boolean value usually return
>false if the out args could not be set.
>
>We handle those by discarding the boolean value if it is True, and by
>returning None instead of a tuple if the C function returns False.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=620912
>---
> gi/overrides/Gdk.py |    4 ++--
> gi/overrides/Gtk.py |    6 +++---
> gi/pygi-invoke.c    |   14 +++++++++++++-
> 3 files changed, 18 insertions(+), 6 deletions(-)
>

* snip *

>diff --git a/gi/pygi-invoke.c b/gi/pygi-invoke.c
>index 8dc3d26..74248c0 100644
>--- a/gi/pygi-invoke.c
>+++ b/gi/pygi-invoke.c
>@@ -218,7 +218,7 @@ _prepare_invocation_state (struct invocation_state *state,
>     }
> 
>     state->n_return_values = state->n_out_args - state->n_aux_out_args;
>-    if (state->return_type_tag != GI_TYPE_TAG_VOID) {
>+    if (state->return_type_tag != GI_TYPE_TAG_VOID && state->return_type_tag != GI_TYPE_TAG_BOOLEAN) {
>         state->n_return_values += 1;
>     }

This isn't true for actual boolean returns such as gdk_pixbuf_format_is_writable

>@@ -748,6 +748,18 @@ _process_invocation_state (struct invocation_state *state,
>             if (state->return_type_tag == GI_TYPE_TAG_VOID) {
>                 /* The current return value is None. */
>                 Py_DECREF (state->return_value);
>+            } else if (state->return_type_tag == GI_TYPE_TAG_BOOLEAN) {
>+                if (state->return_value == Py_False) {
>+                    /* The function returned false. We must return None */
>+                    Py_DECREF (state->return_value);
>+                    Py_DECREF (return_values);
>+                    state->return_value = Py_None;
>+                    Py_INCREF (state->return_value);
>+                    return TRUE;
>+                }
>+                else {
>+                    Py_DECREF (state->return_value);
>+                }

So this isn't complete.  We can't assume that all boolean returns indicate success.  We either need to add an annotation to GObject-introspection (such as a success return type) or use some sort of heuristic.  I would say only do this for methods which have one out variable.  This is because returning None for something that returns a tuple would break something like (key, val) = success_func().  I would also process this right before we return as it should be easy to pop the boolean off the return stack and at that point you will have all the information needed to perform the correct heuristic. Good start though.

>             } else {
>                 /* Put the return value first. */
>                 g_assert (state->return_value != NULL);
>-- 
>1.7.1

Comment on attachment 162984 [details] [review]
Correctly handle functions with out args and bool return value.

>From 36dc7c1ac9c98975abf39f536f237fcd2229a93d Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Steve=20Fr=C3=A9cinaux?= <code@istique.net>
>Date: Tue, 8 Jun 2010 00:24:30 +0200
>Subject: [PATCH] Correctly handle functions with out args and bool return value.
>
>Functions with out args which return a boolean value usually return
>false if the out args could not be set.
>
>We handle those by discarding the boolean value if it is True, and by
>returning None instead of a tuple if the C function returns False.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=620912
>---
> gi/overrides/Gdk.py |    4 ++--
> gi/overrides/Gtk.py |    6 +++---
> gi/pygi-invoke.c    |   14 +++++++++++++-
> 3 files changed, 18 insertions(+), 6 deletions(-)
>
>diff --git a/gi/overrides/Gdk.py b/gi/overrides/Gdk.py
>index de35870..b4ad4e7 100644
>--- a/gi/overrides/Gdk.py
>+++ b/gi/overrides/Gdk.py
>@@ -72,6 +72,6 @@ __all__ = ['Rectangle', 'Color', 'Drawable']
> 
> import sys
> 
>-initialized, argv = Gdk.init_check(sys.argv)
>-if not initialized:
>+argv = Gdk.init_check(sys.argv)
>+if argv is None:
>     raise RuntimeError("Gdk couldn't be initialized")
>diff --git a/gi/overrides/Gtk.py b/gi/overrides/Gtk.py
>index 38b0dfa..916b340 100644
>--- a/gi/overrides/Gtk.py
>+++ b/gi/overrides/Gtk.py
>@@ -244,7 +244,7 @@ __all__ = ['ActionGroup', 'Builder', 'UIManager']
> 
> import sys
> 
>-initialized, argv = Gtk.init_check(sys.argv)
>-sys.argv = argv
>-if not initialized:
>+argv = Gtk.init_check(sys.argv)
>+if argv is None:
>     raise RuntimeError("Gtk couldn't be initialized")
>+sys.argv = argv


Oh, I see.  I wasn't quite sure what you were doing here but you were just fixing this up if the patch went in.  No need to split it out.

(In reply to comment #4)
> This isn't true for actual boolean returns such as
> gdk_pixbuf_format_is_writable

Right, I miss the checks for the number of out args

> So this isn't complete.  We can't assume that all boolean returns indicate
> success.  We either need to add an annotation to GObject-introspection (such as
> a success return type) or use some sort of heuristic.  I would say only do this
> for methods which have one out variable.

Actually, there is this "state->n_return_values > 1" check around that bit of code, which I think implements what you say at the end.

> This is because returning None for
> something that returns a tuple would break something like (key, val) =
> success_func().

Isn't that what pygtk used to do?

Taking out from the queue as we need tests (will ping J5 when I see him around).

Steve, handling such functions the way you propose may prevent other functions to be used correctly, since there is no indication that the boolean is about the successful completion of the function. We need annotations first.

We have had the current behaviour for at least two GNOME release cycles now. By now we have overrides and annotations which deal with the problem on a per-case basis, and programs which expect the current API. Special-casing this would IMHO be unexpected, an API break, and also not in line with GI's aim at keeping the original API.

So I'm tempted to just close this as wontfix at this point. Steve, what do you think?

What do the annotations look like?

As long as the behaviour is the one a python programmer would expect (ie, the one described here and used in pygtk) then fine by me. At the time of the patch there were no such annotations...

One instance which I'm aware of is that gdbus-codegen generates them that way. E. g. in udisks2:

/**
 * udisks_manager_call_loop_setup_sync:
[...]
 * Returns: (skip): %TRUE if the call succeded, %FALSE if @error is set.
 */

Otherwise, with PyGI most people now expect the C API, i. e. (return value, out arg1, out arg2, ...) as return value. After 1.5 years this should now be considered an API which we shouldn't deliberately break, and it's also helpful for consistency. Also, as Simon already said, there might well be functions which return a boolean which isn't mutually exclusive with the out argument.

Closing for now, as this would be quite a significant API break, introduce an inconsistency, and is potentially wrong for some methods.

Please feel free to reopen if you still think this is relevant. But I unmark the "patch" flag to get this off the patch review queue.

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