Repository navigation
jsc: finish arguments_old -> arguments_as_array migration and delete arguments_old - #35326
Conversation
…arguments_old Migrates the remaining 267 arguments_old::<N>() call sites across 82 files to the modern CallFrame accessors (arguments_as_array, arguments, arguments_count), then deletes arguments_old and the now-unused Arguments::init (ZERO-padding) constructor so the legacy accessor cannot come back. The half-migrated state left both idioms standing side by side in the same files (e.g. src/runtime/node/node_net_binding.rs used both a screen apart), hiding which accessor was canonical. This collapses to one. Mapping applied per site: .slice() -> frame.arguments() .len -> frame.arguments_count() .ptr[k] -> arguments_as_array::<N>()[k] (destructured) .ptr[len-1] -> frame.arguments()[len()-1] mutable .ptr[..] -> let mut a = arguments_as_array::<N>(); &mut a[..] ZERO-vs-UNDEFINED padding: arguments_old padded unpassed slots with JSValue::ZERO; arguments_as_array pads with JSValue::UNDEFINED. Every unguarded slot read was audited against its downstream predicate; the four sites where the predicate distinguishes ZERO from UNDEFINED were adjusted to preserve identical JS-visible behavior (see PR body for the list).
Locks in the boundary behavior at the four sites whose unpassed-slot predicate was adjusted in the arguments_old migration (is_empty-only checks in crc32; is_empty||is_undefined collapse in ServerWebSocket close).
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (85)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits, I spot-checked several of the higher-risk transforms and they hold: udp_socket.rs js_send/js_send_many (the != N → < N/>= N rewrites are equivalent under the old cap semantics), Blob::get_slice (padded slots still fail is_number()/is_string() identically under UNDEFINED vs ZERO), CryptoHasher::update (encoding_value.is_cell() is false for both paddings), and the args[args.len()-1] promise-callback sites in Body/Blob/RequestContext (fixed-arity internal callbacks, so uncapping is transparent). Deferring to a human given the 82-file blast radius across core runtime paths.
Extended reasoning...
Overview
Mechanical migration finishing the CallFrame::arguments_old → arguments_as_array/arguments()/arguments_count() transition: 267 call sites across 82 files, plus deletion of arguments_old and Arguments::init. The one semantic axis is padding — arguments_old filled unpassed slots with JSValue::ZERO, arguments_as_array uses JSValue::UNDEFINED — and the PR description enumerates every site where that distinction matters, with the four sites that needed predicate adjustments (crc32 args 0/1, ServerWebSocket::close args 0/1) and the intentionally-left-dead is_empty() in parse_compress_arg.
Security risks
None identified. This is an internal accessor refactor; no new inputs are accepted, no validation is loosened, no security-gated paths (TLS verify, rejectUnauthorized, path parsing) change shape. The udp_socket != N → < N/>= N rewrites preserve the same accept/reject partition under the old cap-at-N semantics.
Level of scrutiny
High — not because any individual edit is complex, but because the change fans out across essentially every JS-visible native entry point (fetch, Bun.serve, sockets, crypto, HTTP/2, DNS, subprocess, WebSocket, S3, test runner matchers). A single mis-mapped padding predicate could silently change error-vs-accept behavior on an edge-case argument shape. The PR description's audit is unusually thorough (per-site padding table, non-obvious-equivalence table, cross-target build + test-suite verification), and the bug hunter surfaced only two nits (a pre-existing dead check and a test line that certifies pre-existing Node divergence). I independently traced the args.len cap-removal sites in udp_socket.rs and ServerWebSocket::publish* and confirmed the > 1/< N/>= N predicates are unaffected by uncapping.
Other factors
The two inline findings are non-blocking nits. New tests cover the two adjusted-predicate sites (ws.close(undefined, undefined) and crc32 missing/undefined args). Given the sheer breadth — 82 files spanning the entire runtime surface — and REVIEW.md's "treat every refactor as guilty until proven behavior-preserving" bar, this warrants a human pass over the padding-sensitive table and CI results even though I found nothing wrong.
…-divergent behavior
There was a problem hiding this comment.
No bugs found; both earlier nits are resolved (the Node-divergent crc32(data, undefined) assertion was dropped in 489bce9, and the pre-existing dead set_ref count check is intentionally left behavior-preserving). Deferring only because of scope: 267 call sites across 85 files rewrite argument handling for essentially every user-facing API surface (fetch, Bun.serve, ServerWebSocket, subprocess, crypto, S3, HTTP/2, sockets, expect matchers), and the ZERO→UNDEFINED padding shift is subtle enough that a human skim of the four adjusted-predicate sites and the args.len → arguments_count() conversions is worth the time.
What was reviewed:
- Padding-sensitive sites in
node_zlib_binding.rs(crc32) andServerWebSocket::close— theis_empty()→arguments_count() < N/is_undefined()rewrites preserve the old ZERO-detection semantics exactly. udp_socket.rssend()/send_many()— theargs.len == N(capped) →arguments_count() >= N/< N(uncapped) rewrites trace to identical branch outcomes for all arg counts including >N.Blob::get_sliceand theargs[args.len()-1]promise-context callbacks inBody.rs/RequestContext.rs/Blob.rs— UNDEFINED padding vs ZERO reaches the same.is_number()/.is_string()false path; last-arg indexing is only used from internal fixed-arity callbacks.Bun.inspectprotect/unprotect now covers all passed args instead of the first 4 — extra roots are transparent;register_macro's!= 2→< 2is exact per the old capped-len semantics.
Extended reasoning...
Overview
Mechanical migration finishing the deprecated CallFrame::arguments_old::<N>() → arguments_as_array::<N>() / arguments() transition across 82 Rust files (267 call sites), deleting arguments_old and Arguments::init from src/jsc/CallFrame.rs, plus two small test additions covering the crc32() and ws.close(undefined, undefined) argument paths. Net -277 lines. The PR description includes a per-site mapping table and an explicit audit of the four sites where the ZERO→UNDEFINED padding difference is observable, each rewritten to preserve identical JS-visible behavior.
Security risks
None identified. This is an internal argument-accessor refactor; no new user input is parsed, no validation is loosened, and no security-sensitive predicates (auth, TLS, path traversal) change. The one class of risk — a missed is_empty()/.is_undefined_or_null() predicate on a padded slot silently changing behavior — was the focus of the audit and my spot checks; I found no unhandled instances.
Level of scrutiny
High, purely because of breadth. Each individual hunk is trivially reviewable (two-line accessor swap), but the change touches the argument-decode prologue of nearly every JS-callable native function in the runtime. A single mis-converted site could surface as a subtle behavior change in a rarely-exercised overload. The PR description's audit is thorough and my spot checks of the non-obvious conversions (udp_socket::send's capped-vs-uncapped len comparisons, Blob::get_slice's mutable 3-slot shuffle, inspect's protect loop, register_macro's != 2 rewrite, the args[len-1] promise-context callbacks) all check out — but a maintainer skim is appropriate at this scale.
Other factors
Both of my prior inline comments are resolved: the crc32("hello", undefined) assertion that certified Node-divergent behavior was dropped in 489bce9; the dead set_ref arguments.len() == 0 check is intentionally left as-is to keep the refactor strictly behavior-preserving (fixing it would change the 0-arg error code). CI build #79027 was still running at last timeline update. arguments_undef and its 17 callers are explicitly out of scope. The added ws.close(undefined, undefined) and crc32 missing-arg tests directly cover two of the four adjusted-predicate sites.
There was a problem hiding this comment.
Thanks for addressing the feedback — the Node-divergent crc32(data, undefined) assertion is dropped and the set_ref dead-check rationale makes sense for a behavior-preserving refactor.
I didn't find any bugs on this pass, but with 267 sites across 85 files spanning fetch/serve/sockets/expect and a ZERO→UNDEFINED padding-semantics audit in play, this is worth a human look before landing.
What was reviewed:
- Padding-sensitive predicate sites (
crc32,ws.close,parse_compress_arg) — the four documented rewrites preserve behavior; new tests cover the adjusted paths. udp_socket send/send_manycapped-len →arguments_count()conversions — the!= N→< N/>= Nrewrites are equivalent under the old cap.Blob::get_slicemutable-array path andBun.inspectprotect/unprotect scope change — both trace through cleanly.register_macro!= 2→< 2andpublish*<4>→<3>reductions match the PR's stated equivalences.
Extended reasoning...
Overview
Finishes the arguments_old → arguments_as_array/arguments() migration: 267 call sites across 82 Rust files plus deletion of CallFrame::arguments_old and Arguments::init, with two small test additions covering the four predicate-adjusted sites. Net −277 lines. The transformation is mechanical per the mapping table, but the ZERO→UNDEFINED padding change means every unguarded slot read had to be audited against its downstream predicate.
Security risks
None identified. This is an internal call-frame accessor migration; no new inputs are trusted, no auth/crypto/permission code paths change semantics.
Level of scrutiny
High — not because any individual site is complex, but because the diff touches essentially every JS-visible native entry point (fetch, Bun.serve, ServerWebSocket, sockets/TLS, h2, dns, expect matchers, Blob/Response/Request, S3, subprocess, transpiler). A single missed padding-sensitive predicate would surface as a user-visible behavior change on an unpassed-argument path, which is exactly the kind of edge case that slips past test suites. The PR description's per-site audit is thorough and the four adjusted sites are correctly reasoned, but independently verifying all 267 sites is beyond what an automated pass can guarantee.
Other factors
- My two prior inline nits are resolved: the Node-divergent
crc32("hello", undefined)test assertion was dropped (489bce9), and the pre-existing deadset_refcheck was reasonably left as-is to keep the refactor strictly behavior-preserving. - I spot-checked several of the non-obvious equivalences beyond the four documented ones (
udp_socket::send/send_manycap removal,Blob::get_slicefixed-array mutation,Bun.inspectfull-slice protect,ini_jscunguarded index) and found them correct. - The bug-hunting system found nothing on this pass.
- Build/test verification in the description looks solid (
rust:check-all× 10 targets, targeted suite runs,rg arguments_oldempty).
Given the breadth across critical runtime surface, deferring to a human reviewer.
|
CI on build #79035: the only hard-red test is
|
…ing arguments_count()
| let arguments = frame.arguments(); | ||
|
|
||
| if arguments.len() < 1 { |
There was a problem hiding this comment.
🟣 Pre-existing: expect(x).toBeWithin(3) (one numeric arg) panics with a Rust slice index-out-of-bounds — the guard at line 21 is arguments.len() < 1 but line 36 unconditionally reads arguments[1]. The old arguments_old::<2>().slice() also had length 1 with one passed arg and panicked identically, so this PR faithfully preserved behavior; flagging as a follow-up (guard should be < 2 to match its own "requires 2 arguments" message) since you already declined a similar out-of-scope fix in node_cluster_binding.rs.
Extended reasoning...
What the bug is
to_be_within at src/runtime/test_runner/expect/toBeWithin.rs:21 guards the argument count with if arguments.len() < 1, but the body unconditionally indexes arguments[1] at line 36. frame.arguments() returns a &[JSValue] sized to the actual passed-argument count, so calling expect(x).toBeWithin(3) — one argument, missing end — reaches line 36 with a 1-element slice and panics with a Rust slice index-out-of-bounds. Per REVIEW.md ("user-reachable failures are recoverable errors, never panics"), this is a bug: a matcher misuse should throw a catchable JS error, not crash the process.
Step-by-step proof
Call expect(5).toBeWithin(3):
frame.arguments()returns&[JSValue::from(3)]— length 1.- Line 21:
arguments.len() < 1→1 < 1→ false. The "requires 2 arguments" guard is skipped. - Line 27:
start_value = arguments[0]→3. - Line 30:
!start_value.is_number()→!true→ false. The type check passes. - Line 36:
let end_value = arguments[1]→ panic (index out of bounds: the len is 1 but the index is 1).
The intended behavior — the throw_invalid_arguments("toBeWithin() requires 2 arguments") at line 22 — is unreachable on this path because the guard's threshold is off by one.
Why this is pre-existing (not introduced by this PR)
The old code was:
let _arguments = frame.arguments_old::<2>();
let arguments = _arguments.slice();Arguments::<MAX>::slice() returns &self.ptr[0..self.len] where len = min(passed, MAX) (verified at the deleted arguments_old in the diff and Arguments::slice() at CallFrame.rs). With 1 passed arg and MAX=2, len = 1, so the old arguments slice also had length 1 and arguments[1] panicked identically. This PR's mechanical args.slice() → frame.arguments() mapping is exact here — no behavior change.
Why flag it on this PR anyway
REVIEW.md calls out user-reachable panics as bugs, and the guard's own error message already says "requires 2 arguments", so the intended check is clearly < 2. However, the PR's explicit rule is identical argument handling everywhere — the maintainer already declined a similar out-of-scope fix in node_cluster_binding.rs on this PR ("a JS-visible behavior change and this PR's rule is identical argument handling everywhere. Happy to fix it in a follow-up"). The same reasoning applies here: changing < 1 to < 2 converts a panic into a thrown JS error, which is a user-visible behavior change (albeit strictly an improvement) and outside the scope of a behavior-preserving refactor.
Fix (for the follow-up)
if arguments.len() < 2 {
return Err(global.throw_invalid_arguments(format_args!(
"toBeWithin() requires 2 arguments"
)));
}Not blocking this PR.
Brings in #35002 (remove ~39k lines of dead Rust) and its follow-ups (#35019, #35052, #35152, #35225, #35293, #35326). The binary-size check compares against current main; this branch was 137 commits behind, so it still carried the dead code main dropped and registered as +630KB..+1.7MB on x64 while aarch64 linux showed -513KB/-601KB (different dead-code elimination outcomes per target). This stack's own native contribution is 19 files / +392 -45 lines; src/js is net -977 lines (domain.ts +692 vs fast-utf8-stream.ts -856 etc). Also: drop the hoisted pbkdf2 .bind handlers back to closures (review nit), and take main's expectations.txt since #34741 audited the stale ASAN entries.
2 participants
Finishes the
arguments_old→arguments_as_arraymigration across the runtime and deletesarguments_olditself so the legacy accessor cannot come back.What changed
CallFrame::arguments_old::<N>()was the legacy JS-argument accessor (the_oldsuffix says as much). Its replacementarguments_as_array::<N>()already existed and was in use, leaving both idioms standing side by side in the same files (e.g.src/runtime/node/node_net_binding.rsused both a screen apart). This collapses to one:arguments_oldcall sites migrated across 82 filesCallFrame::arguments_olddeleted fromsrc/jsc/CallFrame.rsArguments::<N>::init(the ZERO-padding constructor, now unused) deleted+539 / -816Per-site mapping:
args.slice()frame.arguments()args.lenframe.arguments_count()args.ptr[k]let [..] = frame.arguments_as_array::<N>()args.ptr[args.len - 1]frame.arguments()[frame.arguments().len() - 1]&mut args.ptr[..]let mut a = frame.arguments_as_array::<N>(); &mut a[..]arguments_undef(17 callers, separate accessor) and theArguments<N>struct it returns are left in place; they are out of scope here.Padding-sensitive sites (reviewable list)
arguments_oldpadded unpassed slots withJSValue::ZERO;arguments_as_arraypads withJSValue::UNDEFINED. Every unguarded slot read was audited against its downstream predicate. The vast majority use predicates that treat both identically (is_empty_or_undefined_or_null,to_boolean,is_string/is_number/is_object/is_cell/is_callable,as_array_buffer), or guard the read with a preceding count check, or only read via.slice()which never exposes padding.Four predicate sites distinguished ZERO from UNDEFINED and were adjusted to preserve identical JS-visible behavior:
src/runtime/node/node_zlib_binding.rscrc32()arg 0data.is_empty()callframe.arguments_count() < 1is_empty()detected the ZERO-padded unpassed slot; replaced with the explicit count check it was standing in forsrc/runtime/node/node_zlib_binding.rscrc32()arg 1value.is_empty()callframe.arguments_count() < 2src/runtime/server/ServerWebSocket.rsclose()arg 0 (code)args.ptr[0].is_empty() || args.ptr[0].is_undefined()args[0].is_undefined()undefined; with UNDEFINED padding the singleis_undefined()covers bothsrc/runtime/server/ServerWebSocket.rsclose()arg 1 (message)args.ptr[1].is_empty() || args.ptr[1].is_undefined()args[1].is_undefined()No unguarded
is_undefined_or_null()sites existed on potentially-unpassed slots (grep of the full diff confirms).Two checks that are now dead (always short-circuited earlier) but left in place to keep the diff minimal:
ServerWebSocket::parse_compress_arg's!compress_value.is_empty()branch (the preceding!is_undefined()now catches the unpassed case).Non-obvious equivalences worth noting
register_macroinBunObject.rs:arguments.len() != 2becamearguments.len() < 2. The old.slice().len()wasmin(passed, 2), so!= 2was already< 2; the rewrite is exact.Bun.inspectinBunObject.rs: the old code snapshotted the first 4 args into a stack array for protect/unprotect; the new code protects/unprotects the full passed-args slice (&[JSValue]borrowed from the call frame, still no allocation).inspectonly readsarguments[0]andarguments[1], so output is unchanged; the extra GC roots on args 5+ are transparent.ServerWebSocket::publish*:arguments_old::<4>()becamearguments_as_array::<3>()since only indices 0..2 are read;args.len(capped at 4) becamearguments_count()(uncapped).parse_compress_argonly testsargs_len > 1, which is unaffected by the cap.Verification
bun bdbuilds clean on the first passbun run rust:check-all: all 10 targets passmainunder the same containercrc32()andws.close()spot-checked againstmainfor identical output on the 0-arg / 1-arg / 2-arg pathsrg arguments_old --type rustreturns nothingno test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server.test.ts