Conversation
The quote_strings branch of print_string ran before the StringObject branch, so a String wrapper nested in an object, array, Map or Set (or passed to Bun.inspect at the top level) printed as a plain quoted string while Number and Boolean wrappers in the same position printed as [Number: 1] / [Boolean: true].
|
Warning Review limit reached
Next review available in: 4 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 (4)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on released bun 1.4.0 and on main (b7a0431) with |
There was a problem hiding this comment.
LGTM — minimal, well-scoped fix that lets StringObject fall through the quote_strings guard to its existing [String: "..."] branch, matching how RegExpObject was already excluded and how print_double/print_boolean handle the other boxed primitives.
What was reviewed:
- Confirmed
print_stringcontrol flow: primitive strings and RegExps take exactly the same branches as before; onlyStringObjectis redirected. add_for_new_line("[String: \"\"]".len())mirrorsprint_double's decoration-width accounting (content length already added at the top of the function).DerivedStringObjecthandling is unchanged (onlyString.prototype, per the PR description; subclass instances areStringObjectand covered by theMyStringtest).- Tests cover top-level and every container, subclass, empty wrapper, Latin-1/UTF-16 escaping, and the colored instantiation.
Extended reasoning...
Overview
This PR fixes console.log / Bun.inspect output for new String(...) wrapper objects when they appear nested inside a container (object, array, Map, Set) or at the top level of Bun.inspect. Previously they printed as plain quoted strings ("s") instead of [String: "s"], unlike new Number/new Boolean which already printed as wrappers at every depth. The fix is a two-line change to print_string in src/jsc/ConsoleObject.rs: the quote_strings early-return branch now excludes StringObject (alongside the existing RegExpObject exclusion) so it falls through to the pre-existing [String: ...] rendering branch below it, and that branch now contributes its decoration width to the line-length estimate. Tests are added in inspect.test.js and console-log.js/.expected.txt.
Security risks
None. This is purely output formatting in the console/inspect pretty-printer. No parsing of untrusted input, no allocation-size arithmetic, no new FFI or JS re-entry. The value's string content is obtained the same way as before (BunString::from_js at the top of the function) and printed through the same write_json_string / Tag::JSON paths the wrapper branch already used.
Level of scrutiny
Low. The native change is a guard-condition tweak plus one add_for_new_line call, both following the exact pattern already used for RegExpObject and print_double respectively. I traced the surrounding print_string body: primitive strings (quote_strings && !StringObject && !RegExpObject) and RegExps continue on identical paths; only StringObject values, which previously short-circuited into the quoted-primitive rendering, now reach their dedicated branch. DerivedStringObject is not added to the exclusion — consistent with the existing js_type == StringObject check below and with the PR's explanation that subclass instances are StringObject cells (verified by the class MyString extends String test assertion) while DerivedStringObject is reserved for String.prototype (deferred to #36660).
Other factors
Test coverage is thorough: top-level and nested in every container type, subclass instance, empty wrapper, Latin-1 and UTF-16 contents with escape characters, the colored code path, and negative assertions that primitive strings and RegExps keep their previous rendering. The console-log fixture exercises the console.log (non-quote_strings at top level) path with both nested-object and nested-array forms alongside a bare primitive string in the same array. The PR description documents that the new tests fail on the released binary and pass with the change, and that the broader inspect/console/util/expect suites pass. No prior reviewer comments to address.
|
Updated 10:34 AM PT - Aug 13th, 2026
✅ @robobun, your commit 7002aec1e6f47a606f2036a0e640a51a6f1680f4 passed in 🧪 To try this PR locally: bunx bun-pr 38171That installs a local version of the PR into your bun-38171 --bun |
Problem
new String(...)wrapper nested inside an object, array, Map or Set is printed byconsole.log/Bun.inspectas a plain quoted string, while the Number and Boolean wrappers next to it print as wrappers:console.log(new String("top"))alone was already[String: "top"], so the wrapper rendering exists; it was just unreachable for nested values.print_string(src/jsc/ConsoleObject.rs), theif self.quote_stringsbranch, which every value nested in a container takes (and whichBun.inspectalso enables at the top level), JSON-quotes the string and returns before thejs_type == StringObjectbranch below it is reached. Reproduces on released bun 1.4.0 and on main; the Rust port kept the branch order of the original Zig code.Fix
StringObjectthe same way it already skippedRegExpObject, so a String wrapper falls through to its[String: "..."]branch at every depth. The wrapper branch also adds the width of its decoration to the line-length estimate, likeprint_doubledoes, so arrays of wrappers wrap at the right point.class X extends Stringinstances are plainStringObjectcells in JSC (DerivedStringObjectTypeis only used forString.prototypeitself, seeStringPrototypeInlines.h), so subclass instances are covered.String.prototypeis intentionally left as is; console: print built-in prototypes as {} instead of boxed value / method dump #36660 handles it at the tagging layer.test/js/bun/util/inspect.test.js("String wrapper objects print as [String: ...] at every depth"): top level, object, array, Map key and value, Set, subclass instance, empty wrapper, Latin-1 and UTF-16 contents with escaping, colors, plus primitive string / RegExp unchanged. Fails on the released binary (Received: ""s""), passes with this change.test/js/web/console/console-log.js+.expected.txt: theconsole.logform for an object and an array of wrappers next to the existing top-level wrapper lines. Fails on the released binary withs: "Hello", passes with this change.bun bd test test/js/bun/util/inspect.test.js,test/js/bun/console,test/js/web/console,test/js/node/util/{bun-inspect,util},test/js/bun/test/{expect,jest-extended,expect-label,jest-each}all pass.print_stringreads the wrapper's text from and its test currently asserts the oldBun.inspect(new String("hello")) === '"hello"'output, so whichever of the two lands second needs that assertion updated to[String: "hello"]. console: substitute format specifiers in the first argument only #33445 / console: format boxed primitives and RegExp from their internal slot #36404 also cover the separate issue of a String wrapper first argument being used as the format string; unchanged here.Background
Formatterinsrc/jsc/ConsoleObject.rsis the native pretty printer shared byconsole.log,Bun.inspect,bun testfailure messages and the error printer. Each value is classified into aTag;StringObject,RegExpObjectand primitive strings all map toTag::Stringand are printed byprint_string, which then distinguishes them by the value'sJSType.quote_stringsis a formatter flag:console.log("x")prints a barex, but once the printer recurses into a container (or when the caller isBun.inspect) the flag is set so strings print quoted. It is meant to affect primitive strings only; boxedNumber/Booleanvalues are printed byprint_double/print_booleanand never consult it.new String("s"),new Number(3)) is an object wrapping a primitive; Node'sutil.inspectrenders it as[String: 's']/[Number: 3]so it cannot be confused with the primitive itself.