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 785091 - Properties no longer recognized when shadowed by a method
Properties no longer recognized when shadowed by a method
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.49.x
Other All
: Normal critical
: ---
Assigned To: Philip Chimento
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2017-07-19 01:17 UTC by Florian Müllner
Modified: 2017-07-29 08:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
regress: Test for property and method with same name (4.98 KB, patch)
2017-07-20 06:47 UTC, Philip Chimento
none Details | Review
object: Don't let a method shadow a property (5.70 KB, patch)
2017-07-20 06:54 UTC, Philip Chimento
none Details | Review
regress: Test for property and method with conflicting names (17.25 KB, patch)
2017-07-20 08:48 UTC, Rico Tzschichholz
none Details | Review
object: Don't let a method shadow a property (5.65 KB, patch)
2017-07-21 05:11 UTC, Philip Chimento
committed Details | Review
regress: Test for property and method with conflicting names (17.90 KB, patch)
2017-07-21 05:44 UTC, Rico Tzschichholz
committed Details | Review

Description Florian Müllner 2017-07-19 01:17:03 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.
Comment 1 Philip Chimento 2017-07-20 05:42:20 UTC
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.
Comment 2 Philip Chimento 2017-07-20 06:47:01 UTC
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.
Comment 3 Philip Chimento 2017-07-20 06:54:22 UTC
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.
Comment 4 Philip Chimento 2017-07-20 06:56:58 UTC
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.
Comment 5 Rico Tzschichholz 2017-07-20 08:48:40 UTC
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.
Comment 6 Philip Chimento 2017-07-21 05:10:34 UTC
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.
Comment 7 Philip Chimento 2017-07-21 05:11:23 UTC
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.
Comment 8 Rico Tzschichholz 2017-07-21 05:44:54 UTC
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.
Comment 9 Philip Chimento 2017-07-21 06:08:39 UTC
Review of attachment 356082 [details] [review]:

This works great for me!
Comment 10 Rico Tzschichholz 2017-07-21 06:19:06 UTC
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
Comment 11 Cosimo Cecchi 2017-07-28 15:27:57 UTC
Review of attachment 356081 [details] [review]:

Looks good to me.
Comment 12 Philip Chimento 2017-07-29 08:52:13 UTC
Attachment 356081 [details] pushed as 40b7b17 - object: Don't let a method shadow a property