Conversation
|
Updated 5:32 AM PT - Sep 26th, 2026
❌ @robobun, your commit c762b7b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44050That installs a local version of the PR into your bun-44050 --bun |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (18)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe changes remove JSX inlining state, option fields and related assignments, logging scope declarations, two empty platform-specific modules, and an unused VM pointer field. ChangesRust declaration and state cleanup
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This cleanup removes unused state and declarations without an established behavior change. No actionable merge-blocking risk remains; the PR is mergeable subject to normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — pure deletion of unreferenced log-scope statics, never-read fields, and empty cfg placeholder modules; no behavior change.
What was reviewed:
- Grepped every
scoped_log!/.is_visible()/.log()/path-qualified use of the 17 removed scope names: the only survivors (uwsin us_socket_t.rs/Timer.rs,Bundle/scan_counterinsidemod bv2_impl) resolve to statics that remain declared in the same module. - napi_body.rs: every reference to
posix_platform_specific_v8_apis/uv_functions_to_exportis behind#[cfg(all(not(windows), ...))]or#[cfg(unix)], so removing the windows/non-unix empty stubs leaves no unresolved path on those targets. - Removed fields: no remaining struct-literal constructions of
Features,CommandLineArguments,DebugOptions, or sqlArgumentsSlicename them;c_voidis still used elsewhere in sql_jsc/jsc.rs; the rewrittenmark_binding!comment matches theJSC_SCOPEstatic it now points at.
Extended reasoning...
Single-commit, 20-file, +2/-63 cleanup across 10 Rust crates that deletes declare_scope! statics nothing logs to, four struct fields with no reader (plus their initializers and the dead can_be_inlined/output_file locals), and two empty cfg-gated placeholder modules in napi_body.rs. It touches no security-sensitive surface: the S3/AWS and quic hunks remove only logger statics, not signing, TLS, or credential logic. Static grep confirmed no remaining reference to any removed name on any cfg arm, and the surviving scoped_log! call sites all resolve to statics still declared in their own module; none of the changed paths fall under .github/CODEOWNERS. The bug hunt ran dry, and the diff contains no new logic, so a human review would add nothing beyond what the compiler already enforces.
StatusVerification on c762b7b
CI build 120945 (complete, 177 of 181 jobs passed)
|
Behaviour change: none
Problem
declare_scope!log scopes have noscoped_log!user, four struct fields have no reader, and two emptycfgmodules have no user.pubfield, or a name that starts with_.Fix
napi_body.rs(Notes). Each user of those names is behind the oppositecfg.CommandLineArguments.lockfile,Runtime::Features.jsx_optimization_inlinewith the localcan_be_inlined,DebugOptions.output_file, andArgumentsSlice::_vm.bun bd,bun run rust:check-all(12 targets ok),test/internal/source-lints/, and seven test files (Notes).Background
declare_scope!(NAME, visible)expands to onepub staticlogger thatBUN_DEBUG_NAME=1turns on. It registers nothing, so a scope with no user prints nothing.bundle_v2.rsdeclaredBundleandscan_countertwice. Each user is inmod bv2_impl, which has its own pair. This PR removes the outer pair.Downsides
cargo checkon 12 targets proves that no code names a removed item. No open PR adds a use of a removed scope.Notes
Removed
JSC(bun_core/Global.rs,mark_binding!logs to the hand-writtenJSC_SCOPE),STR,Bundleandscan_counter(outer pair),Store,hot_reloader,SYS(spawn/stdio.rs, with thelog!macro thatdefine_scoped_log!made),GetHostByAddrInfoRequest,CAresNameInfo,GetNameInfoRequest,CAresReverse,CAresLookup,quic_session,S3Client,S3Stat,AWS,uws(uws_sys/socket.rs,us_socket_t.rsandTimer.rshave their own).CommandLineArguments.lockfile: nothing reads or sets it.jsx_optimization_inline: alwaysfalse.can_be_inlinedinparse_jsx.rswas assigned and never read.DebugOptions.output_file: its only source was a local that is alwaysNone.ArgumentsSlice::_vm: always null.SYSscope, minus the items below.Left out, because an open PR holds the same lines
PackageManager.total_scripts: install: remove every unsafe from src/install #40284 still initialises and writes it.PathWatcherManager(win_watcher.rs): Remove libuv on Windows #42819 deletes the file.LibUVBackend(dns.rs): Remove libuv on Windows #42819 removes it. ScopeResolveInfoRequest: the line directly below it.struct_Channeldata(c_ares.rs): dns: remove the remaining unsafe from the resolver module #40190 adds a section header directly after it.CLI(cli/mod.rs): fix(cli): support 'bun node <file>' — node emulation via argv[1] #37277 merges cleanly today and would conflict.ArchiveIterator.filter(libarchive/lib.rs): Bun.Archive: reject when libarchive fails to read an entry header #43242 merges cleanly today, would conflict, and changes the samenext().Tests with the debug build, all pass
test/bundler/transpiler/jsx-production.test.ts(32),test/bundler/transpiler/jsx-tsconfig-react-jsx.test.ts(7),test/js/node/dns/dns-resolver-class.test.ts(6),test/js/sql/postgres-bytea-bind.test.ts(12),test/js/sql/postgres-bind-wire.test.ts(18),test/cli/install/bun-pm-diff.test.ts(46),test/js/bun/spawn/spawn-stdin-readable-stream.test.ts(37).rustfmtreports no change.cargo mordantdid not run locally (the pinned toolchain is not installed here). Removals can only lower its counts.Self-review
The review found each deletion correct and asked for a different unit. What changed:
cli/mod.rs,libarchive/lib.rs).total_scriptsis out, because install: remove every unsafe from src/install #40284 uses it.Verified and held for the next runs (branch
robobun/a11e46ad/dead-code-held, 65 files, 211 lines removed, based on 36cd151)InspectorInstrumentationandJSExecStatecalls,LOG(Network, ...)lines inWebSocket.cpp, and 22static_assert(!std::is_base_of<ActiveDOMObject, T>::value, ...)lines in the WebCore wrappers.CryptoAlgorithmAES_CBC::Paddingwith its ignored parameter, theMediaTimeoverload ofJSConverter<IDLUnrestrictedDouble>, thePLATFORM(WIN)arms inJSURLPattern.cpp, and the#ifndef RENAMED_JSDOM_GLOBAL_OBJECTblock inJSDOMWrapper.h..gitignorelines, andzig-cachein.cursorignore.bun bdand 12 test files on 2026-09-25. The branch is a subset of that set on a newer base. It was not built again.ChunkedEncoding.h(Bun.serve: consume chunked trailer-part instead of rejecting it #32887),JSEventListener.cpp(worker: keep a TerminationException pending through microtask jobs, event listeners, and emit() #40128),misctools/package.json(Remove the Zig-emitting unicode table generator from misctools #41473), one hunk ofWebSocket.cpp(WebSocket: bound every script-supplied value that an error message quotes #42216),BunClientData.h(node:worker_threads: answer getHeapSnapshot() and startCpuProfile() while the worker runs synchronous JavaScript #42388),epoll_kqueue.c(Remove libuv on Windows #42819),hmr-runtime-server.ts(dev server: register a client component without evaluating it #42934).No user found, left alone on purpose
JSDOMConvertSequences.h: theExceptionThroweroverloads of the sequence converters (about 115 lines) and thestd::arrayspecialization ofSequenceTraits(32 lines). EachConverter<T>has the same overload set, so this is an API shape.#[derive(IsCompatible)]insrc/css_derive/lib.rs(136 lines): no type derives it. The header of that file names it as one of five derives that a migration relies on.