webassembly/objjsproxy: Fix binding of self to JavaScript methods.

Fixes a bug in the binding of self/this to JavaScript methods.

The new semantics match Pyodide's behaviour, at least for the included
tests.

Signed-off-by: Damien George <damien@micropython.org>

ports/webassembly/modjs.c

@@ -34,9 +34,7 @@
 // js module
 
 void mp_module_js_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
-    mp_obj_jsproxy_t global_this;
+    mp_obj_jsproxy_global_this_attr(attr, dest);
-    global_this.ref = MP_OBJ_JSPROXY_REF_GLOBAL_THIS;
-    mp_obj_jsproxy_attr(MP_OBJ_FROM_PTR(&global_this), attr, dest);
 }
 
 static const mp_rom_map_elem_t mp_module_js_globals_table[] = {

ports/webassembly/objjsproxy.c

@@ -43,7 +43,7 @@ EM_JS(bool, has_attr, (int jsref, const char *str), {
 });
 
 // *FORMAT-OFF*
-EM_JS(bool, lookup_attr, (int jsref, const char *str, uint32_t * out), {
+EM_JS(int, lookup_attr, (int jsref, const char *str, uint32_t * out), {
     const base = proxy_js_ref[jsref];
     const attr = UTF8ToString(str);
 
@@ -54,23 +54,17 @@ EM_JS(bool, lookup_attr, (int jsref, const char *str, uint32_t * out), {
     // - Otherwise, the attribute does not exist.
     let value = base[attr];
     if (value !== undefined || attr in base) {
-        if (typeof value === "function") {
-            if (base !== globalThis) {
-                if ("_ref" in value) {
-                    // This is a proxy of a Python function, it doesn't need
-                    // binding.  And not binding it means if it's passed back
-                    // to Python then it can be extracted from the proxy as a
-                    // true Python function.
-                } else {
-                    // A function that is not a Python function.  Bind it.
-                    value = value.bind(base);
-                }
-            }
-        }
         proxy_convert_js_to_mp_obj_jsside(value, out);
-        return true;
+        if (typeof value === "function" && !("_ref" in value)) {
+            // Attribute found and it's a JavaScript function.
+            return 2;
+        } else {
+            // Attribute found.
+            return 1;
+        }
     } else {
-        return false;
+        // Attribute not found.
+        return 0;
     }
 });
 // *FORMAT-ON*
@@ -98,33 +92,48 @@ EM_JS(void, call0, (int f_ref, uint32_t * out), {
     proxy_convert_js_to_mp_obj_jsside(ret, out);
 });
 
-EM_JS(int, call1, (int f_ref, uint32_t * a0, uint32_t * out), {
+EM_JS(int, call1, (int f_ref, bool via_call, uint32_t * a0, uint32_t * out), {
     const a0_js = proxy_convert_mp_to_js_obj_jsside(a0);
     const f = proxy_js_ref[f_ref];
-    const ret = f(a0_js);
+    let ret;
+    if (via_call) {
+        ret = f.call(a0_js);
+    } else {
+        ret = f(a0_js);
+    }
     proxy_convert_js_to_mp_obj_jsside(ret, out);
 });
 
-EM_JS(int, call2, (int f_ref, uint32_t * a0, uint32_t * a1, uint32_t * out), {
+EM_JS(int, call2, (int f_ref, bool via_call, uint32_t * a0, uint32_t * a1, uint32_t * out), {
     const a0_js = proxy_convert_mp_to_js_obj_jsside(a0);
     const a1_js = proxy_convert_mp_to_js_obj_jsside(a1);
     const f = proxy_js_ref[f_ref];
-    const ret = f(a0_js, a1_js);
+    let ret;
+    if (via_call) {
+        ret = f.call(a0_js, a1_js);
+    } else {
+        ret = f(a0_js, a1_js);
+    }
     proxy_convert_js_to_mp_obj_jsside(ret, out);
 });
 
-EM_JS(int, calln, (int f_ref, uint32_t n_args, uint32_t * value, uint32_t * out), {
+EM_JS(int, calln, (int f_ref, bool via_call, uint32_t n_args, uint32_t * value, uint32_t * out), {
     const f = proxy_js_ref[f_ref];
     const a = [];
     for (let i = 0; i < n_args; ++i) {
         const v = proxy_convert_mp_to_js_obj_jsside(value + i * 3 * 4);
         a.push(v);
     }
-    const ret = f(... a);
+    let ret;
+    if (via_call) {
+        ret = f.call(... a);
+    } else {
+        ret = f(... a);
+    }
     proxy_convert_js_to_mp_obj_jsside(ret, out);
 });
 
-EM_JS(void, call0_kwarg, (int f_ref, uint32_t n_kw, uint32_t * key, uint32_t * value, uint32_t * out), {
+EM_JS(void, call0_kwarg, (int f_ref, bool via_call, uint32_t n_kw, uint32_t * key, uint32_t * value, uint32_t * out), {
     const f = proxy_js_ref[f_ref];
     const a = {};
     for (let i = 0; i < n_kw; ++i) {
@@ -132,11 +141,16 @@ EM_JS(void, call0_kwarg, (int f_ref, uint32_t n_kw, uint32_t * key, uint32_t * v
         const v = proxy_convert_mp_to_js_obj_jsside(value + i * 3 * 4);
         a[k] = v;
     }
-    const ret = f(a);
+    let ret;
+    if (via_call) {
+        ret = f.call(a);
+    } else {
+        ret = f(a);
+    }
     proxy_convert_js_to_mp_obj_jsside(ret, out);
 });
 
-EM_JS(void, call1_kwarg, (int f_ref, uint32_t * arg0, uint32_t n_kw, uint32_t * key, uint32_t * value, uint32_t * out), {
+EM_JS(void, call1_kwarg, (int f_ref, bool via_call, uint32_t * arg0, uint32_t n_kw, uint32_t * key, uint32_t * value, uint32_t * out), {
     const f = proxy_js_ref[f_ref];
     const a0 = proxy_convert_mp_to_js_obj_jsside(arg0);
     const a = {};
@@ -145,7 +159,12 @@ EM_JS(void, call1_kwarg, (int f_ref, uint32_t * arg0, uint32_t n_kw, uint32_t *
         const v = proxy_convert_mp_to_js_obj_jsside(value + i * 3 * 4);
         a[k] = v;
     }
-    const ret = f(a0, a);
+    let ret;
+    if (via_call) {
+        ret = f.call(a0, a);
+    } else {
+        ret = f(a0, a);
+    }
     proxy_convert_js_to_mp_obj_jsside(ret, out);
 });
 
@@ -208,12 +227,12 @@ static mp_obj_t jsproxy_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const
         }
         uint32_t out[3];
         if (n_args == 0) {
-            call0_kwarg(self->ref, n_kw, key, value, out);
+            call0_kwarg(self->ref, self->bind_to_self, n_kw, key, value, out);
         } else {
             // n_args == 1
             uint32_t arg0[PVN];
             proxy_convert_mp_to_js_obj_cside(args[0], arg0);
-            call1_kwarg(self->ref, arg0, n_kw, key, value, out);
+            call1_kwarg(self->ref, self->bind_to_self, arg0, n_kw, key, value, out);
         }
         return proxy_convert_js_to_mp_obj_cside(out);
     }
@@ -226,7 +245,7 @@ static mp_obj_t jsproxy_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const
         uint32_t arg0[PVN];
         uint32_t out[PVN];
         proxy_convert_mp_to_js_obj_cside(args[0], arg0);
-        call1(self->ref, arg0, out);
+        call1(self->ref, self->bind_to_self, arg0, out);
         return proxy_convert_js_to_mp_obj_cside(out);
     } else if (n_args == 2) {
         uint32_t arg0[PVN];
@@ -234,7 +253,7 @@ static mp_obj_t jsproxy_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const
         uint32_t arg1[PVN];
         proxy_convert_mp_to_js_obj_cside(args[1], arg1);
         uint32_t out[3];
-        call2(self->ref, arg0, arg1, out);
+        call2(self->ref, self->bind_to_self, arg0, arg1, out);
         return proxy_convert_js_to_mp_obj_cside(out);
     } else {
         uint32_t value[PVN * n_args];
@@ -242,7 +261,7 @@ static mp_obj_t jsproxy_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const
             proxy_convert_mp_to_js_obj_cside(args[i], &value[i * PVN]);
         }
         uint32_t out[3];
-        calln(self->ref, n_args, value, out);
+        calln(self->ref, self->bind_to_self, n_args, value, out);
         return proxy_convert_js_to_mp_obj_cside(out);
     }
 }
@@ -298,17 +317,26 @@ static mp_obj_t jsproxy_subscr(mp_obj_t self_in, mp_obj_t index, mp_obj_t value)
     }
 }
 
-void mp_obj_jsproxy_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
+static void mp_obj_jsproxy_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
     mp_obj_jsproxy_t *self = MP_OBJ_TO_PTR(self_in);
     if (dest[0] == MP_OBJ_NULL) {
         // Load attribute.
+        int lookup_ret;
         uint32_t out[PVN];
         if (attr == MP_QSTR___del__) {
             // For finaliser.
             dest[0] = MP_OBJ_FROM_PTR(&jsproxy___del___obj);
             dest[1] = self_in;
-        } else if (lookup_attr(self->ref, qstr_str(attr), out)) {
+        } else if ((lookup_ret = lookup_attr(self->ref, qstr_str(attr), out)) != 0) {
             dest[0] = proxy_convert_js_to_mp_obj_cside(out);
+            if (lookup_ret == 2) {
+                // The loaded attribute is a JavaScript method, which should be called
+                // with f.call(self, ...).  Indicate this via the bind_to_self member.
+                // This will either be called immediately (due to the mp_load_method
+                // optimisation) or turned into a bound_method and called later.
+                dest[1] = self_in;
+                ((mp_obj_jsproxy_t *)dest[0])->bind_to_self = true;
+            }
         } else if (attr == MP_QSTR_new) {
             // Special case to handle construction of JS objects.
             // JS objects don't have a ".new" attribute, doing "Obj.new" is a Pyodide idiom for "new Obj".
@@ -546,5 +574,25 @@ MP_DEFINE_CONST_OBJ_TYPE(
 mp_obj_t mp_obj_new_jsproxy(int ref) {
     mp_obj_jsproxy_t *o = mp_obj_malloc_with_finaliser(mp_obj_jsproxy_t, &mp_type_jsproxy);
     o->ref = ref;
+    o->bind_to_self = false;
     return MP_OBJ_FROM_PTR(o);
 }
+
+// Load/delete/store an attribute from/to the JavaScript globalThis entity.
+void mp_obj_jsproxy_global_this_attr(qstr attr, mp_obj_t *dest) {
+    if (dest[0] == MP_OBJ_NULL) {
+        // Load attribute.
+        uint32_t out[PVN];
+        if (lookup_attr(MP_OBJ_JSPROXY_REF_GLOBAL_THIS, qstr_str(attr), out)) {
+            dest[0] = proxy_convert_js_to_mp_obj_cside(out);
+        }
+    } else if (dest[1] == MP_OBJ_NULL) {
+        // Delete attribute.
+    } else {
+        // Store attribute.
+        uint32_t value[PVN];
+        proxy_convert_mp_to_js_obj_cside(dest[1], value);
+        store_attr(MP_OBJ_JSPROXY_REF_GLOBAL_THIS, qstr_str(attr), value);
+        dest[0] = MP_OBJ_NULL;
+    }
+}

ports/webassembly/proxy_c.h

@@ -38,6 +38,7 @@
 typedef struct _mp_obj_jsproxy_t {
     mp_obj_base_t base;
     int ref;
+    bool bind_to_self;
 } mp_obj_jsproxy_t;
 
 extern const mp_obj_type_t mp_type_jsproxy;
@@ -52,7 +53,7 @@ void proxy_convert_mp_to_js_obj_cside(mp_obj_t obj, uint32_t *out);
 void proxy_convert_mp_to_js_exc_cside(void *exc, uint32_t *out);
 
 mp_obj_t mp_obj_new_jsproxy(int ref);
-void mp_obj_jsproxy_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest);
+void mp_obj_jsproxy_global_this_attr(qstr attr, mp_obj_t *dest);
 
 static inline bool mp_obj_is_jsproxy(mp_obj_t o) {
     return mp_obj_get_type(o) == &mp_type_jsproxy;

tests/ports/webassembly/method_bind_behaviour.mjs

@@ -0,0 +1,43 @@
+// Test how JavaScript binds self/this when methods are called from Python.
+
+const mp = await (await import(process.argv[2])).loadMicroPython();
+
+// Test accessing and calling JavaScript methods from Python.
+mp.runPython(`
+import js
+
+# Get the push method to call later on.
+push = js.Array.prototype.push
+
+# Create initial array.
+ar = js.Array(1, 2)
+js.console.log(ar)
+
+# Add an element using a method (should implicitly supply "ar" as context).
+print(ar.push(3))
+js.console.log(ar)
+
+# Add an element using prototype function, need to explicitly provide "ar" as context.
+print(push.call(ar, 4))
+js.console.log(ar)
+
+# Add an element using a method with call and explicit context.
+print(ar.push.call(ar, 5))
+js.console.log(ar)
+
+# Add an element using a different instances method with call and explicit context.
+print(js.Array().push.call(ar, 6))
+js.console.log(ar)
+`);
+
+// Test assigning Python functions to JavaScript objects, and using them like a method.
+mp.runPython(`
+import js
+
+a = js.Object()
+a.meth1 = lambda *x: print("meth1", x)
+a.meth1(1, 2)
+
+js.Object.prototype.meth2 = lambda *x: print("meth2", x)
+a.meth2(3, 4)
+`);

tests/ports/webassembly/method_bind_behaviour.mjs.exp

@@ -0,0 +1,11 @@
+[ 1, 2 ]
+3
+[ 1, 2, 3 ]
+4
+[ 1, 2, 3, 4 ]
+5
+[ 1, 2, 3, 4, 5 ]
+6
+[ 1, 2, 3, 4, 5, 6 ]
+meth1 (1, 2)
+meth2 (3, 4)