GNOME Bugzilla – Bug 785091
Properties no longer recognized when shadowed by a method
Last modified: 2017-07-29 08:52:18 UTC
ClutterActor has both a has_pointer() method and a :has-pointer property - using 'actor.has_pointer' to access the property used to work, but after the last update now resolves to the method.
By bisection I determined this change happened in the changeover from mozjs38 to mozjs45. I did kind of suspect that, since they rewrote property accesses quite heavily in between those two versions. Here's the difference in the test log with -DGJS_VERBOSE_ENABLE_PROPS=1: GOOD: JS G OBJ: Resolve prop 'overloaded_name' hook obj 0x10e359b40 priv 0x7f975081d5c0 (Regress.BadlyBehaved) gobj 0x7f974f60b060 RegressBadlyBehaved JS G OBJ: Resolve prop 'overloaded_name' hook obj 0x10e370ee0 priv 0x7f975081d520 (Regress.BadlyBehaved) gobj 0x0 (type unknown) JS G OBJ: Defining method overloaded_name in prototype for RegressBadlyBehaved (Regress.BadlyBehaved) JS G OBJ: Get prop 'overloaded_name' hook obj 0x10e359b40 priv 0x7f975081d5c0 JS G OBJ: Overriding overloaded_name with GObject prop overloaded-name JS G OBJ: Resolve prop 'overloaded_name' hook obj 0x10e359b40 priv 0x7f975081d5c0 (Regress.BadlyBehaved) gobj 0x7f974f60b060 RegressBadlyBehaved JS G OBJ: Get prop 'overloaded_name' hook obj 0x10e359b40 priv 0x7f975081d5c0 JS G OBJ: Overriding overloaded_name with GObject prop overloaded-name ok 24 Introspected GObject resolves properties when they are shadowed by methods BAD: JS G OBJ: Resolve prop 'overloaded_name' hook obj 0x10928cb00 priv 0x7f83a50f85c0 (Regress.BadlyBehaved) gobj 0x7f83a4f15a20 RegressBadlyBehaved JS G OBJ: Resolve prop 'overloaded_name' hook obj 0x1092a4e20 priv 0x7f83a50f8520 (Regress.BadlyBehaved) gobj 0x0 (type unknown) JS G OBJ: Defining method overloaded_name in prototype for RegressBadlyBehaved (Regress.BadlyBehaved) JS G OBJ: Get prop 'overloaded_name' hook obj 0x1092a4e20 priv 0x7f83a50f8520 JS G OBJ: Resolve prop 'overloaded_name' hook obj 0x10928cb00 priv 0x7f83a50f85c0 (Regress.BadlyBehaved) gobj 0x7f83a4f15a20 RegressBadlyBehaved JS G OBJ: Get prop 'overloaded_name' hook obj 0x1092a4e20 priv 0x7f83a50f8520 not ok 24 Introspected GObject resolves properties when they are shadowed by methods # Message: Expected Function to equal 42. # Stack: # @installed-tests/js/testEverythingEncapsulated.js:240:9 # Message: Expected true to be falsy. # Stack: # @installed-tests/js/testEverythingEncapsulated.js:241:9 It looks like previously, the getProperty() hook was called on every property access, even if there was already a property defined. Now, apparently, if a property is already defined in JS then the getProperty() hook isn't called.
Created attachment 356009 [details] [review] regress: Test for property and method with same name This is something that libraries are not supposed to do, but some do anyway (Soup and Clutter are two examples) and language bindings should handle it somehow or other. In GJS we want to make sure that the way it's handled doesn't change inadvertently, because buggy library code should not break existing user code.
Created attachment 356010 [details] [review] object: Don't let a method shadow a property This changed due to a change in property access code in SpiderMonkey 45. Previously, the getProperty hook was apparently called for every property access. Now, it's never called if the resolve hook defines a lazy property first. This is unfortunate; we may still change this behaviour (see https://bugzilla.gnome.org/show_bug.cgi?id=690450#c30), but if we do change it then we want it to be intentional.
Rico, the first patch is for gobject-introspection; if you agree with the patch, would it be possible to release a 1.53.4 soon with this change in it? I'd like to release another unstable version of GJS as soon as possible to fix this.
Created attachment 356013 [details] [review] regress: Test for property and method with conflicting names This is something that libraries are not supposed to do, but some do anyway (Soup and Clutter are two examples) and language bindings should handle it somehow or other. In GJS we want to make sure that the way it's handled doesn't change inadvertently, because buggy library code should not break existing user code.
Review of attachment 356013 [details] [review]: This works just as well for me, except for the missing G_PARAM_CONSTRUCT. I'll reattach an updated GJS patch to match it. ::: tests/scanner/regress.c @@ +2683,3 @@ + G_MAXINT, + 42, + "name-conflict property", This should have G_PARAM_CONSTRUCT if it's going to have the default value set by default.
Created attachment 356081 [details] [review] object: Don't let a method shadow a property This changed due to a change in property access code in SpiderMonkey 45. Previously, the getProperty hook was apparently called for every property access. Now, it's never called if the resolve hook defines a lazy property first. This is unfortunate; we may still change this behaviour (see https://bugzilla.gnome.org/show_bug.cgi?id=690450#c30), but if we do change it then we want it to be intentional.
Created attachment 356082 [details] [review] regress: Test for property and method with conflicting names This is something that libraries are not supposed to do, but some do anyway (Soup and Clutter are two examples) and language bindings should handle it somehow or other. In GJS we want to make sure that the way it's handled doesn't change inadvertently, because buggy library code should not break existing user code.
Review of attachment 356082 [details] [review]: This works great for me!
Comment on attachment 356082 [details] [review] regress: Test for property and method with conflicting names Attachment 356082 [details] pushed as d48fb32 - regress: Test for property and method with conflicting names
Review of attachment 356081 [details] [review]: Looks good to me.
Attachment 356081 [details] pushed as 40b7b17 - object: Don't let a method shadow a property