fix(array): route collection reads before array validation - #8061
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesMap and Set indexed-read fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change routes Map/Set indexed reads before strict array validation while preserving rejection of non-array inputs through the array-only path; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
proggeramlug
marked this pull request as ready for review
August 14, 2026 00:25
Contributor
Author
|
@coderabbitai review |
|
proggeramlug
added a commit
that referenced
this pull request
Aug 15, 2026
…the fused forEach (#8117) (#8130) * fix(array): route a Map/Set receiver before the array-only funnel in the fused forEach Codegen fuses a 1-argument `<expr>.forEach(cb)` to the ARRAY entry point `js_array_forEach` whenever it cannot prove the receiver is a collection — `obj.someSet.forEach(cb)` is the ordinary shape, and it is the shape react-server-dom uses for `request.abortableTasks`. #5989 put a Set/Map reroute inside that helper, but placed it AFTER `normalize_array_receiver` and its `if arr.is_null() { return; }` early-out. #8041 then widened `clean_arr_ptr` — the funnel `normalize_array_receiver` ends in — from "reject GC_TYPE_OBJECT / GC_TYPE_CLOSURE" to "reject every tracked non-array". That is correct for the array-layout question, but it nulls a GC_TYPE_SET / GC_TYPE_MAP receiver, so the reroute became unreachable and every fused `set.forEach(cb)` / `map.forEach(cb)` silently iterated nothing. Not a crash: an empty result where node yields elements. Hoist the reroute into `collection_foreach_reroute`, called as the first statement of `js_array_forEach`. Gated on `array_receiver_gc_tag` (the #7765 idiom `js_array_get_f64` already uses), so an ordinary array is excluded by one already-warm header byte and never reaches a registry probe; the registry stays the liveness/layout proof. Same ordering fix #8060/#8061 applied to the indexed read and #8090/#8119/#8109/#8120 applied to the typed-array questions. The 2-argument form `<expr>.forEach(cb, thisArg)` lowers to `js_arraylike_forEach`, which already reroutes before any array validation, and was never affected — which is why only the 1-arg lines of the two gap tests were red. Fixes the two `pass -> parity_fail` entries catalogued in #8117: `test_gap_collection_foreach_member_receiver_thisarg` and `test_gap_set_map_foreach_fused_receiver`. Both reproduce standalone and are now byte-identical to node v26.5.1 with exit 0. Tests: three added to `array/collection_tag_tests.rs`, sabotage-verified twice. Restoring the pre-fix ordering fails the Set/Map cases with `left: []` — the exact production symptom — while the plain-array control stays green; deleting the receiver-tag gate fails the control on the registry probe counters (`left: (3, 3) right: (2, 2)`) while the Set/Map cases stay green. * docs(changelog): note the fused forEach collection reroute fix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
clean_arr_ptr's strict array-only validationFixes #8060.
Causal evidence
Exact base:
ef6e111a4fb85dcc9fd81f69c30bf57a0bf40a61.#8041 made
clean_arr_ptrreject every tracked non-array, butjs_array_get_f64reached its Map/Set branches only after that rejection. Six pre-existing tests failed individually withNaNinstead of collection values.js_array_lengthwas the positive control: it already consulted the managed tag and authoritative registry before cleaning, and returned the correct sizes for the same receivers.Local validation
Using isolated worktree and target:
cargo check -p perry-runtime --libcargo fmt --all --checkgit diff --checkOne full default-parallel runtime suite after the fix reported 2,313 passed, 1 failed, 4 ignored. The sole failure was
promise::keyed_table::tests::settling_many_keys_is_not_quadratic, the independently diagnosed wall-clock assertion replaced by #8059; all six #8060 tests and the shadow-root witness passed.Summary by CodeRabbit
Bug Fixes
MapandSetvalues so they no longer incorrectly returnNaN.undefined.Tests
MapandSetindexing behavior, pointer validation, registry-backed reads, and collection size reporting.Documentation