Conversation
…ength print_array walks an array-like up to its length and, since #33158, skips a run of holes in one step by asking the array's storage for the next present index. That fast path was arrays-only: arguments objects still probed index by index because "their length is small". It is not: an arguments object's length is an ordinary writable property, so console.log / Bun.inspect of one with length = 2 ** 32 (or a getter returning 1e12) never returned, and past 2^32 - 1 the u32 index counter wrapped. Bun__JSArray__nextPresentIndex becomes Bun__JSObject__nextPresentIndex: the same butterfly walk, plus the argument slots that DirectArguments and ScopedArguments keep outside the butterfly (mirroring GenericArgumentsImpl::getOwnPropertyNames). print_array uses it for every hole, and stops probing at MAX_ARRAY_INDEX, past which nothing can be an indexed property, counting the rest of the claimed length as holes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesThe indexed-property binding now supports arbitrary objects and mapped arguments slots. Rust inspection uses the generalized scanner and limits traversal at Indexed-property inspection
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The arguments-object inspection change has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:29 AM PT - Sep 11th, 2026
✅ @robobun, your commit 82a3508f9a55917a6cb1edba526acf2dd3edf83c passed in 🧪 To try this PR locally: bunx bun-pr 42247That installs a local version of the PR into your bun-42247 --bun |
|
Status: ready for review. All review threads are resolved and the self-review is done. Its surviving concerns are addressed (they are listed in the Notes of the PR body). Where this came from (so it can be weighed accordingly)
Reproduce // sloppy script (a DirectArguments); a strict function (ClonedArguments) hangs the same way
const a = new Function("return arguments")(1, 2, 3);
a.length = 2 ** 32;
console.log(a); // 1.4.3: never returns. This branch: [ 1, 2, 3, 4294967293 x empty items ]Verified
Not fixed here / related
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code passed to -e without a require() is loaded as a module, so its functions are strict and all three cases were ClonedArguments. Build the direct and scoped functions with new Function, as the in-process test does, and assert in both tests that the first two kinds are sloppy.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the earlier note about the -e subprocess test only exercising ClonedArguments is now addressed (it uses new Function for the sloppy kinds and asserts isSloppyArguments returns true/true/false, so the test self-verifies it hits all three JSC arguments-object types).
What was reviewed:
Bun__JSObject__nextPresentIndex: butterfly walk unchanged in behavior,uncheckedDowncastguarded byobject->type()switch,std::minof mapped-argument slots and butterfly matches howGenericArgumentsImpl::getOwnPropertyNamesenumerates the same two sources; nothing here allocates, throws, or enters JS, so no ThrowScope needed.print_arrayloop:i > MAX_ARRAY_INDEXguard runs beforeget_direct_index/next_present_index(i + 1), so theu32index cannot wrap even whenlenis 2^50.- Tests pin exact output for delete/defineProperty/length-shrink/index-past-args across all three kinds, and the huge-length subprocess case drains pipes concurrently and asserts the combined
{stdout, stderr, exitCode}.
Extended reasoning...
Overview
The PR generalizes the sparse-array hole-skipping optimization in Bun.inspect/console.log so it also applies to arguments objects, whose length is a plain writable property that can be set arbitrarily large with only a few backing elements. On the C++ side, Bun__JSArray__nextPresentIndex is renamed to Bun__JSObject__nextPresentIndex, the existing butterfly walk is extracted into a static helper, and a small template walks internalLength()/isMappedArgument() for DirectArguments/ScopedArguments (which store argument slots outside the butterfly). The result is std::min of the two sources — exactly the pair GenericArgumentsImpl::getOwnPropertyNames enumerates. On the Rust side, next_present_index now accepts any object, print_array drops the is_array() special-case, and a MAX_ARRAY_INDEX guard stops the loop before the u32 index can overflow. A new MAX_ARRAY_INDEX constant is added to src/jsc/lib.rs.
Security risks
None. The change is read-only traversal of JSC-managed storage during formatting; it does not allocate, does not call anything that can throw or re-enter JS, and holds no pointers across GC. The uncheckedDowncast calls are gated on the exact JSType tag, which is the standard JSC pattern. The added bound check strictly reduces the work done on adversarial input (huge length).
Level of scrutiny
Moderate. This is native JSC-binding code, but it is a focused generalization of an existing, already-reviewed function (from #33158) rather than a new mechanism. The butterfly-walk logic is byte-for-byte the same modulo the JSArray* → JSObject* receiver widening; the new arguments-slot walk is four lines mirroring a named JSC function. I traced the u32 overflow concern: the i > MAX_ARRAY_INDEX check fires at i == u32::MAX, which is the only value reachable past 0xFFFF_FFFE (via i += 1 after processing the element at MAX_ARRAY_INDEX); next_present_index(i + 1) is only reached with i <= MAX_ARRAY_INDEX, so i + 1 never wraps.
Other factors
My one prior inline comment (the -e subprocess test ran as a module, so all three "kinds" were ClonedArguments) was addressed in the follow-up commit: the subprocess now builds direct/scoped via new Function(...) and bakes an isSloppyArguments assertion into the expected stdout (true/true/false), so the test would fail if the kinds ever collapsed again. Test coverage is thorough across the variant matrix REVIEW.md asks for — all three arguments kinds × delete/defineProperty/length-shrink/length-grow/index-past-args, plus the huge-length cases (2^32 and a 2^50 getter) in a subprocess with concurrent pipe draining and a combined {stdout, stderr, exitCode} assertion. No CODEOWNERS entry covers these paths, and there are no outstanding third-party review objections.
…ts shapes The two new rows make the argument-slot walk step over consecutive deleted arguments, and put a re-assigned index (regular indexed storage) between a hole and an argument that is still mapped, so both sources of the minimum in Bun__JSObject__nextPresentIndex are exercised.
This branch has not been deployed
2 participants
Problem
console.log/Bun.inspectof anargumentsobject with a hugelengthnever returns:const a = (function () { return arguments; })(1, 2, 3); a.length = 2 ** 32; console.log(a).print_array(src/jsc/ConsoleObject.rs:4307) probes every index up toget_length(), which for an arguments object is its writablelengthproperty. The hole-skipping walk from inspect: don't probe every hole when printing a sparse array #33158 wasis_array()only. Past 2^32 - 1 theu32index also overflowed.Fix
Bun__JSArray__nextPresentIndexbecomesBun__JSObject__nextPresentIndex: the same butterfly walk, plus the argument slots thatDirectArguments/ScopedArgumentskeep outside the butterfly.print_arrayuses it for arguments objects too.MAX_ARRAY_INDEX. Nothing above it is an indexed property, so the rest oflengthcounts as holes and the index cannot overflow.test/js/bun/util/inspect.test.js(the child-process test times out on 1.4.3). Alsotest/js/bun/console/,test/js/web/console/. Self-reviewed: all surviving concerns addressed, see Notes.Background
DirectArguments(sloppy function) stores the arguments inline.ScopedArguments(a closure captures a parameter) aliases them into the scope.ClonedArguments(strict function) keeps them in the butterfly.ArrayStoragemode.MAX_ARRAY_INDEXis 2^32 - 2. A larger integer key is a string-named property. Only an arguments object'slengthcan exceed it.Notes
Scope and what is left
a.length = 1e9takes ~13 s on 1.4.3. Alengthgetter returning1e12,console.log({ a }), and anErrorwith such a property all hang the same way. They all reachprint_array. Node prints[Arguments] { '0': 1, '1': 2, '2': 3 }immediately for the same object (it never readslengthfor an arguments object).console.table/Bun.inspect.tableof the same object is not fixed here. That path does not go throughprint_array: it builds one row per claimed index (1e6 rows in 630 ms on 1.4.3), so it still does not return forlength = 2 ** 32. console.table: walk an array by its own properties instead of trusting length #41119 (open) owns that path.ConsoleObject.rs:5233) also iterates up toget_length. It prints every child, so its cost is proportional to its output. Withlength > 2 ** 32it ends inpanic: int cast: TryFromIntError(PosOverflow), but only after 2^32 iterations (about 9 minutes). Pre-existing, left alone to keep this PR to one loop.MAX_ARRAY_INDEX(for examplea["4294967295"]withlength = 2 ** 33) is counted as a hole, not printed. Arrays can never have one below theirlength. For arguments objects the old loop read it once and then wrapped the counter.How the walk works
getDirectIndexon aGenericArgumentsImplconsults the mapped argument slots (isMappedArgument(i)fori < internalLength()) and then the ordinary butterfly path. So "next present index" is the minimum over both, the same two sourcesGenericArgumentsImpl::getOwnPropertyNamesenumerates.ClonedArgumentsneeds nothing special: its elements are butterfly elements.next_present_indexcan scan the whole sparse map on each call.print_arraybounds the number of calls (it stops after 100 present elements). A caller that wants a full walk should snapshot and sort the keys instead, asJSObject::getOwnIndexedPropertyNamesdoes. The doc comment says so.Evidence
DirectArguments/ScopedArguments/ClonedArguments, three distinct JSC structures):lengthof 2^32, 1e9, 2^32 ± k, 2^53,Infinity, a 2^50 getter, and an element at index 2^32 - 2 under a 2^33lengthall format in 2 ms or less.-1andNaNgive[]as before.definePropertywith accessor and data descriptors,lengthup to 5000,freeze/seal/preventExtensions) print byte-identical output on 1.4.3 and on this branch.Related open PRs
selftothison theget_direct_indexline inside this loop, adjacent to this diff. Whichever lands second needs a one-line manual merge.array_layoutpre-pass that carries a copy of the oldis_array()gate and au32index. It merges cleanly over this PR and would probe every index of an arguments object again. It needs the same two changes. I left a note there.get_lengthfor an object with nolengthproperty (delete arguments.length) and forlength = Infinity. It has had no review activity since 2026-08-15 and this PR does not depend on it. With this PR alone those two cases no longer hang inprint_array(they print the clamped hole count).... 1 more items, 6 x empty itemsfor a plain 106-element array on 1.4.3). Pre-existing, not touched here, reported separately.Self-review
next_present_indexwas undocumented (doc comment), and the Related list above was incomplete (fixed). The child test at first only producedClonedArgumentsbecause-ecode is a strict module (fixed withnew Function, and both tests now assert which kinds are sloppy).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js