GNOME Bugzilla – Bug 566571
Overriding base class interface methods requires explicitly re-inheriting from the interface
Last modified: 2018-01-10 19:59:00 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>
This is a fault of PyGObject, both runtime and codegen. Details follow.
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.
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.
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.
(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.
(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.
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.
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.
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.
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?
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?
I'm rusty on Python semantics, so I'll trust that you're correct. Just wanted to double check.
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.
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?
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.
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".
I see -- the patch fixes the crash but does not add support for old-style class bases. Fine with me.
Ping. Can I push this patch into the master branch as fixing the bug?
Apparently I already committed the patch. Git still confuses me.
(In reply to comment #19) > Apparently I already committed the patch. Git still confuses me. Why isn't this bug closed then?
Because the patch was not for the original bug. That issue is not fixed.
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!
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.
*** Bug 730740 has been marked as a duplicate of this bug. ***
As seen in bug 730740, if you don't re-inherit from the interface you end up clobbering the base classes implementation *globally*
*** Bug 703338 has been marked as a duplicate of this bug. ***
-- 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.