console,util: route every %s through one formatPercentS (Node's rule) - #36141
console,util: route every %s through one formatPercentS (Node's rule)#36141robobun wants to merge 12 commits into
Conversation
Bun shipped three distinct %s behaviors:
util.format('%s', v) Node's ported decision tree
new Console(stream).log('%s', v) same (JS formatWithOptions)
console.log('%s', v) native engine ToString(v)
The native global console threw on Symbol, lost -0 and the BigInt 'n'
suffix, and printed [object Object]/[object Map] for plain values. On
the util.format side, hasBuiltInToString classified Buffer, URL and
URLSearchParams as language built-ins because builtInObjects was scraped
from globalThis after Bun had already installed every host global.
Changes:
inspect.js: builtInObjects is now the hardcoded ECMA-262 constructor
names (what Node observes at bootstrap). The case-115 body is hoisted
into formatPercentS(inspectOptions, arg) and exported.
UtilInspect.cpp / ZigGlobalObject: a LazyProperty caches formatPercentS
from internal/util/inspect; Bun__callFormatPercentS calls it.
ConsoleObject.rs: PercentTag::S keeps the primitive-string fast path
and delegates every other value to that one JS routine, writing the
returned string (no ANSI). One source of truth for all three paths.
|
Updated 4:01 PM PT - Jul 27th, 2026
❌ @robobun, your commit cfdcf76 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36141That installs a local version of the PR into your bun-36141 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 14 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 (1)
WalkthroughChangesNode-style Percent-s formatting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
On the duplicate check: #33462's The |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Drop the language globals Node itself does not see at bootstrap
(SharedArrayBuffer, Float16Array, DisposableStack, AsyncDisposableStack,
SuppressedError) and add back Infinity/NaN. Node's set is what shows up
on globalThis before V8's post-bootstrap globals and before Node/Web
globals are installed; freezing to a live scrape would observably
diverge (showHidden on Float16Array walks into its prototype in Node
but not for the other typed arrays).
Also:
- drop Float16Array from the typed-array showHidden loops in
util-inspect.test.js (upstream test-util-inspect.js never had it) and
give that test body a debug-build timeout: its surrogate-pair loop
runs ~10k inspects and already exceeds 5s on bun bd with or without
this change
- remove the stale print_string comment in ConsoleObject.rs now that
%s no longer routes through it
- add the remaining throw-face cases to the %s unification test:
[Symbol], {toString:null}, revoked Proxy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (2)
test/js/web/console/console-log.test.ts (1)
220-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the spawned process exit code.
proc.exitedis awaited, butexitCodeis never checked. The fixture could write matching output and still exit non-zero, allowing the test to pass despite a failure. Assertexpect(exitCode).toBe(0)after the output, file, and behavior assertions.🤖 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 `@test/js/web/console/console-log.test.ts` around lines 220 - 230, Add an assertion for the awaited exitCode in the spawned-process test, requiring it to equal 0 after the existing output, file, and behavior assertions. Keep the current proc.stdout, proc.stderr, and proc.exited collection unchanged.Source: Coding guidelines
src/jsc/ConsoleObject.rs (1)
3891-3912: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve line-length accounting for delegated
%soutput.
print_percent_swrites directly towriter_without updatingself.estimated_line_length. A following%o/object value can therefore make wrapping decisions using the column position from before%s, especially for long or multiline results. Preserve the existing accounting path and add a mixed%splus object regression case.🤖 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 `@src/jsc/ConsoleObject.rs` around lines 3891 - 3912, The delegated output path in print_percent_s must update self.estimated_line_length consistently with the existing formatting output path. Account for the rendered %s result, including long or multiline text, before subsequent %o/object formatting makes wrapping decisions, and add a regression case covering mixed %s followed by an object value.
🤖 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 `@test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js`:
- Line 1819: Remove the per-test timeout argument from the test invocation
around isDebug, and move the 30-second debug budget to the enclosing suite-level
timeout mechanism if needed; otherwise reduce the workload while preserving the
test’s behavior.
---
Outside diff comments:
In `@src/jsc/ConsoleObject.rs`:
- Around line 3891-3912: The delegated output path in print_percent_s must
update self.estimated_line_length consistently with the existing formatting
output path. Account for the rendered %s result, including long or multiline
text, before subsequent %o/object formatting makes wrapping decisions, and add a
regression case covering mixed %s followed by an object value.
In `@test/js/web/console/console-log.test.ts`:
- Around line 220-230: Add an assertion for the awaited exitCode in the
spawned-process test, requiring it to equal 0 after the existing output, file,
and behavior assertions. Keep the current proc.stdout, proc.stderr, and
proc.exited collection unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b0f871d-e2e0-4679-bf40-0a843d87e281
📒 Files selected for processing (4)
src/js/internal/util/inspect.jssrc/jsc/ConsoleObject.rstest/js/node/util/node-inspect-tests/parallel/util-inspect.test.jstest/js/web/console/console-log.test.ts
|
Addressing the outside-diff CodeRabbit findings:
|
There was a problem hiding this comment.
Both earlier nits are addressed and I didn't find new issues, but this is a cross-layer change (new LazyPropertyOfGlobalObject + FFI bridge, native console now calls into JS for every non-string %s) that also picks one of two competing designs vs. #34603 — worth a maintainer's eyes.
What was reviewed:
Bun__callFormatPercentSexception discipline mirrorsJSC__JSValue__callCustomInspectFunctionabove it;print_percent_spropagates viafrom_js_host_calland updatesadd_for_new_line.formatPercentSis a byte-for-byte hoist of the previous case-115 body;formatWithOptionsInternalsemantics unchanged.Float16Arrayremoval from util-inspect.test.js is a required consequence of the frozenbuiltInObjectsset and is commented as such.- The
m_utilInspectFormatPercentSFunctioninitLater follows the existingm_utilInspectFunctionpattern and is in the GC-visited macro list.
Extended reasoning...
Overview
Eight files across three languages: hoists Node's %s decision tree into an exported formatPercentS in internal/util/inspect.js, replaces the dynamic builtInObjects scrape with a hardcoded 47-name ECMA-intrinsic list, adds Bun__callFormatPercentS in UtilInspect.cpp plus a LazyPropertyOfGlobalObject<JSFunction> on ZigGlobalObject, and reroutes the native PercentTag::S arm in ConsoleObject.rs through that bridge (with a string-literal fast path). Tests: a new 32-case three-way consistency test in console-log.test.ts, host-global %s cases added to util-format.test.js, and Float16Array dropped from two typed-array loops in util-inspect.test.js.
Security risks
None identified. No untrusted input parsing; the new FFI entry point takes a JSValue already held by the caller and hands it to a fixed internal JS function. Exception scopes are declared and checked at each fallible step.
Level of scrutiny
Medium-high. This changes user-visible output of console.log('%s', …) and util.format('%s', …) for many value classes, adds a native→JS call on a path that was previously pure-native, and freezes builtInObjects to a specific list whose exact membership (e.g. exclusion of SharedArrayBuffer, Float16Array, WebAssembly) is load-bearing for hasBuiltInToString. The PR also explicitly competes with #34603's Rust-side reimplementation — a maintainer should pick the direction.
Other factors
- Both of my earlier inline findings (stale
print_stringcomment; the debug-only per-test timeout ride-along) were addressed in 6c220fb and 1a2b8d7. - The
builtInObjectshardcoding weakens an existing test (Float16Arrayremoved from twoshowHiddenloops); the removal is commented with the Node-parity rationale, which satisfies REVIEW.md's "every deletion needs a stated reason". - REVIEW.md discourages new fields on
ZigGlobalObject; this one is aLazyPropertysitting alongside three existingm_utilInspect*siblings and is wired intoFOR_EACH_GLOBALOBJECT_GC_MEMBER, so it's consistent with local convention rather than a god-object addition. - Test coverage is thorough (three-way agreement across 32 value shapes including Symbol, -0, BigInt, null-proto, revoked Proxy) and the PR shows the new test failing on the unfixed build.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js (1)
240-243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a dedicated
Float16Arrayregression case.The new intrinsic set intentionally changes
Float16Arrayhandling, but removing it from both matrices leaves that boundary untested. Keep it out of the homogeneous expectations, but add explicitshowHiddenassertions for the same-context and cross-context cases.As per coding guidelines, tests must cover the complete variant matrix and every behavioral change must include an automated test in the same PR.
Also applies to: 275-275
🤖 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 `@test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js` around lines 240 - 243, Add a dedicated Float16Array regression case in the util-inspect tests, keeping it excluded from the homogeneous typed-array expectations. Add explicit showHidden assertions covering both same-context and cross-context Float16Array instances, matching the intended new intrinsic handling and completing the variant matrix.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js`:
- Around line 240-243: Add a dedicated Float16Array regression case in the
util-inspect tests, keeping it excluded from the homogeneous typed-array
expectations. Add explicit showHidden assertions covering both same-context and
cross-context Float16Array instances, matching the intended new intrinsic
handling and completing the variant matrix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf261e41-cf40-459d-928f-555e49bdfb30
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/node/util/node-inspect-tests/parallel/util-inspect.test.js
…type (Node parity)
|
On the Not adding a cross-context variant: |
There was a problem hiding this comment.
No bugs found, but deferring to a human — this reroutes the native console.log('%s', ...) hot path through a JS call for every non-string value and picks one of two competing approaches (vs. #34603's Rust reimplementation), which is a design/perf trade-off worth a maintainer's sign-off.
What was reviewed:
Bun__callFormatPercentS/m_utilInspectFormatPercentSFunctionfollow the same LazyProperty + throw-scope pattern as the existingm_utilInspectFunction; exception paths look correct.print_percent_skeeps the primitive-string fast path and updatesestimated_line_lengthlike the oldprint_stringpath did.- The hardcoded
builtInObjectsset also changesutil.inspect(x, {showHidden: true})for Float16Array/SharedArrayBuffer/DisposableStack/etc. — the PR documents this and adds a positive assertion for Float16Array.
Extended reasoning...
Overview
Eight files. In src/js/internal/util/inspect.js: replaces the dynamic Object.getOwnPropertyNames(globalThis) scrape for builtInObjects with a hardcoded 47-name literal, and hoists the case 115 body of formatWithOptionsInternal into an exported formatPercentS. In src/jsc/ConsoleObject.rs: replaces the PercentTag::S arm's print_as(Tag::String, ...) with a new print_percent_s that fast-paths primitive strings and otherwise calls Bun__callFormatPercentS (new C++ bridge in UtilInspect.cpp) which resolves and invokes the JS formatPercentS via a new LazyPropertyOfGlobalObject<JSFunction> on ZigGlobalObject. Tests: a 32-case three-way parity test in console-log.test.ts, host-global %s assertions in util-format.test.js, and a Float16Array showHidden adjustment in util-inspect.test.js.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permissions surface. The native→JS call goes through from_js_host_call with proper JSError propagation, and the C++ side has RETURN_IF_EXCEPTION after both the lazy-property resolution and the profiledCall.
Level of scrutiny
Medium-high. This is user-visible behavior on two very common APIs (console.log and util.format), and the native formatter is a hot path. The builtInObjects change has wider blast radius than %s alone — it also flips hasBuiltInToString and the showHidden prototype-walk decision for Float16Array, SharedArrayBuffer, WebAssembly, DisposableStack, AsyncDisposableStack and SuppressedError. The PR argues this matches Node's bootstrap-time set, and the Float16Array assertion checks one boundary, but the full set of side effects on util.inspect output deserves human eyes.
Other factors
- Competing approach: #34603 reimplements Node's decision tree in Rust rather than calling into JS. Choosing between "one JS source of truth" and "no JS call on the console hot path" is an architectural call a maintainer should make.
- Prior feedback addressed: my two earlier inline comments (stale
print_stringcomment, ride-along debug timeout) were both fixed; comment-cop and CodeRabbit threads are all resolved. - Correctness spot-checks: the lazy-property initializer mirrors the sibling
m_utilInspectFunctionexactly (samerequireId→getIfPropertyExists→uncheckedDowncast<JSFunction>shape);print_percent_scallsadd_for_new_line(str.length())before writing, preserving column tracking for subsequent%otokens; the string-literal fast path usesis_string_literal()which correctly excludesStringObject. - Test coverage is thorough for the three-way parity claim (32 value shapes across util.format / global console / Console instance) and demonstrably fails on the unfixed build per the PR body.
CI status at cfdcf76This PR's own tests pass on every lane: Remaining red/yellow on build 83618 are unrelated to this diff:
The Ready for review. |
2 participants
Reproduction
util.format('%s', Buffer.from('ab'))ab<Buffer 61 62>abutil.format('%s', new URLSearchParams('a=1'))a=1URLSearchParams {}a=1console.log('%s', new Map(...))Map(1) { 'k' => 'v' }[object Map]Map(1) { 'k' => 'v' }console.log('%s', Symbol('q'))Symbol(q)Symbol(q)console.log('%s', -0)-00-0console.log('%s', 42n)42n4242nconsole.log('%s', Object.create(null))[Object: null prototype] {}[Object: null prototype] {}Cause
Bun shipped three
%sbehaviors:util.format('%s', v)andnew Console(stream).log('%s', v)both ran the JSformatWithOptionsInternalport of Node's decision tree.console.log('%s', v)ran the native formatter'sPercentTag::S, which forced engineToString(v)for every value: throws on Symbol, loses-0and the BigIntnsuffix, and prints[object Object]/[object Map].Separately,
hasBuiltInToStringdecided whether an object's inheritedtoStringis "built-in" by checking the owner prototype's constructor name againstbuiltInObjects, which was populated fromObject.getOwnPropertyNames(globalThis). Node captures that set at bootstrap when only ECMAScript intrinsics are present; Bun captures it after every host global is already installed, so Bun's set was a superset containingBuffer,URL,URLSearchParams,Request,Blob, ... andutil.format('%s', buf)took the inspect branch instead ofString(buf).Fix
One source of truth, Node's architecture:
builtInObjectsis the hardcoded list of ECMA-262 language-level constructor names (what Node's bootstrap scrape actually yields). Host prototypes (Buffer,URL,URLSearchParams, ...) now take theString()path through%s.formatPercentS(inspectOptions, arg)ininternal/util/inspect.js;formatWithOptionsInternalcalls it.PercentTag::Skeeps a primitive-string fast path and delegates every other value to that same JS routine via a lazily cachedLazyPropertyOfGlobalObject<JSFunction>(Bun__callFormatPercentSinUtilInspect.cpp). The returned string is written with colors off, matching Node'scolors: false.util.format,Consoleinstances, and the global console now cannot disagree on%s.Verification
Also green:
test/js/bun/util/inspect.test.js,test/js/node/util/bun-inspect.test.ts,test/js/node/util/util.test.js,test/js/node/util/custom-inspect.test.js,test/js/node/console/,test/js/bun/console/,test/js/web/console/.Supersedes #36138 (the
builtInObjectshardcoding, same list) and takes a different route than #34603 (which reimplements the decision tree in Rust; this calls the one JS routine instead so the three paths share code).builtInObjectscontentsThe frozen set is the 47 names Node v26.3.0 observes on
globalThisat bootstrap:SharedArrayBuffer,WebAssembly,Float16Array,DisposableStack,AsyncDisposableStackandSuppressedErrorare deliberately excluded: V8 installs them after Node computes the set, so they are absent from Node'sbuiltInObjects, and the difference is observable (for example,util.inspect(new Float16Array(1), { showHidden: true })walks intoFloat16Array.prototypein Node, whileFloat32Arraydoes not).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/node/util/node-inspect-tests/parallel/util-inspect.test.js