diff --git a/changelog.d/9247-prototype-override-consumers.md b/changelog.d/9247-prototype-override-consumers.md new file mode 100644 index 0000000000..5af2dafde0 --- /dev/null +++ b/changelog.d/9247-prototype-override-consumers.md @@ -0,0 +1,13 @@ +A receiver with a per-instance `[[Prototype]]` override no longer loses the +methods Perry synthesizes rather than storing on a real prototype. The +override fast path assumed the resolved method was a user closure, so a +property-lookup miss called `undefined` (`[...gen().map(f)]` threw +"undefined is not iterable"), a field-get miss hid the plain-function +`.prototype` and the boxed-wrapper builtins, and a native method was invoked +with no receiver bound (`Object.prototype.isPrototypeOf` and +`Object(true).valueOf()` both threw). + +The fast path now runs only for a resolved callable, a field-get miss defers +to the rest of the lookup instead of answering `undefined`, and the receiver +is bound for the duration of the call. A resolved hit still wins over the +class vtable, so an explicit `Object.setPrototypeOf` is unaffected. diff --git a/crates/perry-runtime/src/object/field_get_set.rs b/crates/perry-runtime/src/object/field_get_set.rs index 5f8dad3ab6..5a9a2bb98c 100644 --- a/crates/perry-runtime/src/object/field_get_set.rs +++ b/crates/perry-runtime/src/object/field_get_set.rs @@ -221,6 +221,7 @@ mod probe_dispatch; /// #9131: per-instance `[[Prototype]]` override lookup, split out of /// `get_field_by_name_tail.rs` for the 2000-line cap. mod prototype_override; +#[allow(dead_code)] // #9244: field-get short-circuits removed; kept for the method path. /// Size of the direct-mapped `(keys_ptr, key_hash, field_index)` inline /// cache backing `js_object_get_field_by_name`'s slow tail. diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs index a3fa385b1c..0ed1a214e9 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs @@ -1350,6 +1350,8 @@ pub(crate) fn get_field_by_name_object_tail( if keys.is_null() { // #9131; see `prototype_override::inherited_field_if_overridden`. + // A miss returns None so the synthesized arms below stay reachable + // (#9244). if let Some(v) = super::prototype_override::inherited_field_if_overridden(obj, key) { return v; } diff --git a/crates/perry-runtime/src/object/field_get_set/prototype_override.rs b/crates/perry-runtime/src/object/field_get_set/prototype_override.rs index 158096edac..762214dd02 100644 --- a/crates/perry-runtime/src/object/field_get_set/prototype_override.rs +++ b/crates/perry-runtime/src/object/field_get_set/prototype_override.rs @@ -8,14 +8,27 @@ use crate::object::ObjectHeader; use crate::value::JSValue; /// An explicit per-instance `[[Prototype]]` REPLACES the class's declaration -/// prototype; it is not an extra link in front of the original vtable. So when -/// the own-key scan misses, walk that authoritative chain before exposing class -/// getters or methods, and do not resurrect the old class surface when the -/// custom chain also misses — hence `Some(undefined)` rather than `None` once -/// an override is present. +/// prototype, so when the own-key scan misses, that chain is what decides — +/// `Some(value)` here is authoritative and the caller must not fall back to the +/// class vtable. /// -/// `None` means no override was installed and the caller keeps its existing -/// class-vtable fallback. +/// A miss on the custom chain returns `None`, NOT `Some(undefined)`. #9131 +/// originally returned `Some(undefined)` to avoid resurrecting the old class +/// surface, but the arms BELOW both call sites are not only the class vtable: +/// they are also everything Perry *synthesizes* rather than stores on a real +/// prototype — a plain-function `.prototype`, the boxed-wrapper builtins, the +/// iterator helpers. Swallowing the miss made those unreachable for every +/// flagged receiver, which is #9244 (`Object(true).valueOf()` → +/// `called on incompatible receiver`, `FooObj.prototype` → `undefined`). +/// +/// This is the same polarity `canonical_shape_excludes_own_property` uses: a +/// question we cannot answer here defers to the tail rather than fabricating a +/// verdict. The flag is also set by ~20 runtime prototype-wiring sites that are +/// not user `setPrototypeOf` calls at all, so "flagged" is far weaker evidence +/// than the original code assumed. +/// +/// `None` therefore means either no override, or an override that does not +/// carry this key — in both cases the caller keeps its existing fallback. pub(super) fn inherited_field_if_overridden( obj: *const ObjectHeader, key: *const crate::string::StringHeader, @@ -26,8 +39,5 @@ pub(super) fn inherited_field_if_overridden( if !crate::object::prototype_chain::object_has_prototype_override(obj as usize) { return None; } - Some( - crate::object::prototype_chain::resolve_inherited_field(obj as usize, key) - .unwrap_or_else(JSValue::undefined), - ) + crate::object::prototype_chain::resolve_inherited_field(obj as usize, key) } diff --git a/crates/perry-runtime/src/object/native_call_method.rs b/crates/perry-runtime/src/object/native_call_method.rs index 4b9d1f6dd9..5b1e468c6e 100644 --- a/crates/perry-runtime/src/object/native_call_method.rs +++ b/crates/perry-runtime/src/object/native_call_method.rs @@ -1150,6 +1150,29 @@ pub unsafe extern "C-unwind" fn js_native_call_method_nullsafe( js_native_call_method(object, method_name_ptr, method_name_len, args_ptr, args_len) } +/// Bind `IMPLICIT_THIS` for the duration of one call and restore the previous +/// value on the way out — including when the callee unwinds, which this +/// `extern "C-unwind"` dispatch surface makes an ordinary outcome rather than an +/// exotic one. A plain set/restore pair would leak the receiver into every later +/// implicit-`this` read once a method throws. #9244. +struct ImplicitThisScope { + previous: f64, +} + +impl ImplicitThisScope { + fn bind(receiver: f64) -> Self { + Self { + previous: crate::object::js_implicit_this_set(receiver), + } + } +} + +impl Drop for ImplicitThisScope { + fn drop(&mut self) { + crate::object::js_implicit_this_set(self.previous); + } +} + #[no_mangle] // Dynamic native calls may synchronously throw from the selected module // implementation. Keep this bridge unwind-capable so a generated caller's JS @@ -1268,18 +1291,46 @@ pub unsafe extern "C-unwind" fn js_native_call_method( let receiver_ptr = JSValue::from_bits(receiver.to_bits()).as_pointer::(); let method = super::js_object_get_field_by_name(receiver_ptr, method_key); - let method_handle = root_scope.root_nanbox_f64(f64::from_bits(method.bits())); - let receiver = object(); - let bound = crate::closure::clone_closure_rebind_this( - method_handle.get_nanbox_f64().to_bits(), - receiver, - ); - let args = refreshed_args(); - return crate::closure::js_native_call_value( - f64::from_bits(bound), - args.as_ptr(), - args.len(), - ); + // Only the RESOLVED case is authoritative. A miss means the + // overridden chain simply does not carry this name, and the + // dispatch tower below still has arms that legitimately answer + // it — notably the #2874 iterator-helper interception, which + // resolves `map`/`filter`/`take`/... on a raw iterator that has + // no such own or inherited property. Returning here on a miss + // called `undefined` and turned `[...gen().map(f)]` into + // `TypeError: undefined is not iterable` (#9244). Falling + // through costs an ordinary lookup on a path that was already + // taking one. + let resolved = !method.is_undefined() + && crate::closure::is_closure_ptr(crate::value::js_nanbox_get_pointer( + f64::from_bits(method.bits()), + ) as usize); + if resolved { + let method_handle = root_scope.root_nanbox_f64(f64::from_bits(method.bits())); + let receiver = object(); + let bound = crate::closure::clone_closure_rebind_this( + method_handle.get_nanbox_f64().to_bits(), + receiver, + ); + let args = refreshed_args(); + // `clone_closure_rebind_this` only rewrites a closure that + // carries CAPTURES_THIS_FLAG; it returns a native builtin + // (and an arrow, and a generator step) UNCHANGED. Native + // method bodies read their receiver from + // `js_implicit_this_get()` — the same #7576 property that + // made the `%IteratorPrototype%.next` THUNK throw — so + // without binding it here `Object.prototype.isPrototypeOf` + // saw no `this` ("called on null or undefined") and + // `Object(true).valueOf()` saw the wrong one ("called on + // incompatible receiver"). #9244. Restored on the way out, + // including when the callee throws. + let _this_scope = ImplicitThisScope::bind(receiver); + return crate::closure::js_native_call_value( + f64::from_bits(bound), + args.as_ptr(), + args.len(), + ); + } } } } diff --git a/crates/perry/tests/issue_9131_prototype_method_replacement.rs b/crates/perry/tests/issue_9131_prototype_method_replacement.rs index 240c6ddd60..bfc1c1b316 100644 --- a/crates/perry/tests/issue_9131_prototype_method_replacement.rs +++ b/crates/perry/tests/issue_9131_prototype_method_replacement.rs @@ -101,3 +101,39 @@ console.log("swap-after:", readA(a)); "replace: 3 300\nmid: 153\ncounter-before: 2\ncounter-after: 777\nswap-before: a\nswap-after: b\n" ); } + +/// #9244: the per-instance prototype-override fast path in +/// `js_native_call_method` resolves the method by ordinary property lookup. +/// A generator carries the override flag but has no own or inherited `map` — +/// Perry synthesizes the iterator helpers (#2874) further down the dispatch +/// tower. Returning on the lookup MISS called `undefined`, so +/// `[...gen().map(f)]` threw `TypeError: undefined is not iterable`. Only a +/// resolved callable may take the fast path; a miss must fall through. +#[test] +fn overridden_receiver_miss_falls_through_to_synthesized_helpers() { + let stdout = compile_and_run( + r#" +function* gen() { + yield 1; + yield 2; + yield 3; +} +console.log("map:", [...gen().map((x: number) => x * 2)].join(",")); +console.log("filter:", [...gen().filter((x: number) => x > 1)].join(",")); +console.log("take:", Iterator.from([1, 2, 3, 4]).take(2).toArray().join(",")); + +// A genuine override that DOES resolve still wins over the class vtable. +class Counter { + inc() { return 2; } +} +const counter = new Counter(); +Object.setPrototypeOf(counter, { inc: () => 777 }); +console.log("override:", (counter as any).inc()); +"#, + ); + + assert_eq!( + stdout, + "map: 2,4,6\nfilter: 2,3\ntake: 1,2\noverride: 777\n" + ); +}