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 566571 - Overriding base class interface methods requires explicitly re-inheriting from the interface
Overriding base class interface methods requires explicitly re-inheriting fro...
Status: RESOLVED OBSOLETE
Product: pygobject
Classification: Bindings
Component: gobject
Git master
Other All
: Low minor
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
: 703338 730740 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-01-04 23:24 UTC by Dimitri Tcaciuc
Modified: 2018-01-10 19:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test case + description why it is not easy to solve + code cleanup (8.83 KB, text/plain)
2009-01-07 19:42 UTC, Paul Pogonyshev
Details
potential fix (1.86 KB, text/plain)
2009-04-22 18:17 UTC, Paul Pogonyshev
Details

Description Dimitri Tcaciuc 2009-01-04 23:24:18 UTC
Please describe the problem:
The bug manifests itself when trying to load custom class using gtk.Builder from a definition file. 

In my case, the custom class is derived from gtk.Dialog, which already implements gtk.Buildable. I need to detect when the parsing is done, so I'm overriding the gtk.Buildable.parser_finished() method like so:

class MyDialog(gtk.Dialog):
    __gtype_name__ = "MyDialog"
    
   def do_parser_finished(self, builder):
        print "Parser finished"

However, the method is never called. It -is- called, however, if I directly re-inherit from gtk.Buildable interface:

class MyDialog(gtk.Dialog, gtk.Buildable):



Steps to reproduce:
1. Inherit from a class which already inherits from gtk.Buildable interface 
2. Override do_parser_finished(self, builder) method 
3. Declare an instance of that class in a gtk.Builder XML file.
4. Load the XML file with gtk.Builder.add_from_file()

Actual results:
At the end of add_from_file() method call, the do_parser_finished() should be invoked to indicate the end of parsing, however it doesn't.

Expected results:
From what I've been told to on IRC discussion, dynamic override finder should notice when we derive from a class implementing gtk.Buildable and should not require inheriting it second time. 


Does this happen every time?
To my knowledge, yes

Other information:
The test driver code:

# BEGIN -----------------------------------
builder = gtk.Builder()
builder.add_from_file("test.glade")
dialog = builder.get_object("mydialog-instance")
# END -------------------------------------

Test XML file:

<?xml version="1.0"?>
<interface>
<!-- interface-naming-policy project-wide -->
  <object class="MyDialog" id="mydialog-instance">
    <property name="border_width">5</property>
    <property name="type_hint">normal</property>
    <property name="has_separator">False</property>
    <child internal-child="vbox">
      <object class="GtkVBox" id="dialog-vbox1">
        <property name="visible">True</property>
        <property name="spacing">2</property>
        <child>
          <placeholder/>
        </child>
        <child internal-child="action_area">
          <object class="GtkHButtonBox" id="dialog-action_area1">
            <property name="visible">True</property>
            <property name="layout_style">end</property>
            <child>
              <placeholder/>
            </child>
            <child>
              <placeholder/>
            </child>
          </object>
          <packing>
            <property name="expand">False</property>
            <property name="pack_type">end</property>
            <property name="position">0</property>
          </packing>
        </child>
      </object>
    </child>
  </object>
</interface>
Comment 1 Paul Pogonyshev 2009-01-07 19:39:37 UTC
This is a fault of PyGObject, both runtime and codegen.  Details follow.
Comment 2 Paul Pogonyshev 2009-01-07 19:42:14 UTC
Created attachment 125957 [details]
test case + description why it is not easy to solve + code cleanup

This patch adds a proper unit test (disabled, because the bug is not solved).  It also cleans up relevant piece of code by breaking out code duplication into a common function and adds several comments describing (clumsily) why it is not possible or at least not easy to solve this bug now.
Comment 3 Paul Pogonyshev 2009-01-07 19:47:20 UTC
Just to note: to solve this bug we first need to make codegen not override virtual methods (at C level) with *_do_proxy_*() when corresponding do_*() methods are not overriden.  Then we can fix runtime type registration to avoid bugs like this.  Currently gtk.Buildable interface initialization contains this code:

    py_method = pytype? PyObject_GetAttrString((PyObject *) pytype, "do_parser_finished") : NULL;
    if (py_method && !PyObject_TypeCheck(py_method, &PyCFunction_Type)) {
        iface->parser_finished = _wrap_GtkBuildable__proxy_do_parser_finished;
    } else {
        PyErr_Clear();
        if (parent_iface) {
            iface->parser_finished = parent_iface->parser_finished;
        }
    Py_XDECREF(py_method);
    }

Since gtk.Dialog already contains do_parser_finished() (invoking the default implementation for C GtkDialog class), this code will always set structure field to the proxy method.  This is bad for performance reasons as it is and worse yet if we unconditionally initialize interfaces.  Currently runtime only initializes interfaces when you explicitly list them as bases of your subclass.
Comment 4 Gustavo Carneiro 2009-01-09 11:34:07 UTC
Well, you could choose to _not_ fix the bug ;-)

I am the one responsible for the current interface implementation code, and I never thought of this as much of a problem.  Because of potential performance problems that you noticed, I always thought it was a good thing that you need to explicitly list implemented interfaces in the base types.

It is true that one possible optimization could be to check at type registration time whether or not a do_xxx method exists, and avoid redirecting the virtual method through a proxy wrapper in case the do_xxx is not defined.  Johan Dahlin at the time did not like that approach, though, since it kind of goes against Python principles (makes it less dynamic).  I had partially agreed with him, but in retrospect I am more inclined to agree that the optimization is worth the loss of dynamism.

On the other hand it just occurred to me another conflicting problem.  You see, all virtual methods of all implemented interfaces are mapped into a single namespace.  Conflicts are bound to arise (there's a bug report open about it IIRC).  If you start to no longer require listing implemented interfaces in the base types list then I suspect the conflicts will be much more frequent.
Comment 5 Paul Pogonyshev 2009-01-09 18:03:37 UTC
(In reply to comment #4)
> I am the one responsible for the current interface implementation code, and I
> never thought of this as much of a problem.  Because of potential performance
> problems that you noticed, I always thought it was a good thing that you need
> to explicitly list implemented interfaces in the base types.

One pretty simple idea is to check class dictionary instead of class object.  I.e. the former shouldn't find inherited methods.  As it is currently, standard methods already define do_*() so you can invoke super method in your custom override.  So, simply declaring an interface as your explicit base will drag in all proxies, regardless of whether you actually override anything.

> It is true that one possible optimization could be to check at type
> registration time whether or not a do_xxx method exists, and avoid redirecting
> the virtual method through a proxy wrapper in case the do_xxx is not defined. 
> Johan Dahlin at the time did not like that approach, though, since it kind of
> goes against Python principles (makes it less dynamic).  I had partially agreed
> with him, but in retrospect I am more inclined to agree that the optimization
> is worth the loss of dynamism.

I tend to value optimization more in this case too.  However, out of purity reasons we could later use "light" proxies.  I.e. proxies that would just invoke C-level function if no override is detected in runtime, bypassing all the type conversion and all that stuff.

> On the other hand it just occurred to me another conflicting problem.  You see,
> all virtual methods of all implemented interfaces are mapped into a single
> namespace.  Conflicts are bound to arise (there's a bug report open about it
> IIRC).  If you start to no longer require listing implemented interfaces in the
> base types list then I suspect the conflicts will be much more frequent.

That's a valid point that never occured to me.  Need to have a look at practical implications at least for PyGTK before doing anything regarding this.
Comment 6 Gustavo Carneiro 2009-01-09 18:51:33 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > I am the one responsible for the current interface implementation code, and I
> > never thought of this as much of a problem.  Because of potential performance
> > problems that you noticed, I always thought it was a good thing that you need
> > to explicitly list implemented interfaces in the base types.
> 
> One pretty simple idea is to check class dictionary instead of class object. 
> I.e. the former shouldn't find inherited methods.  As it is currently, standard
> methods already define do_*() so you can invoke super method in your custom
> override.  So, simply declaring an interface as your explicit base will drag in
> all proxies, regardless of whether you actually override anything.

Sure, but at least only the proxies for the interfaces you explicitly choose to implement.

> 
> > It is true that one possible optimization could be to check at type
> > registration time whether or not a do_xxx method exists, and avoid redirecting
> > the virtual method through a proxy wrapper in case the do_xxx is not defined. 
> > Johan Dahlin at the time did not like that approach, though, since it kind of
> > goes against Python principles (makes it less dynamic).  I had partially agreed
> > with him, but in retrospect I am more inclined to agree that the optimization
> > is worth the loss of dynamism.
> 
> I tend to value optimization more in this case too.  However, out of purity
> reasons we could later use "light" proxies.  I.e. proxies that would just
> invoke C-level function if no override is detected in runtime, bypassing all
> the type conversion and all that stuff.

However performance won't be great even with light proxies.  At the very minimum the light proxy has to:

  1. Acquire the Python GIL;
  2. dict lookup, or attribute lookup;
  3. Release Python GIL;

If you do this for lots of interface/virtual methods then you will probably notice some performance degradation.
Comment 7 Paul Pogonyshev 2009-01-14 18:57:07 UTC
I committed the patch with the obvious change of removing the comment, since the requested clarification is already there.  The patch is only code cleanup and doesn't address this bug.
Comment 8 Matthew Barnes 2009-04-22 17:56:54 UTC
I have a Fedora crasher that I traced back to the patch here.
See: http://bugzilla.redhat.com/show_bug.cgi?id=492737

Problem seems to be an invalid type cast in pyg_type_add_interfaces():

    for (i = 0; i < PyTuple_GET_SIZE(bases); ++i) {
        ...
        PyTypeObject *base = (PyTypeObject *) PyTuple_GET_ITEM(bases, i);

Code assumes the "bases" tuple is all PyTypeObjects, but in my case one of the bases is a PyClassObject.  This then leads to a crash in:

    PyType_IsSubtype(base, &PyGInterface_Type)

The function tries to access the "tp_mro" pointer, which is garbage.

Raising the severity since it's currently a Fedora 11 blocker.
Comment 9 Paul Pogonyshev 2009-04-22 18:17:58 UTC
Created attachment 133131 [details]
potential fix

This is a case when the old code used to work for a wrong reason: it just tested 'tp_base' instead of calling PyType_IsSubtype().

Anyway, please test if this patch solves the crash.
Comment 10 Matthew Barnes 2009-04-22 21:31:43 UTC
If I'm reading this correctly it effectively drops support for GObject interfaces written as classic-style Python classes, since you're now just skipping over them.  It may well avoid a crash, but won't it still break some modules out there?
Comment 11 Paul Pogonyshev 2009-04-23 16:55:42 UTC
Not really.  There were no support for classic classes before either.  Besides, if I'm not mistaken, classic classes don't have interitance concept at all, they cannot be subclasses of each other.  No?
Comment 12 Matthew Barnes 2009-04-23 17:51:56 UTC
I'm rusty on Python semantics, so I'll trust that you're correct.  Just wanted to double check.
Comment 13 John Ehresman 2009-04-23 17:56:47 UTC
I don't think old-style / classic subclasses of pygtk / pyobject types were ever supported in pygtk 2.x, though I haven't read through the whole bug and may be confused.  If you inherit from a pygtk / pygobject type, you get a new-style class (which aren't so new anymore).  How would you use an old style class to implement an interface?

FWIW, old-style classes have inheritance.
Comment 14 Paul Pogonyshev 2009-04-23 20:04:18 UTC
Yes, they are not supported as far as I can tell, but you can still inherit from multiple bases, some of which are classic classes.  And this causes the crash.

Anyway, Matthew, can you verify that the patch fixes your problem?
Comment 15 John Ehresman 2009-04-23 20:16:52 UTC
It would need to be an old-style mixin then, something like:

class Impl: # Old-style
  def do_method(self):
    pass

class Derived(gtk.SomeType, Impl): # New-style since one base is new-style
   ...

Pretty obscure -- though I may have code that does this, but I don't assume it can run with any version of pygtk.
Comment 16 Paul Pogonyshev 2009-04-23 20:25:36 UTC
The classic class doesn't even need to do anything.  As long as it is mentioned in the list of bases, you get a crash.  And anyway, it was never supported before the cleanup patch either, just before it used to not give a crash, but for a "wrong reason" as it used a somewhat relaxed non-crashing test:

    if (((PyTypeObject *) base)->tp_base != &PyGInterface_Type)
        continue;

Note that it still used an invalid typecast, that's why I say it was working for a "wrong reason".
Comment 17 John Ehresman 2009-04-23 20:30:44 UTC
I see -- the patch fixes the crash but does not add support for old-style class bases.  Fine with me.
Comment 18 Paul Pogonyshev 2009-04-27 17:10:44 UTC
Ping.

Can I push this patch into the master branch as fixing the bug?
Comment 19 Paul Pogonyshev 2009-05-03 13:12:21 UTC
Apparently I already committed the patch.  Git still confuses me.
Comment 20 Emilio Pozuelo Monfort 2009-08-14 12:48:45 UTC
(In reply to comment #19)
> Apparently I already committed the patch.  Git still confuses me.

Why isn't this bug closed then?
Comment 21 Paul Pogonyshev 2009-08-14 15:52:32 UTC
Because the patch was not for the original bug.  That issue is not fixed.
Comment 22 Martin Pitt 2012-04-04 13:16:20 UTC
I confirm that the test case is still valid for current master. However, as we do not have a patch for the original problem, I change the status of the two attachments to get this off the patch review queue.

Thanks!
Comment 23 Simon Feltman 2013-09-20 19:40:37 UTC
I verified that do_parser_finished is called with recent PyGObject. However, the type returned from builder.get_object is still Gtk.Dialog and not an instance of CustomDialog, so there seems to be another problem here.
Comment 24 Simon Feltman 2014-05-30 04:28:14 UTC
*** Bug 730740 has been marked as a duplicate of this bug. ***
Comment 25 Simon Feltman 2014-05-30 04:31:53 UTC
As seen in bug 730740, if you don't re-inherit from the interface you end up clobbering the base classes implementation *globally*
Comment 26 Simon Feltman 2014-08-23 05:22:43 UTC
*** Bug 703338 has been marked as a duplicate of this bug. ***
Comment 27 GNOME Infrastructure Team 2018-01-10 19:59:00 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/3.