GNOME Bugzilla – Bug 646742
fix up constructor handling with interface types
Last modified: 2018-02-08 12:05:37 UTC
Two patches to correctly recognize constructors that return interface types. Note that these may break existing programs that were relying on the fact that, eg, g_file_new_for_path() was exported as Gio.file_new_for_path() before.
Created attachment 185143 [details] [review] scanner: recognize constructors that return interface types Constructors that return interface types (eg, g_themed_icon_new(), which returns a GIcon*) were not being recognized as constructors. Fix.
Created attachment 185144 [details] [review] scanner: allow interfaces to have constructors Some interface types have "factory"-type constructors that return an object of the correct subtype. (Eg, g_icon_new_for_string(), g_file_new_for_path()). Recognize those as constructors. (These might not actually work correctly in bindings without additional work there.)
So I think this is that it's too much of an incompatible change at this point - if we go with just Torsten's patch from bug 572408, we should be 100% compatible. But how bindings and user code would behave if we just moved existing methods to constructors is likely a problem.
But his approach of defining the method both ways could be used here too.
Created attachment 193766 [details] [review] scanner: allow interfaces to have constructors Some interface types have "factory"-type constructors that return an object of the correct subtype. (Eg, g_icon_new_for_string(), g_file_new_for_path()). Recognize those as constructors. (These might not actually work correctly in bindings without additional work there.) Based on a patch by Dan Winship.
Created attachment 193767 [details] [review] scanner: recognize constructors that return interface types Constructors that return interface types (eg, g_themed_icon_new(), which returns a GIcon*) were not being recognized as constructors. Fix.
We need the approach of copying the old definition though, right? Can you attach the analysis file you did of which functions get changed by this?
Here is the same pair of patches with slight modifications. The first one now also updates ast.Interface._walk so that the newly added constructors can the same treatment later on. The only change to the second patch is some rewrapping so as to pass 'make check'. With these applied, I see changes to following classes in the GIR files of glib, atk, pango and gtk+: - GFile (INCOMPTABILE: old functions lost) - GEmblemedIcon (INCOMPATIBLE: function->constructor) - GFileIcon (INCOMPATIBLE: function G.File.icon_new -> constructor G.FileIcon.new) - GIcon (INCOMPATIBLE: old functions lost) - GInitable (COMPATIBLE: old function lost, but is introspectable=0) - GNetworkAddress (INCOMPATIBLE: function->constructor) - GNetworkService (INCOMPATIBLE: function->constructor) - GThemedIcon (INCOMPATIBLE: functions->constructors) - GTlsClientConnection (COMPATIBLE: old function lost, was introspectable=0 before) - GTlsFileDatabase (COMPATIBLE: old function lost, was introspectable=0 before) - GTlsServerConnection (COMPATIBLE: old function lost, was introspectable=0 before) - GtkNumerableIcon (INCOMPATIBLE: functions->constructors) - GtkTreeModelFilter (INCOMPATIBLE: function Gtk.TreeModel.filter_new -> constructor Gtk.TreeModelFilter.new) - GtkTreeModelSort (INCOMPATIBLE: function Gtk.TreeModel.sort_new_with_model -> constructor Gtk.TreeModelSort.new_with_model) - PangoCairoFontMap (INCOMPATIBLE: old functions lost)
Excellent analysis - ok so let's just focus on the incompatible changes. Some of these we may be able to just change like PangoCairoFontMap. Others I'm a bit more worried about changing - like GtkTreeModelFilter I'm pretty sure has existing Python overrides. There are probably not many scripted users of GNetworkAddress or GtkNumerableIcon yet - we could likely just change those. So...options: 1) Take the patch as is, and ensure that the python bindings are updated as necessary. Accept that we break any users of Gtk.TreeModel.filter_new() and Gio.File.icon_new() 2) Try to only move the compatible changes - this may be hard 3) Leave the tree as is Anything I'm missing?
These look good. I say we should fix them sooner than later. A couple of the more used ones may cause pain but they are all clearly currently wrong and we should not encourage use of broken API.
Johan has now also said that he would be fine with option 1). As am I.
From a Vala point of view, we have File.new_for_path which is a static method, not a constructor. A constructor is said to construct a File instance which is not the case, instead it's a factory method. I wouldn't abuse constructors, instead let it be moved/copied as in bug 572408 or bug 656499 in the relative interface.
(In reply to comment #12) > From a Vala point of view, we have File.new_for_path which is a static method, > not a constructor. A constructor is said to construct a File instance which is > not the case, instead it's a factory method. The C methods aren't named to sound factory-like though (eg, as opposed to g_socket_connection_factory_create_connection()). And in some cases (eg, g_tls_client_connection_new()), there's only a single type that each "factory" method could possibly return, but they return an interface type because the concrete type is defined by a module. (g_file_new_for_path() is also mostly like that, although it actually returns 2 different types.) Anyway, if some bindings want to call them constructors and some want to call them factories, then it's better to call them constructors in the typelib, because vala can trivially just say "any so-called constructor that returns an interface type should be rewritten as a static method instead", whereas to go in the other direction, bindings would need to include all of giscanner's logic for deciding which static methods are actually constructors.
Created attachment 198020 [details] [review] Alright, here's an updated attempt. The first two patches are basically the same as the earlier ones except for an additional fix that ensure that interface constructors actually end up in the typelib. The third patch implements a backwards compatibility approach (and depends on bug 660698). With this, of the incompatible changes listed in comment 8, only those remain in which a static method is turned into a constructor with the same name. In many languages, this is not actually an incompatible change. What do you think?
Created attachment 198021 [details] [review] scanner: recognize constructors that return interface types
Created attachment 198022 [details] [review] scanner: create backcompat copies for interface constructors
So this missed 3.2 =( But we can try again for 3.4. I'd like to wait a week and fix up other bugs before doing major work on g-i master though.
We missed 3.4 as well. And we're getting late for 3.6.
*** Bug 679981 has been marked as a duplicate of this bug. ***
(In reply to comment #18) > We missed 3.4 as well. And we're getting late for 3.6. Yeah, sorry about forgetting about this, Torsten. My bugzilla experience is mostly trying to keep up with the continual stream. Periodic pings are ok =)
Can we get this reviewed?
Created attachment 222071 [details] [review] scanner: allow interfaces to have constructors Some interface types have "factory"-type constructors that return an object of the correct subtype. (Eg, g_icon_new_for_string(), g_file_new_for_path()). Recognize those as constructors. (These might not actually work correctly in bindings without additional work there.) Based on a patch by Dan Winship.
Created attachment 222072 [details] [review] scanner: recognize constructors that return interface types Constructors that return interface types (eg, g_themed_icon_new(), which returns a GIcon*) were not being recognized as constructors. Fix.
Created attachment 222073 [details] [review] scanner: create backcompat copies for interface constructors
To make reviewing easier, I've attached the rebased commits and I've redone the analysis from comment 8. The only changes that these patches cause in the GIR files of glib, pango, gdk-pixbuf and gtk+ are the following: • In GFile, GIcon, GTlsClientConnection, GTlsFileDatabase, GTlsServerConnection, PangoCairoFontMap and GtkNumerableIcon many class-static functions become constructors (g_file_new_for_path, for example). • GtkTreeModelFilter and GtkTreeModelSort get their constructors mapped correctly, but the old incorrect mappings (as functions Gtk.TreeModel.filter_new and Gtk.TreeModel.sort_new_with_model) are retained. The only compatibility concern is therefore whether changing, say, Gio.File.new_for_path from a class-static function into a constructor breaks things so badly that the correctness improvement is outweighed.
gjs doesn't handle constructors any differently than methods, so I doubt this would break anything on our side. Do you know what some of the other bindings do?
Well, the Perl bindings do (Gio::File->new vs. Gio::File::new; effectively, Perl constructors expect to receive the class name as their first argument). But I (the Perl binding's maintainer) don't care about this kind of breakage at this point. I don't know of many people using gio via Perl and introspection currently, so I find correctness more important.
I suspect that the Vala people may be affected at least. We need to look at things using vapigen. I do want to get this patch in, but it's just a nontrivial patch that affects a lot of hard-to-test things.
(In reply to comment #28) > I suspect that the Vala people may be affected at least. We need to look at > things using vapigen. Yes, Vala is affected. Treating these symbols as constructors in Vala would be a significant change (and will probably never happen, but you'd have to talk to Jürg about that) but it should be pretty straightforward to change vapigen to simply output static methods when it sees a constructor in an interface, meaning there would be no change from the current behavior. Since people using valac don't typically rely on GIR directly this probably wouldn't be a big issue. The only problems would come when someone tries to use a new-style GIR with an old vapigen, so some libraries (those which distribute Vala bindings based on an affected GIR) may need to bump their vapigen dependency in order to avoid a regression.
(In reply to comment #29) > > Since people using valac don't typically rely on GIR directly this probably > wouldn't be a big issue. The only problems would come when someone tries to > use a new-style GIR with an old vapigen, so some libraries (those which > distribute Vala bindings based on an affected GIR) may need to bump their > vapigen dependency in order to avoid a regression. Ok...I'm really leaning towards doing this early in the 3.7 cycle. Sorry Torsten...it is a good change and I am hopeful by next cycle I'll be spending less time on a build system and more time on g-i. Do note though as a general rule I'm happy if patches get review from *someone* - it doesn't have to be me. Anyone should feel free to dive in and review g-i patches if you feel you can.
(In reply to comment #30) > Ok...I'm really leaning towards doing this early in the 3.7 cycle. Sorry > Torsten...it is a good change and I am hopeful by next cycle I'll be spending > less time on a build system and more time on g-i. Right now the GIR has a function inside of the interface, and the proposed patch changes that function to a constructor instead of adding a new constructor and keeping the old function around. I'd like to modify vapigen this cycle to future-proof it against this change, so could you confirm what you want to do?
(In reply to comment #30) > Ok...I'm really leaning towards doing this early in the 3.7 cycle. Sorry > Torsten... No worries; I think this is the sensible decision at this point. I don't depend on these patches for anything. This also gives us a little more time to take the arguments into consideration that have been raised against the general idea of constructors for interfaces. If we decide to implement this, it should be a conscious choice. (In reply to comment #31) > Right now the GIR has a function inside of the interface, and the proposed > patch changes that function to a constructor instead of adding a new > constructor and keeping the old function around. I think changing the functions to be constructors is the only option, if we decide to go ahead. The alternative of having both, a function and a constructor with the same name, would lead to undefined behavior. What would, say, g_object_info_find_method return when called with the common name? Also, in Perl, constructors and class-static functions are both represented as namespaced functions, so there can only be one for a given name, but the calling semantics that users expect are different for the two types of functions.
So we landed https://bugzilla.gnome.org/show_bug.cgi?id=572408 which is related in some cases. For example, at this point now, g_file_new_for_path will have gone from a factory function to a static method and now a constructor? What problem are we fixing here again? The GFile one honestly was pretty ugly from a binding point of view, and in retrospect I'm *really* glad we fixed that (thanks Torsten!) - I use Gio.File.new_for_path constantly in my scripts. But I haven't hit the GtkTreeModelFilter/GtkTreeModelSort ones myself.
I was bumping into g_icon_new_for_string() in gnome-shell when I filed the bug. That was also fixed by 572408 (at least as far as making it a static method rather than a global function. It's still not a constructor.) (And the shell appears to still use the old version.) The bug addressed by the second patch ("recognize constructors that return interface types") appears to have been worked around via (type) annotations in some cases. Eg, g_network_address_new() now has: * Returns: (transfer full) (type GNetworkAddress): the new #GNetworkAddress So, it gets recognized as a constructor rather than a static method. And that could be done elsewhere too, but it really shouldn't be needed, especially since the patch here gives us backcompat entries too...
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
-- 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/gobject-introspection/issues/48.