Repository: pyodide/pyodide
Pull request: Make PyProxy of a callable an instanceof Function
Resolves #3924. 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.

E.g.,
```js
const pyFunc = pyodide.runPython(`lambda: print("zero")`);
pyFunc instanceof Function
```

### Checklist

- [x] Add a [CHANGELOG](https://github.com/pyodide/pyodide/blob/main/docs/project/changelog.md) entry
- [x] Add / update tests



Problem statement:
Python Callables are not instanceof Function
## 🐛 Bug

Using the `instanceof` operator on PyProxy's of Python Functions (or other Callables) returns False.

### To Reproduce

All of the following result in `false`:

```js
const pyFunc_0 = pyodide.runPython(`
    lambda: print("zero")
`);

const pyFunc_1 = pyodide.runPython(`
    from pyodide.ffi import create_proxy
    create_proxy(lambda: print("one"))
`);

const pyFunc_2 = pyodide.runPython(`
    def foo():
        print("two")
    foo
`)

const pyFunc_3 = pyodide.runPython(`
    class A():
        def a(self):
            print("three") # method from class
    A.a
`)

const pyFunc_4 = pyodide.runPython(`
    class A():
        def a(self):
            print("four") # method from instance
    A().a
`)

const pyCallable_5 = pyodide.runPython(`
    class B():
        def __call__(self):
            print("five (B as a callable instance)")

    b = B()
    b
`)

console.log(pyFunc_0 instanceof Function);
console.log(pyFunc_1 instanceof Function);
console.log(pyFunc_2 instanceof Function)
console.log(pyFunc_3 instanceof Function)
console.log(pyFunc_4 instanceof Function)
console.log(pyCallable_5 instanceof Function)

// Call all the things, to verify they really are callable
pyFunc_0()
pyFunc_1()
pyFunc_2()
pyFunc_3('a')
pyFunc_4()
pyCallable_5()
```

### Expected behavior

I'd expect Python callables (or at least functions/lambdas) make `instanceof Function` true.

### Environment

- This the case in Pyodide:
  -  0.23
  -  0.22
  -  0.21
  -  0.20 (*with the previous import of `create_proxy` from just the `pyodide` module)
  -  0.19 (*with the indexURL supplied to `loadPyodide`)
  - *That's as far as I got; in 0.18 and earlier, this code throws a fatal error: `TypeError: Module._run_python_simple_inner is not a function`

Hints from issue discussion:
Thanks for another great bug report @JeffersGlass!

Commit message: [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Patch to review:
diff --git a/src/core/pyproxy.ts b/src/core/pyproxy.ts
index 00e53eb81b0..7b634f6ca9e 100644
--- a/src/core/pyproxy.ts
+++ b/src/core/pyproxy.ts
@@ -322,9 +322,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 +473,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 +741,14 @@ export class PyProxy {
   }
 }
 
+const PyProxyProto = PyProxy.prototype;
+const PyProxyFunctionProto = Object.create(
+  Function.prototype,
+  Object.getOwnPropertyDescriptors(PyProxyProto),
+);
+function PyProxyFunction() {}
+PyProxyFunction.prototype = PyProxyFunctionProto;
+
 /**
  * A :js:class:`~pyodide.ffi.PyProxy` whose proxied Python object has a :meth:`~object.__len__`
  * method.
@@ -2035,7 +2050,11 @@ const PyProxyHandlers = {
     // 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 (
+      objHasKey &&
+      jskey in jsobj &&
+      (!(jsobj instanceof Function) || (jskey !== "name" && jskey !== "length"))
+    ) {
       return true;
     }
     // python_hasattr will crash if given a Symbol.
@@ -2053,7 +2072,12 @@ const PyProxyHandlers = {
     // 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" ||
+      (jskey in jsobj &&
+        (!(jsobj instanceof Function) ||
+          (jskey !== "name" && jskey !== "length")))
+    ) {
       return Reflect.get(jsobj, jskey);
     }
     // If keys start with $ remove the $. User can use initial $ to
diff --git a/src/tests/test_pyproxy.py b/src/tests/test_pyproxy.py
index 1b021c05077..6d3e696e856 100644
--- a/src/tests/test_pyproxy.py
+++ b/src/tests/test_pyproxy.py
@@ -2096,3 +2096,63 @@ 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):
+    selenium.run_js(
+        """
+        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(() => 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();
+        }
+        """
+    )


Code review comments:
diff_hunk: @@ -2096,3 +2096,63 @@ 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):
+    selenium.run_js(
+        """
+        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(() => d instanceof pyFunc_0.constructor);
path: src/tests/test_pyproxy.py
text: @user1:
The two checks are the same.

diff_hunk: @@ -2053,7 +2072,12 @@ const PyProxyHandlers = {
     // 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" ||
+      (jskey in jsobj &&
+        (!(jsobj instanceof Function) ||
+          (jskey !== "name" && jskey !== "length")))
path: src/core/pyproxy.ts
text: @user1:
repeated: I suggest this check is factored out as utility such as `isKeyAcceptable` or something

diff_hunk: @@ -2035,7 +2050,11 @@ const PyProxyHandlers = {
     // 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 (
+      objHasKey &&
+      jskey in jsobj &&
path: src/core/pyproxy.ts
text: @user1:
isn't this exactly what `Reflect.has(jsobj, jskey)` returns? if not ... due Proxy indirection, what's the reason to use `Reflect.has(jsobj, jskey)` at all? 

@author:
Oops good point.