FIX: Make JsBoundMethod a subclass of JsProxy (pyodide#1124)

jmsmdy · Jan 14, 2021 · 61c56f8 · 61c56f8
1 parent 7c0705c
commit 61c56f8
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 27 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -44,6 +44,8 @@
 - `JsProxy.__bool__` now produces more consistent results: both `bool(window)` and `bool(zero-arg-callback)` were `False` but now are `True`. Conversely, `bool(empty_js_set)` and `bool(empty_js_map)` were `True` but now are `False`. [#1061](https://github.com/iodide-project/pyodide/pull/1061)
 - When calling a javascript function from Python without keyword arguments, Pyodide no longer passes a `PyProxy`-wrapped `NULL` pointer as the last argument.
 [#1033](https://github.com/iodide-project/pyodide/pull/1033)
+- JsBoundMethod is now a subclass of JsProxy, which fixes nested attribute access and various other strange bugs.
+  [#1124](https://github.com/iodide-project/pyodide/pull/1124)
 
 
 ## Version 0.16.1

diff --git a/src/core/hiwire.c b/src/core/hiwire.c
@@ -334,6 +334,16 @@ EM_JS_REF(JsRef, hiwire_call, (JsRef idfunc, JsRef idargs), {
   return Module.hiwire.new_value(jsfunc(... jsargs));
 });
 
+EM_JS_REF(JsRef,
+          hiwire_call_bound,
+          (JsRef idfunc, JsRef idthis, JsRef idargs),
+          {
+            let func = Module.hiwire.get_value(idfunc);
+            let this_ = Module.hiwire.get_value(idthis);
+            let args = Module.hiwire.get_value(idargs);
+            return Module.hiwire.new_value(func.apply(this_, args));
+          });
+
 EM_JS_REF(JsRef,
           hiwire_call_member,
           (JsRef idobj, const char* ptrname, JsRef idargs),
@@ -371,6 +381,12 @@ EM_JS_NUM(bool, hiwire_get_bool, (JsRef idobj), {
   // clang-format on
 });
 
+EM_JS_NUM(bool, hiwire_is_pyproxy, (JsRef idobj), {
+  // clang-format off
+  return Module.PyProxy.isPyProxy(Module.hiwire.get_value(idobj));
+  // clang-format on
+});
+
 EM_JS_NUM(bool, hiwire_is_function, (JsRef idobj), {
   // clang-format off
   return typeof Module.hiwire.get_value(idobj) === 'function';

diff --git a/src/core/hiwire.h b/src/core/hiwire.h
@@ -403,6 +403,9 @@ hiwire_dir(JsRef idobj);
 JsRef
 hiwire_call(JsRef idobj, JsRef idargs);
 
+JsRef
+hiwire_call_bound(JsRef idfunc, JsRef idthis, JsRef idargs);
+
 /**
  * Call a member function.
  *
@@ -441,6 +444,9 @@ hiwire_get_length(JsRef idobj);
 bool
 hiwire_get_bool(JsRef idobj);
 
+bool
+hiwire_is_pyproxy(JsRef idobj);
+
 /**
  * Returns 1 if the object is a function.
  *

diff --git a/src/core/jsproxy.c b/src/core/jsproxy.c
@@ -21,7 +21,7 @@ static PyTypeObject* PyExc_BaseException_Type;
 _Py_IDENTIFIER(__dir__);
 
 static PyObject*
-JsBoundMethod_cnew(JsRef this_, const char* name);
+JsBoundMethod_cnew(JsRef func, JsRef this_);
 
 ////////////////////////////////////////////////////////////
 // JsProxy
@@ -91,8 +91,8 @@ JsProxy_GetAttr(PyObject* self, PyObject* attr)
     FAIL();
   }
 
-  if (hiwire_is_function(idresult)) {
-    pyresult = JsBoundMethod_cnew(JsProxy_REF(self), key);
+  if (!hiwire_is_pyproxy(idresult) && hiwire_is_function(idresult)) {
+    pyresult = JsBoundMethod_cnew(idresult, JsProxy_REF(self));
   } else {
     pyresult = js2python(idresult);
   }
@@ -617,7 +617,8 @@ static PyTypeObject JsProxyType = {
   .tp_setattro = JsProxy_SetAttr,
   .tp_as_async = &JsProxy_asyncMethods,
   .tp_richcompare = JsProxy_RichCompare,
-  .tp_flags = Py_TPFLAGS_DEFAULT | _Py_TPFLAGS_HAVE_VECTORCALL,
+  .tp_flags =
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | _Py_TPFLAGS_HAVE_VECTORCALL,
   .tp_doc = "A proxy to make a Javascript object behave like a Python object",
   .tp_methods = JsProxy_Methods,
   .tp_getset = JsProxy_GetSet,
@@ -629,17 +630,24 @@ static PyTypeObject JsProxyType = {
   .tp_as_buffer = &JsProxy_BufferProcs
 };
 
-PyObject*
-JsProxy_cnew(JsRef idobj)
+// TODO: Instead use tp_new and Python's inheritance system
+void
+JsProxy_cinit(PyObject* obj, JsRef idobj)
 {
-  JsProxy* self;
-  self = (JsProxy*)JsProxyType.tp_alloc(&JsProxyType, 0);
+  JsProxy* self = (JsProxy*)obj;
   self->vectorcall = JsProxy_Vectorcall;
   self->js = hiwire_incref(idobj);
   self->bytes = NULL;
   self->supports_kwargs = -1; // don't know
   self->awaited = false;
-  return (PyObject*)self;
+}
+
+PyObject*
+JsProxy_cnew(JsRef idobj)
+{
+  PyObject* self = JsProxyType.tp_alloc(&JsProxyType, 0);
+  JsProxy_cinit(self, idobj);
+  return self;
 }
 
 typedef struct
@@ -739,22 +747,25 @@ JsProxy_new_error(JsRef idobj)
 
 typedef struct
 {
-  PyObject_HEAD JsRef this_;
-  const char* name;
+  JsProxy super;
+  JsRef this_;
 } JsBoundMethod;
 
+#define JsBoundMethod_THIS(x) (((JsBoundMethod*)x)->this_)
+
 static void
-JsBoundMethod_dealloc(JsBoundMethod* self)
+JsBoundMethod_dealloc(PyObject* self)
 {
-  hiwire_decref(self->this_);
-  Py_TYPE(self)->tp_free((PyObject*)self);
+  hiwire_CLEAR(JsBoundMethod_THIS(self));
+  Py_TYPE(self)->tp_free(self);
 }
 
+// TODO: once #1033 is accepted, switch to VECTOR_CALL for this and unify the
+// argument handling here and there so that bound methods and unbound methods
+// actually behave the same.
 static PyObject*
-JsBoundMethod_Call(PyObject* o, PyObject* args, PyObject* kwargs)
+JsBoundMethod_Call(PyObject* self, PyObject* args, PyObject* kwargs)
 {
-  JsBoundMethod* self = (JsBoundMethod*)o;
-
   Py_ssize_t nargs = PyTuple_Size(args);
 
   JsRef idargs = hiwire_array();
@@ -765,31 +776,33 @@ JsBoundMethod_Call(PyObject* o, PyObject* args, PyObject* kwargs)
     hiwire_decref(idarg);
   }
 
-  JsRef idresult = hiwire_call_member(self->this_, self->name, idargs);
+  JsRef idresult =
+    hiwire_call_bound(JsProxy_REF(self), JsBoundMethod_THIS(self), idargs);
   hiwire_decref(idargs);
   PyObject* pyresult = js2python(idresult);
   hiwire_decref(idresult);
   return pyresult;
 }
 
 static PyTypeObject JsBoundMethodType = {
+  //.tp_base = &JsProxy, // We have to do this in jsproxy_init.
   .tp_name = "JsBoundMethod",
   .tp_basicsize = sizeof(JsBoundMethod),
   .tp_dealloc = (destructor)JsBoundMethod_dealloc,
   .tp_call = JsBoundMethod_Call,
-  .tp_flags = Py_TPFLAGS_DEFAULT,
+  .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
   .tp_doc = "A proxy to make it possible to call Javascript bound methods from "
             "Python."
 };
 
+// TODO: use tp_new and Python inheritance system
 static PyObject*
-JsBoundMethod_cnew(JsRef this_, const char* name)
+JsBoundMethod_cnew(JsRef func, JsRef this_)
 {
-  JsBoundMethod* self;
-  self = (JsBoundMethod*)JsBoundMethodType.tp_alloc(&JsBoundMethodType, 0);
-  self->this_ = hiwire_incref(this_);
-  self->name = name;
-  return (PyObject*)self;
+  PyObject* self = JsBoundMethodType.tp_alloc(&JsBoundMethodType, 0);
+  JsProxy_cinit(self, func);
+  JsBoundMethod_THIS(self) = hiwire_incref(this_);
+  return self;
 }
 
 ////////////////////////////////////////////////////////////
@@ -798,8 +811,7 @@ JsBoundMethod_cnew(JsRef this_, const char* name)
 bool
 JsProxy_Check(PyObject* x)
 {
-  return (PyObject_TypeCheck(x, &JsProxyType) ||
-          PyObject_TypeCheck(x, &JsBoundMethodType));
+  return PyObject_TypeCheck(x, &JsProxyType);
 }
 
 JsRef
@@ -862,6 +874,7 @@ JsProxy_init(PyObject* core_module)
   PyExc_BaseException_Type = (PyTypeObject*)PyExc_BaseException;
   _Exc_JsException.tp_base = (PyTypeObject*)PyExc_Exception;
 
+  JsBoundMethodType.tp_base = &JsProxyType;
   // Add JsException to the pyodide module so people can catch it if they want.
   FAIL_IF_MINUS_ONE(PyModule_AddType(core_module, &JsProxyType));
   FAIL_IF_MINUS_ONE(PyModule_AddType(core_module, &JsBoundMethodType));

diff --git a/src/tests/test_jsproxy.py b/src/tests/test_jsproxy.py
@@ -1,5 +1,6 @@
 # See also test_typeconversions, and test_python.
 import pytest
+from pyodide_build.testing import run_in_pyodide
 
 
 def test_jsproxy_dir(selenium):
@@ -420,3 +421,25 @@ async def test():
             r2 = c.send(r1.result())
             """
         )
+
+
+@run_in_pyodide
+def test_nested_attribute_access():
+    import js
+    from js import window
+
+    js.URL.createObjectURL
+    window.URL.createObjectURL
+
+
+@run_in_pyodide
+def test_window_isnt_super_weird_anymore():
+    import js
+    from js import window, Array
+
+    assert window.Array != window
+    assert window.Array == Array
+    assert window.window.window.window == window
+    assert js.window.Array == Array
+    assert js.window.window.window.window == window
+    assert window.window.window.window.Array == Array