GNOME Bugzilla – Bug 666616
gobject: fix property override type checks
Last modified: 2011-12-21 01:05:55 UTC
There is currently broken type-checking code for ensuring that property overrides on interface implementations: /* The implementation paramspec must have a less restrictive * type than the interface parameter spec for set() and a * more restrictive type for get(). We just require equality, * rather than doing something more complicated checking * the READABLE and WRITABLE flags. We also simplify here * by only checking the value type, not the G_PARAM_SPEC_TYPE. */ if (g_type_is_a (pspecs[n]->value_type, class_pspec->value_type)) g_critical ("Property '%s' on class '%s' has type '%s' " "which is different from the type '%s', " "of the property on interface '%s'\n", pspecs[n]->name, g_type_name (G_OBJECT_CLASS_TYPE (class)), g_type_name (G_PARAM_SPEC_VALUE_TYPE (class_pspec)), g_type_name (G_PARAM_SPEC_VALUE_TYPE (pspecs[n])), g_type_name (iface_type)); From the comment it's clear that the intention was to do a direct comparison of the types, but instead, g_type_is_a() slipped in somehow. We could fix this somewhat easily by changing it to a direct comparison but this could break existing (valid) code that has come to rely on the relative permissiveness of the is-a check. Instead, I'm going to try to implement the logic originally suggested in the comment. That way we'll only break people's code if it's actually broken...
Created attachment 203997 [details] [review] GObject: fix property override type checks The property override typecheck was meant to enforce the type on the overriding property being exactly equal to the type on the interface property. Instead, g_type_is_a() was incorrectly used. We could try to enforce equality, but if a property is read-only then it should be possible for the implementation to type the property with any subtype of the type specified on the interface (because returning a more specific type will still satisfy the interface). Likewise, if the property is write-only then it should be possible for the implementation to type the property with any supertype. We implement the check this way.
Created attachment 203998 [details] [review] GObject: change the order of property checks Change the order of the checks so that we hear about the 'biggest' problem first. Also, stop reporting problems after we report the first one for a particular property. Add some comments.
Created attachment 203999 [details] [review] GObject: add test for interface property overrides Add a testcase to check all possibilities for overriding a property specified on an interface from an implementation of that interface, changing the type and flags.
There are 4 things that I uncovered while writing this test that we should consider addressing: - g_object_class_install_property allows you to install properties that are neither readable nor writable - g_object_interface_install_property does no checks at all on the properties you install with it - pspec_pool is initialised from g_object_do_class_init but used as a result of g_object_interface_install_property which would happen before GObjectClass is initialised - the treatment of G_PARAM_CONSTRUCT is somewhat suspect. I wouldn't classify this as a 'restriction' so much as it is a preference left up to the implementation to decide. After construction, it makes no difference at all...
Review of attachment 203998 [details] [review]: Looks good, except: ::: gobject/gobject.c @@ +1414,3 @@ + if (pspecs[n]->flags & G_PARAM_WRITABLE) + { + if (!SUBSET (class_pspec->flags, pspecs[n]->flags, G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY)) Checking G_PARAM_CONSTRUCT here doesn't make sense to me - G_PARAM_COSNTRUCT is an implementation level construct not an interface level construct - it means basically "call my constructor with the default value if another value isn't specified by the caller"
Review of attachment 203999 [details] [review]: Going to trust you on this one
Review of attachment 203998 [details] [review]: ::: gobject/gobject.c @@ +1414,3 @@ + if (pspecs[n]->flags & G_PARAM_WRITABLE) + { + if (!SUBSET (class_pspec->flags, pspecs[n]->flags, G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY)) I agree with this (see comment 4 for my remarks on the topic). Since this code is just being moved in this patch, I'll add another patch to make that change.
Review of attachment 203997 [details] [review]: Logic here looks mostly right to me - I think the messages are a little cryptic, but less so than many messages in GObject. ::: gobject/gobject.c @@ +1410,3 @@ + g_type_name (G_OBJECT_CLASS_TYPE (class)), g_type_name (G_PARAM_SPEC_VALUE_TYPE (class_pspec)), + g_type_name (G_PARAM_SPEC_VALUE_TYPE (pspecs[n])), g_type_name (iface_type)); + break; shouldn't this continue if it g_critical's?
Review of attachment 203997 [details] [review]: ::: gobject/gobject.c @@ +1410,3 @@ + g_type_name (G_OBJECT_CLASS_TYPE (class)), g_type_name (G_PARAM_SPEC_VALUE_TYPE (class_pspec)), + g_type_name (G_PARAM_SPEC_VALUE_TYPE (pspecs[n])), g_type_name (iface_type)); + break; we're inside of a switch here. this is just leaving the switch.
(In reply to comment #4) > There are 4 things that I uncovered while writing this test that we should > consider addressing: > > - g_object_class_install_property allows you to install properties > that are neither readable nor writable foot, gun, pow, not our problem > - g_object_interface_install_property does no checks at all on the > properties you install with it I suppose adding checks similar to what we do for classes where applicable would make sense, but interface properties aren't especially common > - pspec_pool is initialised from g_object_do_class_init but used as a > result of g_object_interface_install_property which would happen before > GObjectClass is initialised sounds like a bug, but not one most people will hit outside of test cases > - the treatment of G_PARAM_CONSTRUCT is somewhat suspect. I wouldn't > classify this as a 'restriction' so much as it is a preference left up > to the implementation to decide. After construction, it makes no > difference at all... addressed in review
(In reply to comment #9) > Review of attachment 203997 [details] [review]: > > ::: gobject/gobject.c > @@ +1410,3 @@ > + g_type_name (G_OBJECT_CLASS_TYPE (class)), g_type_name > (G_PARAM_SPEC_VALUE_TYPE (class_pspec)), > + g_type_name (G_PARAM_SPEC_VALUE_TYPE (pspecs[n])), > g_type_name (iface_type)); > + break; > > we're inside of a switch here. this is just leaving the switch. Ah, ok, so this ends up the last thing in the loop and doesn't need the continue. I see.
Created attachment 204003 [details] [review] GObject: allow G_PARAM_CONSTRUCT on any override We were previously preventing implementations of an interface from specifying G_PARAM_CONSTRUCT for a property of that interface if the interface didn't specify it itself (or was readonly). This is something that should only interest the implementation, so we remove this restriction. This allows 6 new possible override scenarios: - writable -> writable/construct - writable -> readwrite/construct - readwrite -> readwrite/construct - writable/construct-only -> writable/construct - writable/construct-only -> readwrite/construct - readwrite/construct-only -> readwrite/construct and we update the testcase to reflect this.
Created attachment 204004 [details] [review] GObject: require READ or WRITE on property install g_object_class_install_property() currently lets you install properties that are neither readable nor writable. Add a check to prevent that.
Created attachment 204005 [details] [review] GObject: do checks on interface property install Add some checks to g_object_interface_install_property() similar to those in g_object_class_install_property().
I'll open another bug to deal with the pspec_pool issue since it's not really related to the rest of what's here. Attachment 203997 [details] pushed as 5fb7a8e - GObject: fix property override type checks Attachment 203998 [details] pushed as d8d7868 - GObject: change the order of property checks Attachment 203999 [details] pushed as b3b9f82 - GObject: add test for interface property overrides Attachment 204003 [details] pushed as 4e793c2 - GObject: allow G_PARAM_CONSTRUCT on any override Attachment 204004 [details] pushed as b237187 - GObject: require READ or WRITE on property install Attachment 204005 [details] pushed as 557da16 - GObject: do checks on interface property install
See bug 666625.