[gjs] object: Don't let a method shadow a property

[Philip Chimento <pchimento src gnome org>]

commit 40b7b1767dcd5176d3c493cb02e2a3ff1fca7eae
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Jul 19 23:48:16 2017 -0700

    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.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=785091

 configure.ac                                     |    2 +-
 gi/object.cpp                                    |   82 +++++++++++++++++++++-
 installed-tests/js/testEverythingEncapsulated.js |   10 +++
 3 files changed, 90 insertions(+), 4 deletions(-)
---
diff --git a/configure.ac b/configure.ac
index b4bc63a..25903af 100644
--- a/configure.ac
+++ b/configure.ac
@@ -254,7 +254,7 @@ AC_CONFIG_LINKS([
 AC_OUTPUT
 
 # Warn about conditions that affect runtime
-PKG_CHECK_EXISTS([gobject-introspection-1.0 >= 1.53.1], [], [
+PKG_CHECK_EXISTS([gobject-introspection-1.0 >= 1.53.4], [], [
     AC_MSG_WARN([You do not have a new enough version of
 gobject-introspection to run the tests. You can still build GJS, but some
 tests will fail.])])
diff --git a/gi/object.cpp b/gi/object.cpp
index 76f204e..ab19dcd 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -322,6 +322,14 @@ lookup_field_info(GIObjectInfo *info,
         g_clear_pointer(&retval, g_base_info_unref);
     }
 
+    if (!retval)
+        return nullptr;
+
+    if (!(g_field_info_get_flags(retval) & GI_FIELD_IS_READABLE)) {
+        g_base_info_unref(retval);
+        return nullptr;
+    }
+
     return retval;
 }
 
@@ -345,9 +353,6 @@ get_prop_from_field(JSContext             *cx,
     GITypeTag tag;
     GIArgument arg = { 0 };
 
-    if (!(g_field_info_get_flags(field) & GI_FIELD_IS_READABLE))
-        goto out;
-
     gjs_debug_jsprop(GJS_DEBUG_GOBJECT, "Overriding %s with GObject field",
                      name);
 
@@ -650,6 +655,65 @@ object_instance_resolve_no_info(JSContext       *context,
     return true;
 }
 
+/* Taken from GLib */
+static void
+canonicalize_key(char *key)
+{
+    for (char *p = key; *p != 0; p++) {
+        char c = *p;
+
+        if (c != '-' &&
+            (c < '0' || c > '9') &&
+            (c < 'A' || c > 'Z') &&
+            (c < 'a' || c > 'z'))
+            *p = '-';
+    }
+}
+
+static bool
+is_gobject_property_name(GIObjectInfo *info,
+                         const char   *name)
+{
+    int n_props = g_object_info_get_n_properties(info);
+    int ix;
+    GIPropertyInfo *prop_info = nullptr;
+
+    char *canonical_name = gjs_hyphen_from_camel(name);
+    canonicalize_key(canonical_name);
+
+    for (ix = 0; ix < n_props; ix++) {
+        prop_info = g_object_info_get_property(info, ix);
+        const char *prop_name = g_base_info_get_name(prop_info);
+        if (strcmp(canonical_name, prop_name) == 0)
+            break;
+        g_clear_pointer(&prop_info, g_base_info_unref);
+    }
+
+    g_free(canonical_name);
+
+    if (!prop_info)
+        return false;
+
+    if (!(g_property_info_get_flags(prop_info) & G_PARAM_READABLE)) {
+        g_base_info_unref(prop_info);
+        return false;
+    }
+
+    g_base_info_unref(prop_info);
+    return true;
+}
+
+static bool
+is_gobject_field_name(GIObjectInfo *info,
+                      const char   *name)
+{
+    GIFieldInfo *field_info = lookup_field_info(info, name);
+    if (!field_info)
+        return false;
+    g_base_info_unref(field_info);
+    return true;
+}
+
 /*
  * The *objp out parameter, on success, should be null to indicate that id
  * was not resolved; and non-null, referring to obj or one of its prototypes,
@@ -748,6 +812,18 @@ object_instance_resolve(JSContext       *context,
          * method resolution. */
     }
 
+    /* If the name refers to a GObject property or field, don't resolve.
+     * Instead, let the getProperty hook handle fetching the property from
+     * GObject. */
+    if (is_gobject_property_name(priv->info, name) ||
+        is_gobject_field_name(priv->info, name)) {
+        gjs_debug_jsprop(GJS_DEBUG_GOBJECT,
+                         "Breaking out of %p resolve, '%s' is a GObject prop",
+                         obj.get(), name.get());
+        *resolved = false;
+        return true;
+    }
+
     /* find_method does not look at methods on parent classes,
      * we rely on javascript to walk up the __proto__ chain
      * and find those and define them in the right prototype.
diff --git a/installed-tests/js/testEverythingEncapsulated.js 
b/installed-tests/js/testEverythingEncapsulated.js
index 0142160..1039f0e 100644
--- a/installed-tests/js/testEverythingEncapsulated.js
+++ b/installed-tests/js/testEverythingEncapsulated.js
@@ -231,6 +231,16 @@ describe('Introspected GObject', function () {
         obj.ownprop = 'foo';
         expect(obj.hasOwnProperty('ownprop')).toBeTruthy();
     });
+
+    // This test is not meant to be normative; a GObject behaving like this is
+    // doing something unsupported. However, we have been handling this so far
+    // in a certain way, and we don't want to break user code because of badly
+    // behaved libraries. This test ensures that any change to the behaviour
+    // must be intentional.
+    it('resolves properties when they are shadowed by methods', function () {
+        expect(obj.name_conflict).toEqual(42);
+        expect(obj.name_conflict instanceof Function).toBeFalsy();
+    });
 });
 
 describe('Introspected function length', function () {