From 0b2461fc1971913eb7333f1b125bb7edb1168151 Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Mon, 7 Aug 2023 12:57:45 +0200 Subject: [PATCH] ENH Improve PyProxy handling of read only properties (#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. --- docs/project/changelog.md | 5 ++ src/core/pyproxy.ts | 21 +++--- src/tests/test_pyproxy.py | 135 ++++++++++++++++++++++++++++++-------- 3 files changed, 125 insertions(+), 36 deletions(-) diff --git a/docs/project/changelog.md b/docs/project/changelog.md index 54cc24b651b..17c2caf0a28 100644 --- 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 index 40dcf46f9ee..54e8c8ce38b 100644 --- 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 index 2aaf520913a..a2d27478e3b 100644 --- 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):