Make PyProxy of a callable an instanceof Function (#3925)

Resolves #3924. Some code checks whether an object is callable with `instanceof Function`.
The correct way to check for callability is with `typeof x === "function"`, but ideally we want
to work with existing code that uses a less accurate callability check.

Make a copy of the PyProxy class which is a subclass of Function.
Since the difference between `PyProxy` and `PyProxyFunction` is an implementation
detail, we use `[Symbol.hasInstance]` to make them recognize the same set of objects
as their instances.

we also need some extra logic to filter out additional attributes that come from the
`Function` prototype.

docs/project/changelog.md

@@ -53,6 +53,12 @@ myst:
   `{env: {HOME: whatever_directory}}`.
   {pr}`3870`
 
+- {{ Fix }} A `PyProxy` of a callable is now an `instanceof Function`. (If you
+  are trying to feature detect whether something is callable or not in
+  JavaScript, the correct way is to use `typeof o === "function"`. But you may
+  have dependencies that don't do this correctly.)
+  {pr}`3925`
+
 ### Packages
 
 - OpenBLAS has been added and scipy now uses OpenBLAS rather than CLAPACK

src/core/pyproxy.ts

@@ -14,6 +14,7 @@
  * See Makefile recipe for src/js/pyproxy.gen.ts
  */
 
+declare var Tests: any;
 declare var Module: any;
 declare var Hiwire: any;
 declare var API: any;
@@ -322,9 +323,10 @@ Module.getPyProxyClass = function (flags: number) {
     descriptors,
     Object.getOwnPropertyDescriptors({ $$flags: flags }),
   );
-  let new_proto = Object.create(PyProxy.prototype, descriptors);
+  const super_proto = flags & IS_CALLABLE ? PyProxyFunctionProto : PyProxyProto;
+  const sub_proto = Object.create(super_proto, descriptors);
   function NewPyProxyClass() {}
-  NewPyProxyClass.prototype = new_proto;
+  NewPyProxyClass.prototype = sub_proto;
   pyproxyClassMap.set(flags, NewPyProxyClass);
   return NewPyProxyClass;
 };
@@ -472,6 +474,12 @@ export class PyProxy {
   /** @private */
   $$flags: number;
 
+  static [Symbol.hasInstance](obj: any): obj is PyProxy {
+    return [PyProxy, PyProxyFunction].some((cls) =>
+      Function.prototype[Symbol.hasInstance].call(cls, obj),
+    );
+  }
+
   /**
    * @private
    * @hideconstructor
@@ -734,6 +742,20 @@ export class PyProxy {
   }
 }
 
+const PyProxyProto = PyProxy.prototype;
+// For some weird reason in the node and firefox tests, the identity of
+// `Function` changes between now and the test suite. Can't reproduce this
+// outside the test suite though...
+// See test_pyproxy_instanceof_function.
+Tests.Function = Function;
+const PyProxyFunctionProto = Object.create(
+  Function.prototype,
+  Object.getOwnPropertyDescriptors(PyProxyProto),
+);
+function PyProxyFunction() {}
+PyProxyFunction.prototype = PyProxyFunctionProto;
+globalThis.PyProxyFunction = PyProxyFunction;
+
 /**
  * A :js:class:`~pyodide.ffi.PyProxy` whose proxied Python object has a :meth:`~object.__len__`
  * method.
@@ -2024,6 +2046,36 @@ function python_pop(jsobj: any, pop_start: boolean): void {
   return Hiwire.pop_value(res);
 }
 
+function filteredHasKey(
+  jsobj: PyProxy,
+  jskey: string | symbol,
+  filterProto: boolean,
+) {
+  if (jsobj instanceof Function) {
+    // If we are a PyProxy of a callable we have to subclass function so that if
+    // someone feature detects callables with `instanceof Function` it works
+    // correctly. But the callable might have attributes `name` and `length` and
+    // we don't want to shadow them with the values from `Function.prototype`.
+    return (
+      jskey in jsobj &&
+      !(
+        [
+          "name",
+          "length",
+          "caller",
+          "arguments",
+          // we are required by JS law to return `true` for `"prototype" in pycallable`
+          // but we are allowed to return the value of `getattr(pycallable, "prototype")`.
+          // So we filter prototype out of the "get" trap but not out of the "has" trap
+          filterProto ? "prototype" : undefined,
+        ] as (string | symbol)[]
+      ).includes(jskey)
+    );
+  } else {
+    return jskey in jsobj;
+  }
+}
+
 // See explanation of which methods should be defined here and what they do
 // here:
 // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
@@ -2031,11 +2083,10 @@ const PyProxyHandlers = {
   isExtensible(): boolean {
     return true;
   },
-  has(jsobj: PyProxy, jskey: any): boolean {
+  has(jsobj: PyProxy, jskey: string | symbol): boolean {
     // Note: must report "prototype" in proxy when we are callable.
     // (We can return the wrong value from "get" handler though.)
-    let objHasKey = Reflect.has(jsobj, jskey);
-    if (objHasKey) {
+    if (filteredHasKey(jsobj, jskey, false)) {
       return true;
     }
     // python_hasattr will crash if given a Symbol.
@@ -2047,13 +2098,13 @@ const PyProxyHandlers = {
     }
     return python_hasattr(jsobj, jskey);
   },
-  get(jsobj: PyProxy, jskey: any): any {
+  get(jsobj: PyProxy, jskey: string | symbol): any {
     // Preference order:
     // 1. stuff from JavaScript
     // 2. the result of Python getattr
 
     // python_getattr will crash if given a Symbol.
-    if (jskey in jsobj || typeof jskey === "symbol") {
+    if (typeof jskey === "symbol" || filteredHasKey(jsobj, jskey, true)) {
       return Reflect.get(jsobj, jskey);
     }
     // If keys start with $ remove the $. User can use initial $ to
@@ -2067,10 +2118,10 @@ const PyProxyHandlers = {
       return Hiwire.pop_value(idresult);
     }
   },
-  set(jsobj: PyProxy, jskey: any, jsval: any): boolean {
+  set(jsobj: PyProxy, jskey: string | symbol, jsval: any): boolean {
     let descr = Object.getOwnPropertyDescriptor(jsobj, jskey);
     if (descr && !descr.writable) {
-      throw new TypeError(`Cannot set read only field '${jskey}'`);
+      throw new TypeError(`Cannot set read only field '${String(jskey)}'`);
     }
     // python_setattr will crash if given a Symbol.
     if (typeof jskey === "symbol") {
@@ -2082,10 +2133,10 @@ const PyProxyHandlers = {
     python_setattr(jsobj, jskey, jsval);
     return true;
   },
-  deleteProperty(jsobj: PyProxy, jskey: any): boolean {
+  deleteProperty(jsobj: PyProxy, jskey: string | symbol): boolean {
     let descr = Object.getOwnPropertyDescriptor(jsobj, jskey);
     if (descr && !descr.writable) {
-      throw new TypeError(`Cannot delete read only field '${jskey}'`);
+      throw new TypeError(`Cannot delete read only field '${String(jskey)}'`);
     }
     if (typeof jskey === "symbol") {
       return Reflect.deleteProperty(jsobj, jskey);

src/tests/test_pyproxy.py

@@ -527,7 +527,7 @@ def test_pyproxy_mixins3(selenium):
     )
 
 
-def test_pyproxy_mixins4(selenium):
+def test_pyproxy_mixins41(selenium):
     selenium.run_js(
         """
         [Test, t] = pyodide.runPython(`
@@ -536,24 +536,47 @@ def test_pyproxy_mixins4(selenium):
                 prototype="prototype"
                 name="me"
                 length=7
+                def __call__(self, x):
+                    return x + 1
+
             from pyodide.ffi import to_js
             to_js([Test, Test()])
         `);
         assert(() => Test.$prototype === "prototype");
-        assert(() => Test.prototype === undefined);
+        assert(() => Test.prototype === "prototype");
         assert(() => Test.name==="me");
         assert(() => Test.length === 7);
 
         assert(() => t.caller === "fifty");
+        assert(() => "prototype" in t);
         assert(() => t.prototype === "prototype");
         assert(() => t.name==="me");
         assert(() => t.length === 7);
+        assert(() => t(7) === 8);
         Test.destroy();
         t.destroy();
         """
     )
 
 
+def test_pyproxy_mixins42(selenium):
+    selenium.run_js(
+        """
+        let t = pyodide.runPython(`
+            class Test:
+                def __call__(self, x):
+                    return x + 1
+
+            from pyodide.ffi import to_js
+            Test()
+        `);
+        assert(() => "prototype" in t);
+        assert(() => t.prototype === undefined);
+        t.destroy();
+        """
+    )
+
+
 def test_pyproxy_mixins5(selenium):
     selenium.run_js(
         """
@@ -2096,3 +2119,101 @@ def test_pyproxy_of_list_fill(selenium, func):
     assert func(a) is a
     func(ajs)
     assert a == ajs.to_py()
+
+
+def test_pyproxy_instanceof_function(selenium):
+    weird_function_shim = ""
+    if selenium.browser in ["firefox", "node"]:
+        # A hack to make the test work: In node and firefox this test fails. But
+        # I can't reproduce the failure in a normal browser / outside of the
+        # test suite. The trouble seems to be that the value of
+        # `globalThis.Function` changes its identity from when we define
+        # `PyProxyFunction` to when we execute this test. So we store `Function`
+        # on `pyodide._api.tests` so we can retrieve the original value of it
+        # for the test. This is nonsense but because the failure only occurs in
+        # the test suite and not in real life I guess it's okay????
+        # Also, no clue how node and firefox are affected but not Chrome.
+        weird_function_shim = "let Function = pyodide._api.tests.Function;"
+
+    selenium.run_js(
+        f"""
+        {weird_function_shim}
+        """
+        """
+        const pyFunc_0 = pyodide.runPython(`
+            lambda: print("zero")
+        `);
+
+        const pyFunc_1 = pyodide.runPython(`
+            def foo():
+                print("two")
+            foo
+        `);
+
+        const pyFunc_2 = pyodide.runPython(`
+            class A():
+                def a(self):
+                    print("three") # method from class
+            A.a
+        `);
+
+        const pyFunc_3 = pyodide.runPython(`
+            class B():
+                def __call__(self):
+                    print("five (B as a callable instance)")
+
+            b = B()
+            b
+        `);
+
+        assert(() => pyFunc_0 instanceof Function);
+        assert(() => pyFunc_0 instanceof pyodide.ffi.PyProxy);
+        assert(() => pyFunc_0 instanceof pyodide.ffi.PyCallable);
+
+        assert(() => pyFunc_1 instanceof Function);
+        assert(() => pyFunc_1 instanceof pyodide.ffi.PyProxy);
+        assert(() => pyFunc_1 instanceof pyodide.ffi.PyCallable);
+
+        assert(() => pyFunc_2 instanceof Function);
+        assert(() => pyFunc_2 instanceof pyodide.ffi.PyProxy);
+        assert(() => pyFunc_2 instanceof pyodide.ffi.PyCallable);
+
+        assert(() => pyFunc_3 instanceof Function);
+        assert(() => pyFunc_3 instanceof pyodide.ffi.PyProxy);
+        assert(() => pyFunc_3 instanceof pyodide.ffi.PyCallable);
+
+        d = pyodide.runPython("{}");
+        assert(() => !(d instanceof Function));
+        assert(() => !(d instanceof pyodide.ffi.PyCallable));
+        assert(() => d instanceof pyodide.ffi.PyProxy);
+        assert(() => d instanceof pyFunc_0.constructor);
+        assert(() => pyFunc_0 instanceof d.constructor);
+
+        for(const p of [pyFunc_0, pyFunc_1, pyFunc_2, pyFunc_3, d])  {
+            p.destroy();
+        }
+        """
+    )
+
+
+def test_pyproxy_callable_prototype(selenium):
+    r = selenium.run_js(
+        """
+        const o = pyodide.runPython("lambda:None");
+        const res = Object.fromEntries(Reflect.ownKeys(Function.prototype).map(k => [k.toString(), k in o]));
+        o.destroy();
+        return res;
+        """
+    )
+    assert r == {
+        "length": False,
+        "name": False,
+        "arguments": False,
+        "caller": False,
+        "constructor": True,
+        "apply": True,
+        "bind": True,
+        "call": True,
+        "toString": True,
+        "Symbol(Symbol.hasInstance)": True,
+    }