ENH Improve PyProxy handling of read only properties (pyodide#4033)

A descriptor is *writable* if either writable is true or it has a setter. A descriptor is *deletable* if it is configurable. Also, the normal JS behavior here is to return false, not to throw.
rsohlot · Aug 7, 2023 · 0b2461f · 0b2461f
1 parent 85a1e1f
commit 0b2461f
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 36 deletions.
diff --git a/docs/project/changelog.md b/docs/project/changelog.md
@@ -15,6 +15,11 @@ myst:
 
 ## Unreleased
 
+- {{ Fix }} Fixed adding getters/setters to a `PyProxy` with
+  `Object.defineProperty` and improved compliance with JavaScript rules around
+  Proxy traps.
+  {pr}`4033`
+
 - {{ Enhancement }} Adds `check_wasm_magic_number` function to validate `.so`
   files for WebAssembly (WASM) compatibility.
   {pr}`4018`

diff --git a/src/core/pyproxy.ts b/src/core/pyproxy.ts
@@ -2153,11 +2153,11 @@ const PyProxyHandlers = {
   },
   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 '${String(jskey)}'`);
+    if (descr && !descr.writable && !descr.set) {
+      return false;
     }
     // python_setattr will crash if given a Symbol.
-    if (typeof jskey === "symbol") {
+    if (typeof jskey === "symbol" || filteredHasKey(jsobj, jskey, true)) {
       return Reflect.set(jsobj, jskey, jsval);
     }
     if (jskey.startsWith("$")) {
@@ -2168,19 +2168,22 @@ const PyProxyHandlers = {
   },
   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 '${String(jskey)}'`);
+    if (descr && !descr.configurable) {
+      // Must return "false" if "jskey" is a nonconfigurable own property.
+      // Otherwise JavaScript will throw a TypeError.
+      // Strict mode JS will throw an error here saying that the property cannot
+      // be deleted. It's good to leave everything alone so that the behavior is
+      // consistent with the error message.
+      return false;
     }
-    if (typeof jskey === "symbol") {
+    if (typeof jskey === "symbol" || filteredHasKey(jsobj, jskey, true)) {
       return Reflect.deleteProperty(jsobj, jskey);
     }
     if (jskey.startsWith("$")) {
       jskey = jskey.slice(1);
     }
     python_delattr(jsobj, jskey);
-    // Must return "false" if "jskey" is a nonconfigurable own property.
-    // Otherwise JavaScript will throw a TypeError.
-    return !descr || !!descr.configurable;
+    return true;
   },
   ownKeys(jsobj: PyProxy): (string | symbol)[] {
     let ptrobj = _getPtr(jsobj);

diff --git a/src/tests/test_pyproxy.py b/src/tests/test_pyproxy.py
@@ -383,7 +383,7 @@ def test_pyproxy_get_buffer_type_argument(selenium, array_type):
         selenium.run_js("a.destroy(); self.a = undefined;")
 
 
-def test_pyproxy_mixins(selenium):
+def test_pyproxy_mixins1(selenium):
     result = selenium.run_js(
         """
         let [noimpls, awaitable, iterable, iterator, awaititerable, awaititerator] = pyodide.runPython(`
@@ -489,9 +489,10 @@ def test_pyproxy_mixins2(selenium):
     )
 
 
-def test_pyproxy_mixins3(selenium):
+def test_pyproxy_mixins31(selenium):
     selenium.run_js(
         """
+        "use strict";
         let [Test, t] = pyodide.runPython(`
             class Test: pass
             from pyodide.ffi import to_js
@@ -512,21 +513,93 @@ class Test: pass
         pyodide.runPython("assert Test.prototype == 7");
         pyodide.runPython("assert Test.name == 7");
         pyodide.runPython("assert Test.length == 7");
-        delete Test.prototype;
+        // prototype cannot be removed once added because it is nonconfigurable...
+        assertThrows(() => delete Test.prototype, "TypeError", "");
         delete Test.name;
         delete Test.length;
-        pyodide.runPython(`assert not hasattr(Test, "prototype")`);
+        pyodide.runPython(`assert Test.prototype == 7`);
         pyodide.runPython(`assert not hasattr(Test, "name")`);
         pyodide.runPython(`assert not hasattr(Test, "length")`);
 
-        assertThrows( () => Test.$$ = 7, "TypeError", /^Cannot set read only field/);
-        assertThrows( () => delete Test.$$, "TypeError", /^Cannot delete read only field/);
+        assertThrows(() => Test.$$ = 7, "TypeError", "");
+        assertThrows(() => delete Test.$$, "TypeError", "");
+
+        Test.$a = 7;
+        Object.defineProperty(Test, "a", {
+            get(){ return Test.$a + 1; },
+            set(v) {
+                Test.$a = v;
+            }
+        });
+
+        pyodide.runPython("assert Test.a == 7")
+        assert(() => Test.a === 8);
+        Test.a = 9;
+        assert(() => Test.a === 10);
+        pyodide.runPython("assert Test.a == 9")
+        assertThrows(() => delete Test.a, "TypeError", "");
+
+        Object.defineProperty(Test, "b", {
+            get(){ return Test.$a + 2; },
+        });
+        assert(() => Test.b === 11);
+        assertThrows(() => Test.b = 7,"TypeError", "");
+        assertThrows(() => delete Test.b, "TypeError", "");
         Test.destroy();
         t.destroy();
         """
     )
 
 
+@pytest.mark.parametrize("configurable", [False, True])
+@pytest.mark.parametrize("writable", [False, True])
+def test_pyproxy_mixins32(selenium, configurable, writable):
+    # Probably should get rid of this...
+    match selenium.browser:
+        case "node" | "chrome":
+            template = "'{}' on proxy: trap returned falsish for property 'x'"
+            setText = template.format("set")
+            deleteText = template.format("deleteProperty")
+        case "firefox":
+            template = "proxy {} handler returned false"
+            setText = template.format("set")
+            deleteText = template.format("deleteProperty")
+        case "safari":
+            setText = "Proxy object's 'set' trap returned falsy value for property 'x'"
+            deleteText = "Unable to delete property."
+
+    selenium.run_js(
+        f"""
+        "use strict";
+        const configurable = !!{int(configurable)};
+        const writable = !!{int(writable)};
+        """
+        """
+        const d = pyodide.runPython("{}");
+        Object.defineProperty(d, "x", {
+            value: 9,
+            configurable,
+            writable,
+        });
+        assert(() => d.x === 9);
+        if(writable) {
+            d.x = 10;
+            assert(() => d.x === 10);
+        } else {
+            assertThrows(() => d.x = 10, "TypeError", "%s");
+        }
+        if(configurable) {
+            delete d.x;
+            assert(() => d.x === undefined);
+        } else {
+            assertThrows(() => delete d.x, "TypeError", "%s");
+        }
+        d.destroy();
+        """
+        % (setText, deleteText)
+    )
+
+
 def test_pyproxy_mixins41(selenium):
     selenium.run_js(
         """
@@ -578,27 +651,35 @@ def __call__(self, x):
 
 
 def test_pyproxy_mixins5(selenium):
-    selenium.run_js(
-        """
-        [Test, t] = pyodide.runPython(`
-            class Test:
-                def __len__(self):
-                    return 9
-            from pyodide.ffi import to_js
-            to_js([Test, Test()])
-        `);
-        assert(() => !("length" in Test));
-        assert(() => t.length === 9);
-        assert(() => t instanceof pyodide.ffi.PyProxyWithLength);
-        t.length = 10;
-        assert(() => t.$length === 10);
-        let t__len__ = t.__len__;
-        assert(() => t__len__() === 9);
-        t__len__.destroy();
-        Test.destroy();
-        t.destroy();
-        """
-    )
+    try:
+        r = selenium.run_js(
+            """
+            "use strict";
+            const [Test, t] = pyodide.runPython(`
+                class Test:
+                    def __len__(self):
+                        return 9
+                from pyodide.ffi import to_js
+                to_js([Test, Test()])
+            `);
+            assert(() => !("length" in Test));
+            assert(() => t.length === 9);
+            assert(() => t instanceof pyodide.ffi.PyProxyWithLength);
+            assertThrows(() => {t.length = 10}, "TypeError", "'set' on proxy: trap returned falsish for property 'length'");
+            assert(() => t.length === 9);
+
+            // For some reason, this is the normal behavior for a JS getter:
+            // delete just does nothing...
+            delete t.length;
+            assert(() => t.length === 9);
+
+            Test.destroy();
+            t.destroy();
+            """
+        )
+        print(r)
+    finally:
+        print(selenium.logs)
 
 
 def test_pyproxy_mixins6(selenium):