assert: render Myers diffs natively - #39924
Conversation
AssertionError messages were built by native myersDiff() returning an array
of { kind, value } objects (one JS object + string per line/char) that
internal/assert/myers_diff.ts then walked to concatenate the +/-/... output.
Move printMyersDiff / printSimpleMyersDiff into node_assert.rs so the diff is
rendered straight into one Latin-1 or UTF-16 string; the JS file becomes a
thin wrapper that passes the current internal/util/colors escapes.
When one side is Latin-1 and the other UTF-16 the Latin-1 side is now widened
to UTF-16 before diffing (previously both were re-encoded to UTF-8 for line
diffs, and char diffs compared mismatched byte slices). This also fixes line
diffs that showed U+FFFD for Latin-1 characters and for some trailing
spaces; output now matches Node byte-for-byte on those cases.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 3 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (5)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it adds ~200 lines of new native rendering code with a generic CodeUnit/DiffText trait design and changes the mixed-encoding handling path, a human look would still be worthwhile.
What was reviewed:
render_lines/render_simpletraced line-for-line against the removed JSprintMyersDiff/printSimpleMyersDiff— collapse thresholds,diff[idx+1]/[idx+2]back-references, and thetrimEndguard preserving the leading\\nall match.is_js_whitespacechecked against the ECMA-262 WhiteSpace + LineTerminator set; operating on code units is safe since none of the listed code points fall in the surrogate range.- Latin-1 → UTF-16 widening via
u16::from(b)is correct for the 0x00–0xFF range;colors_from_jsbytes are always ASCII frominternal/util/colorssoappend_ascii's per-byte widening is sound.
Extended reasoning...
Overview
This PR moves printMyersDiff and printSimpleMyersDiff from src/js/internal/assert/myers_diff.ts into src/runtime/node/node_assert.rs, so the native myersDiff path renders the final diff string directly instead of returning a Diff[] array for JS to walk. It adds an Output enum, a Colors struct, and two small traits (CodeUnit, DiffText) to keep the renderer generic over Latin-1 bytes vs UTF-16 code units and over char vs line values. The mixed-encoding path is reworked: instead of re-encoding both sides to UTF-8 (line diff) or comparing mismatched byte slices (char diff), the Latin-1 side is widened to UTF-16. Two new host functions are exported from node_assert_binding.rs; the JS module becomes a thin wrapper passing internal/util/colors. Three tests are added with expected messages captured from Node v24.
Security risks
None identified. This is error-message rendering for node:assert; inputs come from util.inspect output on user values, not from untrusted network/file data. No parsing of external formats, no allocation sizing derived from untrusted lengths beyond what the existing diff algorithm already handles (with its DiffTooLarge/InputsTooLarge guards preserved).
Level of scrutiny
Medium. The change is not mechanical: it ports ~60 lines of JS control flow into ~200 lines of generic Rust, introduces a small trait-based design for output-code-unit polymorphism, and rewrites the encoding-mismatch branch. That said, it is confined to assertion error message formatting — a wrong output here degrades diagnostics rather than corrupting state — and the port is a direct transliteration whose behavior is pinned by exact-string tests against Node.
Other factors
I compared the Rust renderers against the removed JS line-by-line: the previousType && kind !== previousType && previousType === Equal condition simplifies to exactly what the Rust checks (the JS previousType && short-circuit on Insert=0 is subsumed by the === Equal test); the nopCount +1/+2/≥+3 branches, ... emission, and skipped flag match; the trailing trimEnd is reproduced with a code-unit loop that stops at length 1 to keep the leading newline (matching JS's trim-then-prepend). The is_js_whitespace set matches ECMA-262 WhiteSpace ∪ LineTerminator, and since none of those code points are surrogates, testing UTF-16 code units directly is safe. Memory: all Vecs are locals dropped at scope exit, borrowed &[T] line slices never outlive their input strings, and transfer_to_js hands the freshly-cloned BunString to JS. Diff<T> derives Copy, so the diff[idx] destructuring and diff[idx+1].value reads are sound.
I'm deferring rather than approving because this is a non-trivial native addition with a design choice (the trait abstraction) that a maintainer should glance at, not because I found anything wrong.
…ff tests Output::List and Output::Lines now hold the lines and check_comma_disparity flags, so a render mode can not be paired with the wrong diff mode and the bindings pass no bare bools. myers_diff takes the Output by reference. The new tests cover a line diff where both sides are UTF-16, which the previous renderer printed as arrays of char codes, and the colored char diff for Latin-1, UTF-16 and mixed inputs.
|
Picking this up. Pushed 3e228a3:
Verified: |
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Since it's a ~300-line native-code port introducing new rendering logic, traits, and encoding handling, a human look would still be worthwhile.
What was reviewed:
render_simple/render_linestraced against the removed JSprintSimpleMyersDiff/printMyersDiff— iteration order, nop-collapse thresholds, and the trimEnd-then-prepend-\n ordering all match.is_js_whitespacechecked against the ECMAScript WhiteSpace + LineTerminator set used byString.prototype.trimEnd.- Mixed-encoding widening (Latin-1 → UTF-16 via
u16::from(b)) is byte-value-preserving for the Latin-1 range, so the U+FFFD fix is sound. colors_from_jsreads from the internalutil/colorsmodule (not user-controlled), and falls back to empty on non-string — matches the no-color case.
Extended reasoning...
Overview
This PR moves printMyersDiff and printSimpleMyersDiff from src/js/internal/assert/myers_diff.ts into src/runtime/node/node_assert.rs, so the diff is rendered directly to a single JS string instead of round-tripping through a JS array of {kind, value} objects. It adds an Output enum to select the rendering shape, two new traits (CodeUnit, DiffText) to write generically over Latin-1/UTF-16, and two new host functions in node_assert_binding.rs. It also fixes a pre-existing encoding bug where mixed Latin-1/UTF-16 inputs were diffed over mismatched byte slices (or re-encoded to UTF-8 and later reinterpreted as Latin-1, producing U+FFFD). New tests in assert.test.cjs cover Latin-1, UTF-16, mixed-encoding, collapsed-run, and colored char-diff cases with expected messages captured from Node.
Security risks
None. This is error-message formatting for node:assert failures. Inputs are the util.inspect output of user values (strings), and the colors object is the internal internal/util/colors module. No parsing of untrusted external data, no filesystem/network, no auth/crypto.
Level of scrutiny
Medium. This is not a hot path (only runs when an assertion fails) and the blast radius of a bug is a slightly wrong diff message, not incorrect assertion results. But it is ~220 lines of new native rendering code with encoding-sensitive string handling, new trait abstractions, and a hand-rolled trimEnd whitespace matcher — the kind of port where an off-by-one or encoding mismatch is easy to miss. I traced the two render functions line-for-line against the removed JS and they match (including the nop_count == kNopLinesToCollapse + {1,2} special cases and the fact that the JS previousType && ... falsy-0 quirk only applied to Insert=0, which the Rust guard on Some(Equal) doesn't need to replicate). The trimEnd reimplementation preserves the leading \n via out.len() > 1, which mirrors JS building the message, calling .trimEnd(), then prepending \n.
Other factors
- Test coverage is solid: exact-string assertions against Node-captured output for both encodings, mixed encodings, the
...collapse path withskipped: true, and a subprocess test for the colored char-diff path (which only fires underFORCE_COLOR). The subprocess test drains stdout/stderr/exited concurrently and asserts stderr before exitCode. BunString::clone_latin1/clone_utf16andtransfer_to_jsexist and have the expected signatures;Diff<T>isCopysodiff[idx]inrender_linesis sound.- The raw
myersDifflist export is preserved for parity, and its argument reading was simplified (frame.argument(n).is_truthy()on missing args returns false, matching the old defaults).
Given the size and the new native abstractions, I'm deferring rather than approving outright, but I found nothing wrong.
|
Updated 3:28 AM PT - Aug 21st, 2026
✅ @robobun, your commit 320783efff9bba739f7e0a5d7eaf88ff2ccf5cdc passed in 🧪 To try this PR locally: bunx bun-pr 39924That installs a local version of the PR into your bun-39924 --bun |
…rship contracts compile errors Rebased onto main as a single commit; the branch history (with its merge commits) is not preserved. Adds a zero-cost branded scope layer over the raw JSC FFI (`src/jsc/scope.rs`: `Scope<'s>`, `Local<'s>`) that makes two boundary bug classes compile errors for code that stays on the scoped API: - a JS value escaping its host call unrooted (persisting requires the explicit `Scope::persist` -> `Strong`); - a JS-heap view (`Local::array_buffer_bytes`) held across an operation that can re-enter user JS (`&mut Scope`), e.g. a coercion that detaches the buffer. Codegen integration: - `#[bun_jsc::host_fn(scoped)]`: functions written as `fn(scope: &mut Scope, callframe) -> JsResult<Local>` get a macro-synthesized wrapper under their original name and unscoped signature, so js2native / `.classes.ts` / direct-call wiring stays byte-compatible. User `cfg`/doc/lint attributes propagate to the public wrapper and the extern shims. - `ZIG_EXPORT(tag, reenters_js | no_user_js)` effect markers on the C++ declarations; explicitly classified functions get branded wrappers generated into `bun_jsc::cpp::scoped` (`&mut Scope` / `&Scope`), unclassified and `null_is_throw` functions get none. All classified exports are verified against their C++ (`toMatch` and `putMayBeIndex` are `reenters_js`: a non-uint32 `lastIndex` goes through ToNumber, and index puts on exotic receivers reach `defineOwnProperty` traps). Migration: ~470 host functions are converted to the scoped form (behavior-preserving); the remaining escape hatches (`unscoped_global()` / `unscoped_bun_vm()` / `.unscoped()` and unscoped `#[host_fn]`s) are pinned per file by `test/internal/source-lints/scope-escapes.test.ts`. Also expresses the `make_*_with_bytes_no_copy` "pointer stays valid until the deallocator runs" contracts as ownership transfer (`typed_array_from_owned_slice` / `_from_vec` / `_from_owner`, `ForeignBytes`, `external_string_from_utf16`, `OwnedUrl`, `OwnedTextCodec`, `StoreRef::adopt`, `bun_sys::Mmap`, `EventLoop::scope`), replacing hand-paired create/destroy and leak-and-remember-to-free code paths. The static `Bun.CryptoHasher.hash` / `Bun.password.verifySync` argument detach bugs that motivated the layer were fixed independently on main (#36165) by coercing every argument first; here the same behavior is expressed through deferred `materialize` under the shared scope borrow, so reordering the view capture before a coercion is a borrow error. Both main's regression tests and the layer's suites pass. Rebase onto main (461 commits): 40 files conflicted; resolved by taking main's text and re-applying only the scope transformation. Changes that main made obsolete were dropped (TextEncoderStreamEncoder host fns, EventLoop::with_pipe_read_buffer, JSC__JSMap__size global arg, the sendHelperChild scoping). Follow-ups main's newer code required: JSValue::create_buffer_from_foreign now returns JsResult (the binding became fallible on main), ArrayBufferSink::end_from_js uses or_pending_exception (empty-jsvalue-laundering lint), TextDecoder createForStream uses struct update syntax (clippy, since the PR removes TextDecoder's Drop impl), ForeignBytes recorded in the vm-thread-door inventory, scope-escape limits regenerated, and the ratchet's regeneration mode is gated on an explicit --update flag. Second rebase (30 more commits, onto 8bc4d2a): two conflicts from the zero-fill removal (#39417). The zstd sync functions keep main's Failure enum and create_buffer_from_box behind the scoped signatures, and the latin1 TextDecoder path keeps main's uninitialized Vec but hands it to JSC through external_string_from_utf16_vec instead of the raw to_external_u16, matching the file's other two decode paths. Third rebase (6 more commits, onto 0002bf8): conflicts were all with the dead-code sweeps (#39420, #39448). Dropped the scoping of things main deleted (Bun.nanoseconds' host fn, ArrayBufferSink::to_js, the unreachable csrf error arm, three unused node:: re-exports) and kept the ownership refactor of ArrayBufferSink::end_from_js. Inventories regenerated; the jsresult-swallow one also picks up a count #39448 left stale on main. Fourth rebase (17 more commits, onto 258517a): do_publish keeps #39389's shape (topic JSString held and ensure_still_alive'd across the message conversion) under the scoped signature, and the valkey publish scoping sits after the command block #29339 added. Fifth rebase (42 more commits, onto 6948a12): OwnedTextCodec is gone with the WebKit codecs (#39485); TextDecoder keeps main's encoding_rs path and this PR's external_string_from_utf16_vec hand-offs, including on the new path. ArrayBuffer::from_owned_bytes stays deleted (no callers; its u32 cast that #39558 fixed never existed in the replacements), and the no-copy deallocator contract now states both the Err-path timing from Sixth rebase (18 more commits, onto 681a49b): three dead-code conflicts. The scoped js_assert_settings goes away with the native assertSettings (#38900, node:http2 validates in JS now), ParseArgumentsCfg's unused Default impl stays removed (#39585), and TimeoutObject keeps main's generated cached-accessor import next to the scoped imports. Seventh rebase (14 more commits, onto 32e8703): valkey subscribe() keeps stored, no trailing else) with the rejection and the new check spelled through the scope. Eighth rebase (36 more commits, onto 56c4e3d): the three expect matcher utils take #36912's propagating print_value; the conflict was only the line wrapping. memory_pressure.rs (new on main) is added to the scope-escape limits. Ninth rebase (17 more commits, onto 72ec6e2, which includes the #39839 build fix): FileSink::on_close combines this PR's with_mut probe with Tenth rebase (5 more commits, onto 4448a2e): the Windows cluster handle path keeps #39804's `?` on attach_windows_socket_payload under the scoped argument spelling. Eleventh rebase (7 more commits, onto a21f02a): pbkdf2/pbkdf2Sync take under the scoped signatures. Twelfth rebase (16 more commits, onto 6fb7102): the safe vm_loop_ctx is re-applied onto #40002's Cell-based upgrade client, including inside the new clear_data's with_mut. Thirteenth rebase (15 more commits, onto d0f6486): the assert binding is #39924's three thin host fns plus its run() helper, with the thin fns scoped (5 hatches now, was 1); #39995's named-pipe live-count testing fn is scoped like its neighbours.
…JS, the bake dev server assets, the shell, and the SQL crates (#40066) Scheduled dead-code sweep. Net -3275 lines (78 files, +112 / -3387; the source side is +32 / -3387, the rest is the test pin). No behavior change: every item below has zero references on `main` in `src/`, `packages/`, `scripts/`, `test/` and the regenerated `build/debug/codegen/`, and the build passes without it. About 2,350 of these lines carry over the removals from #37659, which was closed only for merge conflicts. Each of those was re-verified against current `main` (nothing gained a caller), and the diff applies to current `main` with no conflicts. The rest comes from fresh scans of the shell, SQL, bake and built-in JS areas, which no open dead-code PR covers. ### bun-uws (`packages/bun-uws/src`, -1168) Bun reaches uWS only through the C shim in `src/uws_sys/` and eight bindings files, so those includers are the complete set of callers. Checked against every one of them: * Whole headers nothing includes: `ClientApp.h`, `HttpError.h` (superseded by `HttpErrors.h`), `Multipart.h` and `MessageParser.h` (its only includer), `ProxyParser.h` (only reachable under `UWS_WITH_PROXY`, which no build defines; `grep UWS_ build/debug/compile_commands.json` is empty), plus every `#ifdef UWS_WITH_PROXY` block and the `reserved` / `proxyParser` parameters that only carried the proxy parser into `getHeaders()`. * `WebSocketBehavior::subscription` and `WebSocketContextData::subscriptionHandler`: the only place a `WebSocketBehavior` is built (`uws_ws()` in the shim) never sets it, so every dispatch site in `WebSocket::end/subscribe/unsubscribe` and `WebSocketContext::onClose` was unreachable. `TopicTree::unsubscribe` drops the `newCount` only those sites read. * `HttpRequest`: the per-route `std::map` of parameter offsets (`setParameterOffsets`, `getParameter(name)`; every caller uses the index overload), `getQuery()` with no key, `isShortRead`, `notFieldNameWord` / `hasMore` / `hasBetween`. * `HttpContext::HTTP_IDLE_TIMEOUT_S`, `HttpFlags::isAuthorized` (the live flag is the per-socket one in `AsyncSocketData`), `HttpResponse::getHttpResponseDataS`, `HttpResponse::endWithoutBody`, `HttpResponse::overrideWriteOffset` (the shim calls `setWriteOffset`). * `App::publish(unsigned char)`, two `App::listen` overloads, the `UWS_NO_ZLIB` block; two `H3App::listen` overloads; `Http3Request::isAncient` / `getCaseSensitiveMethod`; nine `Http3Response` members whose `uws_h3_res_*` shims are stubs; the write-only `Http3ResponseData::totalSize`; `Loop::integrate`, `Loop::setSilent` and `LoopData::noMark`; the `UWS_NO_ZLIB` / `UWS_MOCK_ZLIB` mock streams and `UWS_USE_LIBDEFLATE` guards in `PerMessageDeflate.h` (the macro is defined unconditionally three lines above its first use). * `HttpContext::layoutAssert()` and `WebSocketContext::layoutAssert()` were never called, and as static members of class templates their `static_assert`s never ran. The `HttpContext` one guarded nothing. The `WebSocketContext` one guards the layout `WebSocket::getContextData()` depends on (`group` first, `data` right after), so it becomes a `RELEASE_ASSERT` in `create()`: `offsetof` cannot express it because the struct is not standard-layout, and the layout can differ per ABI, so the check runs once per context in every build. * `misc/` (upstream README assets, demo `cert.pem` / `key.pem`), and the `.gitattributes`, `.dockerignore`, `.cursorignore` entries for a `fuzzing/` directory that no longer exists. ### Native `JSBufferList` (`src/jsc/bindings`, -641) `JSBufferList.cpp` / `.h`, the lazy class structure on `GlobalObject`, its three accessors, and the `DOMIsoSubspaces` / `DOMClientIsoSubspaces` slots. The JS streams implementation replaced it; the only remaining mention of "BufferList" in the tree is a comment in `internal/streams/readable.ts`. The sources are globbed, so no build-script change is needed. ### Built-in JS (`src/js`, -508) * `internal/util/inspect.d.ts` (ambient types for `node-inspect-extracted`, which nothing imports) and `src/js/.gitignore` (ignores `src/js/out`, which codegen has not written since the build moved under `build/`). * `private.d.ts`: the `BunFS` / `BunFSWatcher` types (`Bun.fs()` no longer exists), `Bun.TOML`, `Bun.tty`. * `QuicStream.prototype[kSendHeaders]` and its symbol, the unused `get [kVerifyPeer]` (the setter stays; `session[kVerifyPeer] = …` writes it), `colors.ts` `yellow` / `gray` / `clear` / `reset`, the runtime `SSLMode` export of `internal/sql/shared.ts` (only imported as a type), the `REPL_MODE_*` re-exports of `internal/repl/utils.js` (`repl.js` takes them from `internal/repl/mode`), `Dequeue#clear()`, the write-only `listeningId` in `net.ts`, and the `kGetNativeReadableProto` symbol in `internal/shared.ts` (its last reader went in 032713c). ### Bake (`src/runtime/bake`, -830) * `incremental_visualizer.html` and `memory_visualizer.html`: #36184 removed the `runtime_embed_file!` sites and the `/_bun/incremental_visualizer` route that served them, so the two pages are orphaned assets. The no-op `emit_*_visualizer_*` methods that #36184 left behind are still reachable and are not touched here. * `client/websocket.ts` `getMainWebSocket()` (exported, never imported) and a commented-out Zig `writeJsValue` in `serialized_failure.rs`. ### Shell (`src/runtime/shell`, `src/shell_parser`, -51) * `Base::end_scope()`, an empty function, and its 12 call sites in `states/*.rs`. `Script::deinit_from_interpreter` only called it and goes too. * `ShellErr::InvalidArguments`: never constructed (only matched), with its four match arms. * `bun_shell_parser` dependencies `scopeguard`, `const_format`, `enum-map`, `enumset`, `typed-arena`, `bun_collections`: no `use` or path reference in the crate. ### SQL (`src/sql`, `src/sql_jsc`, -22) and `hawk.toml` (-104) * `bun_sql` dependencies `const_format`, `enum-map`, `enumset`, `libc`; `bun_sql_jsc` dependencies `const_format`, `enum-map`, `enumset`, `libc`, `bun_uws_sys`, and a commented-out `bun_runtime` edge that would be a dependency cycle (`bun_runtime` depends on `bun_sql_jsc`). * `pub use` re-exports nothing imports: `ArrayBuffer`, `JSCell`, `host_fn` in `sql_jsc/jsc.rs` (the proc-macro is used by its absolute path), `mysql::{MySQLConnection, MySQLQuery, MySQLStatement}` (every consumer imports the leaf module), `postgres::SASL`. * `hawk.toml`: 13 overrides for `bun_sql::mysql::status_flags::StatusFlag::*` variants that #37229 removed; `StatusFlag` has one variant left. ### Rust (-18) `zig__ModuleInfo__destroy` and `zig_log` in `src/bundler/analyze_transpiled_module.rs`: `extern "C"` exports that no C++ declares or calls (`BunAnalyzeTranspiledModule.cpp` frees through `zig__ModuleInfoDeserialized__deinit`), with no hit in `vendor/WebKit` either. ### Verification * `rg` for every symbol across `src/`, `packages/`, `scripts/`, `test/`, `docs/` and `build/debug/codegen/`; `.classes.ts` files, `src/codegen/`, string-named lookups (`$getByIdDirectPrivate(this, "…")`, `$zig` / `$cpp`) and token-paste macros (`name##PublicName`) were checked by hand. Two candidates failed those checks during the run and stay: `BunBuiltinNames` `mockedFunction` (reached through `BUN_COMMON_STRINGS_EACH_NAME`) and `writer` (reached through `$getByIdDirectPrivate(this, "writer")` in `ConsoleObject.ts`). * `bun bd` builds. `cargo check --workspace` passes on all 12 target triples. `cargo fmt --check`, prettier, oxlint and clang-format are clean. `tsc -p src/js` reports the same errors before and after (only line numbers move). * `test/js/bun/websocket/websocket-server.test.ts` gains a pin for the pub/sub paths this PR edits: with two sockets on shared topics, `unsubscribe()` of a topic the socket never joined (or that does not exist) reports false and changes nothing, leaving the last topic frees the subscriber and a later `subscribe()` counts again, and `close()` drops the socket from every topic without touching the other socket. Because this PR only deletes unreachable code, the pin passes before and after by design. * `bun bd test`: `bun/websocket/websocket-server.test.ts`, `web/websocket/websocket-permessage-deflate.test.ts`, `bun/http/bun-serve-routes.test.ts`, `bun/http/serve-http3.test.ts`, `node/http/node-http.test.ts`, `node/quic/quic-stream.test.ts`, `node/net/node-net-server.test.ts`, `bun/util/inspect.test.js`, six `bun/shell/*.test.ts` files, `sql/sql-mysql*.test.ts`, `sql/sql-mariadb-json.test.ts`, `sql/postgres-binary-numeric.test.ts`, `sql/sql-connect-error-reporting.test.ts`, `node/stream/node-stream.test.js`, `node/console/console-table-iterators.test.ts`, `web/console/console-log.test.ts`, `internal/fifo.test.ts`, `bake/dev/esm.test.ts`, `bake/dev/hot.test.ts`, `bun/repl/repl.test.ts` (mode tests), `node/test/parallel/test-repl-definecommand.js`. Details on the local failures that also fail on the released bun are under Notes. ### Looked at and left alone * `src/runtime/api/bun/h2/`: the outbound half (`Connection::send_header_block` / `send_data` / `send_push_promise`, `hpack::Coder::encode`, `SendWindow::available`) is only exercised by its unit tests. The module doc describes it as the engine that will replace `h2_frame_parser.rs`, so it is in progress, not dead. * The bake visualizer protocol residue: `MessageId::Visualizer`, the `IncrementalVisualizer` topic, and the empty `emit_*_visualizer_*` methods and their timer. All reachable; removing them is a protocol change that also touches the generated `generated.ts` ids. * `ColumnDefinition41::{fixed_length_fields_length, decimals}` are decoded and never read, but they mirror the MySQL wire struct; left in place. * `bun_sql::mysql::StatusFlags`'s `Display` impl prints nothing, but a debug log in `MySQLConnection.rs` still formats it. <details><summary>Notes</summary> How the candidates were found: the dead-code PRs that were closed for merge conflicts were re-applied against current `main` and every removal re-verified (#37659 applied with no conflicts; its `assertion_error.ts` and `util.ts` hunks had already landed via #39924 and #39628 and are not in this PR). Read-only scans of `src/runtime/shell` + `src/shell_parser`, `src/sql` + `src/sql_jsc` + `src/runtime/node`, `src/runtime/bake` + `src/runtime/api` + `src/runtime/image`, and `src/js` + `src/jsc/modules` produced the fresh items. `src/runtime/node`, `src/runtime/api` (outside `h2/`), `src/runtime/image` and `src/jsc/modules` came back clean. Local test failures, all reproduced with `USE_SYSTEM_BUN=1` (the released bun) in the same container, so none is caused by this diff: * `websocket-server.test.ts`: eight `it.concurrent` cases that overlap the 300,000-message `(benchmark)` test time out under the debug ASAN build in this container (the whole file takes 64 s here against a 6.4 s CI budget); each passes when run with `-t`. * `node-http.test.ts` "request via http proxy": `ECONNREFUSED` from `localhost` resolution in this container. * `bunshell.test.ts` `-c`, `-f character device`, and the seven "stdin redirect from a zero-length buffer" cases: this container's `/dev/null` is a regular file, not a character device. * `sql-mysql-binary-null-indexed.test.ts`: `Access denied for user 'root'@'localhost'` against the local MariaDB. * `fifo.test.ts` "pushing and shifting a lot of items": a 10 s perf budget exceeded under ASAN; the only `fifo.ts` change is the removal of the unused `clear()` method. Files shared with open PRs, different hunks: `src/jsc/bindings/ZigGlobalObject.cpp` / `.h` (#39581 removes other accessors; its `HTMLRewriterSinkPrototype()` hunk sits one blank line above the `JSBufferList` accessors removed here), `Cargo.lock` (#39618 edits other package blocks). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server.test.ts <!-- robobun:evidence:end -->
… the build scripts (#43778) ### Problem - A few C++ and TypeScript items have no caller, no producer, or no effect. - `BUN_MESSAGEPORT_USES_PIPE` (`src/jsc/bindings/webcore/MessagePort.h`) is always `1`. The `#if` around all of `MessagePortPipe.cpp` never excludes anything. ### Fix - WebView: remove `WebViewProto::Reader::u16()` and `f32()` (`ipc_protocol.h`), which have no caller. Remove the CDP `Method` values `RuntimeEnable`, `TargetCloseTarget` and `InputDispatchScrollEvent` with their `case` labels (`ChromeBackend.{h,cpp}`). No pending entry carries them. - WebCore bindings: remove the `BUN_MESSAGEPORT_USES_PIPE` define and guard, the self-alias of `ExtendedDOMClientIsoSubspaces`, the forward declaration of `URLPatternUtilities::URLPatternInit` (no such type), the enumerators `PerformanceEntry::Type::Paint` and `CastedThisErrorBehavior::ReturnEarly`, and 13 lines of commented-out WebKit code. - Build scripts: remove `getBuildNumber()` (`.buildkite/ci.ts`) and `BunOutput.rustObjects` (`scripts/build/bun.ts`). Nothing reads either. - Verified: `rg -w` for each symbol over `src`, `packages`, `scripts`, `test` and `build/debug/codegen` finds no other use. `bun bd` passes. The Notes list the tests. ### Background - `ipc_protocol.h` is the wire format between bun and the WebView host process. `Reader` decodes frames. Every caller uses `u8`, `u32`, `bytes` and `str` only. - `ChromeBackend` tags each pending Chrome DevTools Protocol command with a `Method`, so that the response handler knows which promise to settle. A value that no pending entry carries cannot reach the `switch`. - #29937 added the guard so that `MessagePortPipe.cpp` compiled to nothing while a verification step reverted `MessagePort.h`. Both files are on main now. ### Downsides - None found. Checked each removed symbol for users in C++, Rust, generated code and `src/symbols.*`. The removed enum values are in-memory tags or template arguments. Rust mirrors neither enum. <details><summary>Notes</summary> Smoke tests with the debug build, all pass: `test/js/web/workers/message-port-pipe.test.ts`, `message-channel.test.ts`, `performance-observer-leak.test.ts`, `test/js/node/perf_hooks/perf_hooks.test.ts`, `test/js/web/urlpattern/`, `test/js/bun/webview/webview-chrome-pipe.test.ts`. `clang-format` and `prettier` report no change on the touched files. `.buildkite/ci.ts` still transpiles. Each removal was checked against the diffs of the 34 open dead-code PRs. None of them removes the same lines. `MessagePort.h` is also touched by #40525, in a different hunk. How the Rust side was scanned, and why this PR has no Rust in it: - `cargo mordant` at the revision pinned in `.github/workflows/rust-lints.yml`, over the Linux, Windows and macOS targets, with the `unused_pub` entries taken out of the baseline. It reports 188 `pub` items that nothing in the workspace uses. Every function, method and struct in that list is in one of four groups: an open PR already removes it, a `cfg` that the run does not build uses it (`bun_zstd::inflate_embedded*` under `bun_codegen_embed`, `StoredTrace::from` in `PackageInstall.rs`), only a unit test uses it (`CowSliceZ::init_dupe`), or #42119 left it on purpose (`host_fn_this_value`, `host_fn_setter`, `JsClass::estimated_size`). The constants are flag, errno and syscall-tag tables. - A rust-analyzer SCIP index for five targets (Linux, Windows, macOS, FreeBSD, Android) plus a reachability closure over it. Two blind spots make it unreliable alone: rust-analyzer sets `cfg(test)` by default, which hides `src/runtime/bin_entry`, and references that come from a `macro_rules!` body are not indexed (`comptime_string_map!`, the CSS `to_css` bridges). With those corrected, it agrees with mordant and adds nothing. - Cargo's own `unused_dependencies` warnings: 9 edges. Each is used on another target, or #40294 already removes it. - Rust files outside every module tree: none. Headers that nothing includes: none (the remaining ones come in through `GeneratedJS2Native.h`, `NativeModuleImpl.h` or `include_bytes!`). Also scanned and clean: `src/js/{node,internal,bun,thirdparty}` and `src/js/builtins` (all 50 builtins and all 177 private names have a user), `scripts/`, `misctools/`, `.buildkite/ci.ts`, and the C++ under `webcore/`, `webcrypto/`, `node/` and `src/runtime/` that no open PR touches. Probably dead, left alone on purpose: - `bun_core::strings::split_once` (`src/bun_core/string/immutable.rs`): no caller on any target. It sits next to the hunk in which #40824 removes `rsplit_once`, so a removal here would conflict with that PR. - The raw-list `myersDiff` export of `src/js/internal/assert/myers_diff.ts` and the `Output::List` path behind it in `src/runtime/node/node_assert.rs` (about 115 lines): nothing consumes it. #39924 kept the export on purpose, and Node's own `test-assert-myers-diff.js` needs it if that test is vendored. - `BuiltinName::{redirect, asyncIterator, name, default, fatal, ignoreBOM}` (`src/jsc/lib.rs`, mirrored by position in `bindings.cpp`): never constructed. A removal conflicts with #40232. - `FetchHeaders` guard handling (`removePrivilegedNoCORSRequestHeaders` and the `Guard::Immutable` checks): every construction site passes `Guard::None`, and #43644 removes `setGuard`. This is unreachable code, not unreferenced code. - `ServerTiming`'s constructor and its `durationSet` / `descriptionSet` fields: nothing constructs a `ServerTiming`. #40122 and #41385 touch the same cluster. - The `$isPromiseFulfilled`, `$isPromiseRejected` and `$alwaysInline` codegen macros: no user since 92459cd. - `scripts/build/config.ts` flags `logs` and `baseline`: parsed and printed, never read since the Rust rewrite. `NestedCmakeBuild.{extraCFlags, extraCxxFlags, libSubdir, sourceSubdir, pic}`: no dependency sets them, but `scripts/build/deps/README.md` documents them as the API for future dependencies. - `misctools/gen-unicode-table.ts` and `unicode-generator.ts` emit Zig. Nothing references them, but `src/bun_core/string/identifier.rs` still names the generator as the way to rebuild its tables. - `completions/bun-cli.json` and `misctools/generate-cli-completions.ts`: nothing in the repository reads the JSON. It is still updated by hand, so something outside the repository may use it. </details>
Problem
myersDiff()returned a JS array of{ kind, value }objects, one per line or char.myers_diff.tsbuilt the diff text from it.valueconversion corrupted text.FromAny for &[u8](src/jsc/JSValue.rs) decodes a Latin-1 line as UTF-8, so{ s: "línea" }printed as'l�nea'.FromAny for &[u16]returns an array of numbers, so a diff with emoji or CJK on both sides printed+ 32,32,115,58,...instead of the line.Fix
printMyersDiffandprintSimpleMyersDiffnow render innode_assert.rs, into one Latin-1 or UTF-16 buffer, and return one string (or{ message, skipped }). The JS module only passes theinternal/util/colorsescapes.myersDiff(the raw list) is still exported.Outputcarries the diff mode (lines,check_comma_disparity) next to the render mode, so the two can not be mismatched.test/js/node/assert/assert.test.cjs(three new tests fail on main), the rest oftest/js/node/assert/and the 17 vendoredtest-assert*.jsfiles.Background
deepStrictEqualdiffs the inspected values by line.strictEqualon strings diffs by char, only with colors on, so that test spawns a child withFORCE_COLOR=1.byte_slice()/utf16()onbun_core::Stringgive its code units. Latin-1 bytes above 0x7F are not valid UTF-8.FromAnyis the generic Rust to JS conversion. It has no notion of string encoding.Notes
Differential check against Node: 31
strictEqual/deepStrictEqual/partialDeepStrictEqualfailures (objects, arrays, comma disparity,...collapsing at 6, 7, 8 and 9 equal lines, empty strings, trailing whitespace, Latin-1, UTF-16 and mixed inputs), with and withoutFORCE_COLOR. With this branch the output is byte for byte identical to Node in every case exceptpartialDeepStrictEqual, where Node 26 has a newer gray rendering that main does not have either. Against the current release exactly six cases change: the Latin-1, UTF-16 and mixed cases above.The three new tests that fail on main are the Latin-1 line diff, the UTF-16 line diff, and the colored char diff. The trailing spaces assertion in the Latin-1 test also passes on main. It stays as coverage of the
trimEndport inrender_lines.test/js/node/assert/is 411 tests.The clippy (
needless_pass_by_value) and mordant (bare_bool_args) findings on the first revision are fixed by passingOutputby reference and moving the two flags into its variants.