inspect: print symbol keys after string keys on the fast property walk

[robobun]

Problem

• console.log / Bun.inspect print symbol-keyed properties in a position that depends on which property walk the formatter happens to take:

  const s = Symbol("s");
  console.log({ [s]: 1, a: 2 });                      // { [Symbol(s)]: 1, a: 2 }
  console.log({ [s]: 1, a: 2, get g() { return 1 } }); // { a: 2, g: [Getter], [Symbol(s)]: 1 }

• Node prints { a: 2, Symbol(s): 1 } for both: string keys in insertion order, then symbols.
• Cause: JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp) has two walks. The fast walk iterates structure->forEachProperty(...), which yields the property table in insertion order with symbols interleaved. The slow walk (taken when the object has an accessor, indexed properties, etc.) uses getOwnPropertyNames, which lists string keys first and symbols afterwards. The same inconsistency shows up on every level of the fast walk's prototype chain visit.

Fix

• The fast walk now collects its snapshot in two passes over the property table: string keys, then (only if one was seen) symbol keys. The filter/snapshot logic is unchanged and shared by both passes; the emit loop is untouched.
• Correct because it is the order the spec defines for OrdinaryOwnPropertyKeys, the order the slow walk already produces, and the order node's util.inspect prints; JSC's own Structure::getPropertyNamesFromStructure uses the identical two-pass scheme to get there. Symbols keep their insertion order among themselves; the set of printed properties does not change.
• Cost: one isSymbol() flag check per property for objects without symbol keys; a second read-only table walk only when the object has one.
• Verified with test/js/bun/util/inspect.test.js ("symbol keys print after string keys"): each case is formatted through both walks (the second time with an added getter) and must match the expected string. Covers symbol first / in the middle / several symbols / only symbols, class fields, nested objects, and both prototype-walk shapes of the fast path (own + prototype properties, and an object with no own properties). 8 of the 9 cases fail on the current release; all pass with this change.
• Also ran test/js/web/console, test/js/bun/console, test/js/node/util/bun-inspect.test.ts, test/js/node/util/custom-inspect.test.js: no changes in behaviour. No existing expectation depended on the old interleaved order.

Background

• OrdinaryOwnPropertyKeys is the spec order used by Reflect.ownKeys, getOwnPropertyNames + getOwnPropertySymbols, and node's util.inspect: integer-like keys ascending, then string keys in insertion order, then symbol keys in insertion order.
• A JSC Structure is the object's hidden class; its property table stores every named property (strings and symbols alike) in insertion order. getOwnPropertyNames reorders that table into spec order; walking the table directly does not.
• The fast walk in forEachPropertyImpl is only used when the structure has no accessors, no indexed properties and no custom property lookup, so integer-like keys never reach it (they live in indexed storage and force the slow walk); string-vs-symbol ordering was the only difference between the two walks.
• The only caller of this walk is the console.log / Bun.inspect formatter (ConsoleObject.rs). Bun.inspect(..., { sorted: true }) and bun:test's diff/snapshot printer use forEachPropertyOrdered, which sorts by name and is unaffected. node:util.inspect is implemented in JS and was already correct.

[robobun]

^{Updated 5:58 AM PT - Aug 15th, 2026}

@robobun, your commit 5ccd345 is building: #98105

[robobun]

Status: reproduced on bun 1.4.0 with Bun.inspect({ [Symbol("s")]: 1, a: 2 }) printing the symbol first (the getter twin prints it last); 8 of the 9 new cases in test/js/bun/util/inspect.test.js fail on the release build and pass with this branch's debug build. Fix is in src/jsc/bindings/bindings.cpp (JSC__JSValue__forEachPropertyImpl, fast walk only). Waiting on CI.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 71248a6a-cdc1-470e-b74c-ed900885c344

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and 5ccd345.

📒 Files selected for processing (2)

• src/jsc/bindings/bindings.cpp
• test/js/bun/util/inspect.test.js

---

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

[claude]

LGTM — small, well-scoped ordering fix that makes the fast property walk match the slow path and Node's util.inspect.

What was reviewed:

• The two-pass structure->forEachProperty walk: both passes are pure reads over the property table with no JS execution between them, so the snapshot invariant (no user code mid-collect) still holds; hasSymbolKeys/snapshot/snapshotValues are block-scoped so the goto restart prototype loop re-initializes them correctly.
• Confirmed JSC__JSValue__forEachProperty is only reached from the Bun.inspect/console.log formatter (ConsoleObject.rs), so this only affects display ordering.
• Tests cover both walks per case (plain object + same object with a getter), symbol first/middle/multiple/only, class fields, nested, and both prototype-walk shapes; assertions are exact toBe on the formatted string.

Extended reasoning...

Overview

The PR changes the fast property walk in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp) so that string keys are snapshotted before symbol keys, matching OrdinaryOwnPropertyKeys order. Previously the fast walk emitted keys in raw insertion order (symbols interleaved with strings) while the slow walk (accessors, indexed properties) already produced strings-then-symbols via getOwnPropertyNames, so Bun.inspect output depended on which path was taken. The fix extracts the existing per-entry filter/snapshot body into a lambda and calls structure->forEachProperty twice — first collecting non-symbols, then (only if any were seen) symbols. A new describe("symbol keys print after string keys") block in test/js/bun/util/inspect.test.js covers 9 shapes, each asserted through both the fast and slow walks.

Security risks

None. This is display-ordering only in the inspect formatter; no parsing of untrusted input, no auth/crypto/permissions surface, and no change to which properties are printed — only their relative order.

Level of scrutiny

Low-to-medium. The mechanical change is ~20 lines: hoist the loop body into a lambda, add a symbol-skip first pass and a symbol-only second pass. The surrounding code is memory-safety-sensitive (the snapshot exists precisely to avoid running user JS while iterating the property table), so I checked that the new arrangement preserves that invariant: both forEachProperty calls only read entry.key()/attributes()/offset() and append to local containers; no JS executes until the emit loop, which is unchanged. The two calls iterate the same immutable property table (JSC's own Structure::getPropertyNamesFromStructure uses the identical two-pass scheme). All new locals live inside the if (fast) block, so the existing goto restart prototype-chain loop re-initializes them cleanly on each level.

Other factors

• Sole caller of this FFI entry point is the console/inspect formatter (src/jsc/ConsoleObject.rs:5455), verified by grep — forEachPropertyOrdered (sorted output, snapshot printer) and forEachPropertyNonIndexed are separate exports and unaffected.
• Test coverage is strong: exact-string toBe assertions, it.each matrix, and each case is exercised through both the fast walk and the slow walk (by defining a getter) so future divergence between the two paths would fail immediately. Tests live in the existing inspect.test.js next to related symbol/getter coverage.
• Cost is one isSymbol() bit check per property when no symbols are present, and one extra table walk when they are — negligible for a formatter path.
• No prior human reviews or outstanding comments to address.