fix: dynamic array-method dispatch dropped the thisArg — extracted builtin method as callback with thisArg (#6658)

[proggeramlug]

Root cause

Pi bring-up wall #7 (tracker #6564). At pi-bundle.mjs:203827 (@babel/types' alias-expansion loop), e5 ? e5.forEach(t4.add, t4) : t4.add(r4) threw TypeError: Method Set.prototype.add called on incompatible receiver; node prints sizes: 3 1.

It is NOT the ternary, NOT method extraction, and NOT a plain forEach-thisArg gap (the issue's five ruled-out shapes all work): the dynamic method-dispatch tower (js_native_call_method → the dense-array arms in native_call_method/handle_methods.rs) dropped args[1] — the thisArg — for the whole Array.prototype callback family (forEach/map/filter/some/every/find/findIndex/findLast/findLastIndex) and dispatched to the dense helpers, which deliberately bind the callback's this to undefined (the spec rule for an absent thisArg).

The static lowering (lower_array_method.rs) already routes explicit-thisArg calls through the this-binding js_arraylike_* engine. A receiver only reaches the dynamic tower when codegen can't prove its type — in the bundle, e5 is an object member read through a dynamic key (FLIPPED[r4]), which defeats flow typing. That is why every statically-provable simplification of the shape worked and only the combined form reproduced. The extracted t4.add resolves to the identity-preserving brand-checking Set.prototype.add thunk (#3662), reads IMPLICIT_THIS = undefined, and throws. A lexically-capturing closure callback on the same path silently ran with the wrong this instead (arr.map(fn, thisArg) observed this !== thisArg — also fixed).

Fix

• native_call_method/handle_methods.rs: mirror the static rule in the dynamic tower — with a thisArg present (args_len >= 2), route the nine-method family through dispatch_arraylike_read_method, whose ThisGuard binds the thisArg around each callback call (real-array receivers keep a fast element path inside the engine). flatMap has no engine entry and shares the same thisArg gap on the static path too; untouched here.
• collection_proto_thunks.rs: node appends V8's NoSideEffectsToString rendering of the receiver to the brand-check TypeError (... called on incompatible receiver #<Object> / undefined / 5.5 / [object Array] / #<Foo> / Error: boom). throw_incompatible_receiver now renders the receiver the same way, so the throwing cases are message-identical to node. (No builtin property attributes touched — the fix(runtime,codegen): expose the Web Streams globals; resolve instanceof against a built-in held in a variable #6335 gotcha territory was left alone.)

Tests

• crates/perry/tests/issue_6658_extracted_builtin_method_thisarg.rs: the issue's 11-line combined shape verbatim; the five previously-ruled-out simpler shapes as regression anchors; the minimal dynamic-key trigger with Set.prototype.add, Map.prototype.set, and Array.prototype.push extracted as callbacks; this-binding asserted across the whole nine-method family; eight brand-check throw cases with node-v26-byte-identical messages (undefined, #<Object>, cross-brand #<Set>, 5.5, abc, null, #<Foo>, [object Array]).
• test-parity/node-suite/object/extracted-builtin-method-callback-thisarg.ts: same coverage through the parity runner — 100% vs node v26.
• Suites: perry-runtime lib 1426/0 (--test-threads=1), perry-codegen lib 209/0, perry-hir lib 246/0, node-suite object module 26/26, existing test_gap_3662_collection_brand_check still 100%. cargo fmt --check clean on touched files.

Validation on the real target

Full pi recompile with this fix: HOME=$(mktemp -d) ./pi-native --version → 0.0.0 rc=0 and --help → the 167-line usage rc=0, both byte-identical (stdout+stderr) to node pi-bundle.mjs — GATE 1 of the pi bring-up reached; no wall #8 on these entry points.

Stretch-goal assessment (gaxios class extends Set losing add, issue family #6232): probed — class A extends Set {} + static new works, direct class-expression new works; what fails is construction through a dynamic path (new exportsObj.X() / new obj.A()): the Map/Set backing init (js_map_set_subclass_init) is emitted by codegen only at static new sites (lower_call/new_helpers.rs::emit_native_instance_base_init), so dynamic-new never installs the backing for ANY native base (Set, Map, Event, EventEmitter, …). That is the full #6232 builtin-subclass-construction feature (class-id→native-base registry + dynamic-new runtime wiring + ctor-arg seeding), not a bounded Set-only fix in this PR's subsystem — documented on #6232 instead.

Stacks on #6656 (branched from fix/6652-global-proto-members; merge that first).

Fixes #6658

https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9

Summary by CodeRabbit

• Bug Fixes
  • Fixed extracted array-like and collection callback handling so an explicitly provided thisArg is preserved across methods like forEach, map, filter, some, every, and the find* family.
  • Improved brand-check TypeError messages for incompatible receivers by including clearer, Node/V8-style receiver rendering.
• Tests
  • Added regression tests ensuring parity with Node/V8 for extracted callback thisArg binding and exact error message text for receiver-related failures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e6cd9dd-ab38-44da-a5eb-8f940226d5a3

📥 Commits

Reviewing files that changed from the base of the PR and between c253e68 and a835ae9.

📒 Files selected for processing (1)

• crates/perry-runtime/src/object/collection_proto_thunks.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/perry-runtime/src/object/collection_proto_thunks.rs

---

📝 Walkthrough

Walkthrough

The runtime preserves explicit thisArg values for extracted array callbacks and renders incompatible collection receivers in brand-check errors. Rust and Node parity tests cover callback families, extracted builtins, and V8-like TypeError messages.

Changes

Builtin callback and receiver semantics

Layer / File(s)	Summary
Explicit callback thisArg dispatch crates/perry-runtime/src/object/native_call_method/handle_methods.rs	Array-like callback methods with an explicit thisArg are routed through the dispatch path that preserves callback binding.
Rendered collection receiver errors crates/perry-runtime/src/object/collection_proto_thunks.rs	Set, Map, and weak collection brand-check errors now include side-effect-free rendering of the incompatible receiver.
Extracted builtin parity coverage crates/perry/tests/issue_6658_extracted_builtin_method_thisarg.rs, test-parity/node-suite/object/extracted-builtin-method-callback-thisarg.ts	Regression tests cover dynamic callback dispatch, extracted builtins, callback families, explicit thisArg binding, and rendered TypeError messages.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ExtractedBuiltin
  participant dispatch_handle
  participant dispatch_arraylike_read_method
  participant Callback
  ExtractedBuiltin->>dispatch_handle: invoke array method with callback and thisArg
  dispatch_handle->>dispatch_arraylike_read_method: forward method and arguments
  dispatch_arraylike_read_method->>Callback: invoke callback with bound thisArg
  Callback-->>ExtractedBuiltin: return iteration result

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main fix: explicit thisArg was dropped in dynamic array-method dispatch.
Description check	✅ Passed	It includes the root cause, fix, related issue, tests, and validation, though it uses custom headings instead of the template sections.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/perry/tests/issue_6652_global_proto_inherited_members.rs (1)

1-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Move to a cargo-test-visible unit test.

As per coding guidelines: crates/*/tests/*.rs: Prefer acceptance coverage in cargo-test-visible unit tests because integration suites under crates/*/tests/*.rs do not run on every PR.

While I recall a learning suggesting that end-to-end regression tests invoking the perry binary via CARGO_BIN_EXE_perry should be kept as integration tests under this directory, the project's coding guidelines explicitly mandate prioritizing cargo-test-visible unit tests to ensure they run on every PR. Please consider moving this test to comply with the guidelines or updating the guidelines if the policy has changed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry/tests/issue_6652_global_proto_inherited_members.rs` around lines
1 - 135, The regression coverage currently lives in the external integration
test file and invokes the perry binary through compile_and_run, so it is not
visible to the required cargo-test unit-test path. Move
object_prototype_inherited_globals_match_node and
runtime_created_global_member_access_matches_node, along with their necessary
helpers, into the appropriate in-crate unit-test module while preserving their
assertions and end-to-end behavior.

Sources: Coding guidelines, Learnings

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/perry-runtime/src/object/collection_proto_thunks.rs`:
- Around line 350-367: In the GC_TYPE_OBJECT branch, root obj with a
RuntimeHandleScope before calling js_string_from_bytes, then reload the receiver
from the rooted handle after allocation before invoking own_data_field_by_name
or reading class_id. Preserve the existing toString override and class-name
result behavior.

---

Outside diff comments:
In `@crates/perry/tests/issue_6652_global_proto_inherited_members.rs`:
- Around line 1-135: The regression coverage currently lives in the external
integration test file and invokes the perry binary through compile_and_run, so
it is not visible to the required cargo-test unit-test path. Move
object_prototype_inherited_globals_match_node and
runtime_created_global_member_access_matches_node, along with their necessary
helpers, into the appropriate in-crate unit-test module while preserving their
assertions and end-to-end behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a745ec42-3b49-4a21-b48d-59d6eb68e12a

📥 Commits

Reviewing files that changed from the base of the PR and between 0583b4e and fbc4038.

📒 Files selected for processing (7)

• crates/perry-hir/src/lower/lower_expr/arm_ident.rs
• crates/perry-runtime/src/object/collection_proto_thunks.rs
• crates/perry-runtime/src/object/native_call_method/handle_methods.rs
• crates/perry/tests/issue_6652_global_proto_inherited_members.rs
• crates/perry/tests/issue_6658_extracted_builtin_method_thisarg.rs
• test-parity/node-suite/globals/global-object-prototype-inherited-members.ts
• test-parity/node-suite/object/extracted-builtin-method-callback-thisarg.ts

[proggeramlug]

Rebased onto main after #6656's squash-merge landed — the branch now carries only the #6658 commit (338f87e); the stacks-on-#6656 note in the description is obsolete.

[proggeramlug]

CodeRabbit's finding fixed in c8-amended head: the receiver is now rooted in a RuntimeHandleScope across the toString-key allocation and the object pointer re-derived through the rewritten handle before own_data_field_by_name/class_id. Verified with cargo check -p perry-runtime.