console: use Node's %s format semantics instead of engine ToString - #34603
console: use Node's %s format semantics instead of engine ToString#34603robobun wants to merge 10 commits into
Conversation
The native console formatter's %s specifier routed every argument
through print_as(Tag::String), which calls BunString::from_js, i.e.
the engine ToString abstract operation. That throws a TypeError for
Symbol arguments and yields [object Object] / 1,2 / 0 / locale Date
text where Node's util.format prints Symbol(q) / { a: 1 } / [ 1, 2 ]
/ -0 / the ISO timestamp.
Add print_percent_s, mirroring Node's formatWithOptionsInternal case
for %s: numbers and bigints are printed with sign and n-suffix
preserved, symbols print their descriptive string, other primitives
write their literal form, functions go through String(), and
remaining objects are formatted with the inspect path at
{ depth: 0, compact, colors: false }.
|
Status: reproduced on main, fixed in The diff is green. CI on build 75436 is red on unrelated lanes only: the worker-terminate race that |
WalkthroughChangesThe console formatter now implements Node-style Console percent-s formatting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:09 AM PT - Jul 18th, 2026
❌ @robobun, your commit f281f8a has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34603That installs a local version of the PR into your bun-34603 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The single_line print_object path increments indent unconditionally but
only decrements it on the always_newline branch of print_object_tail,
so each %s on a non-empty object leaked indent += 1 into later
arguments of the same console.log call. Add a defer_restore for indent
alongside the existing quote_strings/single_line/max_depth guards.
Also delegate the Symbol arm to print_symbol::<false> instead of
duplicating it, and switch the test to a combined {out, err, exitCode}
assertion with an extra case covering the indent leak.
There was a problem hiding this comment.
All three items from the earlier pass are addressed in b6b3679 (indent restore + regression case, print_symbol delegation, combined {out, err, exitCode} assertion) and I found nothing new in the updated diff. Leaving this for a human to sign off since it's a user-visible console.log output change with the acknowledged hasBuiltInToString gap — objects with a custom toString now get inspected instead of stringified, which is the opposite direction from Node for that one case.
Extended reasoning...
Overview
Adds print_percent_s in src/jsc/ConsoleObject.rs (~95 lines) replacing the PercentTag::S arm's print_as(Tag::String) call, plus a subprocess test in test/js/web/console/console-log.test.ts covering 19 input shapes. The new function type-dispatches on the argument: strings/callables keep the old Tag::String path; number/bigint/symbol/null/undefined/boolean get literal handling; everything else falls through to format::<false> with single_line=true, quote_strings=true, max_depth=depth, and colors off, with defer_restore! guards on all four mutated formatter fields.
Prior feedback
I raised three items on the previous revision — the self.indent leak across %s object calls, hand-rolled Symbol formatting duplicating print_symbol, and the standalone expect(err).toBe(""). All three are fixed in b6b3679 and the indent leak has a dedicated test line (%s %s %O → asserts the %O object is at indent 0). Re-checked the object fallback: all four mutated fields (quote_strings, single_line, max_depth, indent) are now saved/restored.
Security risks
None. Pure output-formatting; no new coercions that call into user code (the Symbol path now avoids ToString, which is strictly less user-code execution than before).
Level of scrutiny
Medium. Not mechanical: it changes what console.log("%s", x) prints for every non-string argument, which users notice and file issues about. The PR description explicitly defers Node's hasBuiltInToString gate, meaning objects with a user-defined toString (e.g. {toString: () => "custom"}, or library types that override it) previously printed their custom string via engine ToString and will now be inspected — that specific case moves away from Node. Whether to ship the partial fix now (it does fix the Symbol crash and the common plain-object/array/Date/-0 cases) or hold for the toString gate is a maintainer call.
Other factors
Test coverage is good for the handled cases; the PR reports the node util-format parallel test suite still passes. No memory-safety surface — all new code is value-type dispatch and delegation to existing helpers.
The previous commit routed every object through the inspect path, which
regressed objects that define their own toString (or inherit one from a
non-core prototype): before this PR those printed their toString()
result via engine ToString, matching Node.
Add percent_s_has_user_to_string, an approximation of Node's
!hasBuiltInToString: walk the prototype chain to the first object that
owns a callable toString and check whether its own constructor names a
core ECMAScript built-in. Objects whose toString comes from Object /
Array / Date / Map / etc. go through inspect; anything else (literal
{toString(){...}}, user classes, URL) goes through String(value).
Extend the test to cover a literal toString, a class-provided toString,
and a user class without toString (inspected).
|
Addressed the custom |
… in OwnedString .unwrap_or(false) discarded the Rust Err but left the pending exception on the VM, so the following Tag::get re-entered JSC with a stale exception. print_percent_s already returns JsResult and its caller uses ?, so propagate with ? instead. Also wrap ctor.get_name in OwnedString so the +1 WTF ref is released at scope exit.
percent_s_has_user_to_string applied the built-in-constructor check even
when the toString lives on the value itself, and had no path for
Symbol.toPrimitive at all, so classes that define [Symbol.toPrimitive]
and values like {toString(){...}, constructor: Object} or the built-in
prototype objects themselves were routed to inspect instead of
String(value). All of those matched Node before this PR.
Add a BuiltinName::toPrimitive variant (mapped to
vm.propertyNames->toPrimitiveSymbol in bindings.cpp) and a fast_get_own
wrapper around the existing JSC__JSValue__fastGetOwn, then rework the
helper to mirror Node's hasBuiltInToString: own callable toString or
Symbol.toPrimitive on the (proxy-unwrapped) value itself is always
user-provided; otherwise walk prototypes to the first owner of either
and treat it as built-in only when its own constructor names a core
ECMAScript type.
Extend the test with a class [Symbol.toPrimitive], an own
Symbol.toPrimitive literal, {toString, constructor: Object}, and
RegExp.prototype.
![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
🤖 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 `@src/jsc/ConsoleObject.rs`:
- Around line 3343-3373: Update the `%s` hook detection around `owns_callable`
so own `toString` and `Symbol.toPrimitive` properties count as shadowing
regardless of whether their values are callable, undefined, or non-callable;
only prototype traversal should continue when the property is absent. Replace
`pointer.get_own_truthy` with an own-property descriptor lookup for
`constructor`, avoiding getter execution while preserving the existing
builtin-constructor decision. Add tests covering both non-callable/undefined own
hooks and accessor-based constructors.
🪄 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: de38a0af-3b9c-48f6-849a-6449db29998b
📒 Files selected for processing (5)
src/jsc/ConsoleObject.rssrc/jsc/JSValue.rssrc/jsc/bindings/bindings.cppsrc/jsc/lib.rstest/js/web/console/console-log.test.ts
…tters Node's hasBuiltInToString reads the prototype's constructor via Object.getOwnPropertyDescriptor and checks descriptor.value, so an accessor constructor is not invoked and is treated as user-provided. get_own_truthy went through getOwnPropertySlot + slot.getValue, which ran the getter and could misclassify the prototype as a built-in. Add BuiltinName::constructor (mapped to vm.propertyNames->constructor) and a fast_get_direct wrapper around the existing JSC__JSValue__fastGetDirect_ (JSObject::getDirect, no getter invocation), and use it for the constructor lookup in percent_s_has_user_to_string. An accessor slot comes back as a non-callable GetterSetter cell, so the helper returns Ok(true) and the value goes through String(value), matching Node and the pre-PR behaviour.
…scope to fastGetOwn JSValue::get_prototype returns a bare JSValue with no JsResult, and the underlying JSC__JSValue__getPrototype has no throw scope, so a proxy getPrototypeOf trap that throws leaves a pending exception with an empty return. percent_s_has_user_to_string saw the empty value and returned Ok(false), after which print_percent_s re-entered JSC with the exception still pending. Check global.has_exception() after each get_prototype and return Err(JsError::Thrown) so the throw propagates out of console.log, matching Node. Also give JSC__JSValue__fastGetOwn the same ASSERT_NO_PENDING_EXCEPTION / DECLARE_THROW_SCOPE / RETURN_IF_EXCEPTION shape as its sibling JSC__JSValue__getOwn now that this PR makes it live.
Keeps the string -> BuiltinName reverse map in lib.rs a 1:1 mirror of the enum after adding the two new variants.
|
#36141 takes the shared-helper route for this: the native |
1 participant
Reproduction
Symbol("q")Symbol(q)Symbol(q){ a: 1 }{ a: 1 }[object Object]{ a: 1 }[1, 2][ 1, 2 ]1,2[ 1, 2 ]-0-00-0new Date(...)2023-11-14T22:13:20.000ZTue Nov 14 2023 22:13:20 GMT+0000 ...2023-11-14T22:13:20.000ZThe Symbol case is the worst: a debug
console.logthat happens to receive a Symbol throws out ofconsole.logitself, which in a server takes the request down.Cause
PercentTag::Sinsrc/jsc/ConsoleObject.rscalledprint_as(Tag::String, ...), which reachesBunString::from_js(the engineToStringabstract operation).ToString(symbol)throws per spec, and for objects/arrays/-0/Dateit produces the generic string coercion instead of Node'sutil.formatoutput.Fix
Add
print_percent_s, mirroring Node'sformatWithOptionsInternalhandling of%s:dtoa_with_negative_zero(preserves-0,NaN,Infinity)print_bigint(adds thensuffix)print_symbol(Symbol(description)viaget_description, no throw)null/undefined/ booleans: literal textTag::Stringpath (equivalent toString(value))toStringorSymbol.toPrimitive(percent_s_has_user_to_string): existingTag::Stringpath, so{toString(){...}}, user classes withtoString/[Symbol.toPrimitive],URL, and the built-in prototype objects themselves keep printing their string form as beforesingle_line = true,quote_strings = true,max_depth = depth,indentrestored, colors forced offpercent_s_has_user_to_stringapproximates Node's!hasBuiltInToString: unwrap proxy targets, treat own callabletoString/Symbol.toPrimitiveon the value itself as always user-provided, otherwise walk prototypes to the first owner of either and treat it as built-in only when its ownconstructornames a core ECMAScript type (Object, Array, Date, Map, Set, the Error hierarchy, typed arrays, etc.). To support the symbol-keyed lookup this addsBuiltinName::toPrimitive(mapped tovm.propertyNames->toPrimitiveSymbolinbindings.cpp) and aJSValue::fast_get_ownwrapper around the existingJSC__JSValue__fastGetOwn.All
%soutput is emitted withENABLE_ANSI_COLORS = false, matching Node'scolors: false.Verification
Also green:
test/js/web/console/,test/js/bun/console/,test/js/node/console/,test/js/bun/util/inspect.test.js,test/js/node/util/node-inspect-tests/parallel/util-format.test.js.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file