Collapse console formatter TagPayload into Tag - #37358
Conversation
…urcemap, threading, url
Resolved src/bun_core/string/mod.rs: #33909 switched return types to crate::CrateResult<()> in write_pre_quoted_string and quote_for_json, which this branch restructures into the const-generic write_pre_quoted_string_inner; carried CrateResult<()> into both the adapter and _inner signatures. Resolved src/router/lib.rs and src/sourcemap/Chunk.rs: #33909 touched return types inside blocks this branch deletes (orphaned router test scaffolding, uncalled print_source_map_contents); kept the deletions after re-verifying zero callers on current main. Restored Node::find_last in src/collections/pool.rs: gained a test caller via #33311.
…update --interactive
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a ~500-line diff touching the console formatter (which drives console.log, Bun.inspect, error rendering, and snapshot output), a human look would still be worthwhile.
Checked: is_array_buffer_like() matches exactly the 14 explicit JSType arms it replaces in both files; the deleted pretty_format::{MessageType, ZigFormatter, WriteError} are dead duplicates of the canonical ConsoleObject.rs versions (no pretty_format:: references remain); no residual TagPayload references anywhere in src/; custom: None at the two trait-bridge sites (lib.rs, AsymmetricMatcherFormatter) matches the old From<Tag> behavior whose comment stated CustomFormattedObject is never passed there.
Extended reasoning...
Overview
This PR collapses two parallel enums in the console formatter: TagPayload (a copy of Tag with one payload-carrying variant) is deleted, and its sole payload moves to a new custom: Option<CustomFormattedObject> field on TagResult. Alongside this, it extracts a WrappedWriter::new() constructor (replacing ~20 inline struct literals), inlines the local pf!($s) → pfmt!($s, C) macro at each site, replaces two 14-arm typed-array match blocks with t if t.is_array_buffer_like(), and deletes three dead duplicate types from pretty_format.rs. Five files touched; ConsoleObject.rs is the bulk. Split 1 of 8 from #32023, which a maintainer asked to be broken up.
Security risks
None. This is a formatter/type refactor with no I/O, parsing, auth, or untrusted-input handling changes.
Level of scrutiny
Moderate. Each change is individually mechanical, but the console formatter is on the path of console.log, Bun.inspect, error printing, and toMatchInlineSnapshot — a subtle output difference would ripple through many snapshot tests. I verified the specific behavior-preservation claims: JSType::is_array_buffer_like() (JSType.rs:709) matches the identical 14 variants the guard arm replaces, and the guard's position doesn't intercept any later arm; the custom: None at the ConsoleFormatter trait bridge and AsymmetricMatcherFormatter reproduces the old From<Tag> for TagPayload default (whose doc comment already noted CustomFormattedObject is never passed there); the deleted pretty_format types have zero references (the live ones are all console_object::formatter::ZigFormatter / ConsoleObject::MessageType).
Other factors
The PR states the union of all 8 splits is byte-identical to #32023's CI-green head, and the relevant local suites (console-table, bun-inspect-table, macro-test, expect.test.js) pass. No new tests are added, which is appropriate for a no-behavior-change refactor. Deferring rather than approving because the diff is large enough (and the surface user-visible enough) that a maintainer skim of the ConsoleObject.rs hunk is warranted, per this repo's review norms for refactors.
|
Thanks, those are the four equivalence points that matter here and they match what I checked when cutting the split. One more data point on the snapshot-ripple concern: the union of the eight splits is byte-identical to #32023's head, and that head ran the full suite at 285/286 (build 72447), which includes every snapshot and inline-snapshot test in the tree; the one red was an unrelated napi GC-timing flake on windows. So any output difference in the formatter would already have shown up there. Over to @alii for the human skim of the |
2f5dd36 to
9ae3ff5
Compare
|
Updated 5:05 AM PT - Aug 11th, 2026
❌ @alii, your commit fb3a859 has some failures in 🧪 To try this PR locally: bunx bun-pr 37358That installs a local version of the PR into your bun-37358 --bun |
2 participants
What this does
The console formatter carried two parallel enums,
Tag(the discriminant) andTagPayload(the same variants plus a payload onCustomFormattedObject), with aTagPayload::tag()projection andFromimpls between them. This folds the payload into acustom: Option<CustomFormattedObject>field onTagResultand deletesTagPayload, updating the three consumers (macro evaluator, test-runner pretty printer,VirtualMachine's native-code check).Split 1 of 8 from #32023, which alii asked to be broken up. Pure code motion and deletion; no intended behavior change. Stacked on
claude/split/install-cli(merge order: foundations → install-cli → these, in any order among themselves).Notes from rebasing against main
console.tablewas rewritten on main to format each cell once (#32924); the rename is applied on top of that shape.ConsoleObject.rsis the bulk of the diff.Verification
cargo check), and the union of all eight splits is byte-identical to Deduplicate jsc bindings, runtime api, test matchers, and shell builtins #32023's headf29ce8fe4, which CI ran at 285/286 green (the one red was the fleet-widenapi.test.tsGC-timing flake on windows x64-baseline).