Repository navigation
Conversation
|
Updated 10:26 AM PT - Oct 20th, 2026
❌ @robobun, your commit ce048b8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 45988That installs a local version of the PR into your bun-45988 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes remove ChangesTrait Implementation Removals
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: removal of eight unused trait implementations across bun_bundler, bun_core, and bun_css. |
| Description check | |
The description explains the problem, fix, scope, behavior impact, background, risks, and verification results. It does not use the template headings exactly, but it provides the required information … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the problem, fix, scope, behavior impact, background, risks, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Status: ready for review. How the deletions were verified:
The first draft of this sweep had 57 impls. A self-review took 49 out: open pull requests use or already delete some, and earlier sweeps set the others aside for a maintainer decision. The pull request body lists each group. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — pure deletion of eight unused trait impls; a repo-wide search found no == on StableRef and no Display/Error consumer ({} formatting, dyn Error, #[from]) of InvalidCharacter, ColorError or PrintErr, so the removal is behavior-preserving. The inline nit about the matching InvalidDate pair is optional follow-up, not a blocker.
Extended reasoning...
The commit removes 30 lines across src/bun_core/fmt.rs, src/bundler/bundle_v2.rs, src/css/css_parser.rs and src/css/lib.rs, adding nothing; it touches no security-sensitive surface. Grepping src/ shows every parse_double/ColorError/PrintErr consumer only propagates or discards the error (e.g. map_err(|_| ()), Err(_) =>), and the only StableRef consumers call is_less_than in sort comparators that never use equality, which is what decided approve. The changed files have no CODEOWNERS entry and the timeline shows no outstanding reviewer objection. Compilation across targets was not run here; the conclusion rests on the search of callers plus the author's stated rust:check-all run.
…e and bun_css Delete four hand-written impl pairs that no code needs: PartialEq and Eq for StableRef, and Display and Error for InvalidCharacter, ColorError and PrintErr. The workspace compiles for every target without them.
4701048 to
ce048b8
Compare
This branch has not been deployed
2 participants
Behaviour change: none
Problem
PartialEqandEq for StableRef(bun_bundler), andDisplayandErrorforInvalidCharacter(bun_core),ColorErrorandPrintErr(bun_css).dead_codeskips impls.Fix
StableRefwith==or formats one of the three errors.mainat ad4089b:bun run rust:check-all,bun run rust:clippy,cargo check --workspace --all-targets, a release-profile check,cargo fmt --all --check,bun bd, and 8 test files (Notes).bun_lintimpls here and ask in the body (carved out, as in Remove dead code from bun_sema and bun_format: 6 trait impls nothing uses #45430).Background
OrdandPartialOrd for StableRefandFrom<InvalidCharacter> for CrateError. These impls are what it left beside them.Errorimpl goes together with itsDisplay. No merged sweep removed that pair before.mainhas no merge queue, so an open pull request that uses a removed impl breaksmainwhen it merges. The Rust lines that all 5,936 open pull requests add do not use these impls.Downsides
StableRefcomparisons,parse_doublecallers and formatted css errors found none.Notes
What the search found
impl Trait for Typeblocks became blank lines (3,081 of the 4,812 in the workspace, in two passes).cargo check --workspaceran, and each impl that an error named went back. This repeated until the workspace compiled. The set then went throughx86_64-pc-windows-msvc,aarch64-apple-darwin,--all-targets, the debug cfgs, a release-profile check, and--features bun_react_compiler/fixtures.Drop,Deref,DerefMut,Clone,Copy,Send,Sync,Unpin,Debug,Any,Fn*,Future,GlobalAlloc,Allocator, everyunsafe impl, blanket impls, impls whose self type derefs or is a reference, a trait implemented for bothTand&T, and impls that share a member name with an impl of another trait for the same type.The other 122
main: Remove dead code from bun_core, the JSC bindings, the debugger, and 24 other crates #41385, Remove dead code from the WebCore bindings and 20 Rust crates #40525, Remove dead code from bun_url, bun_react_compiler, the class codegen, and bun-error #41169, Remove dead code from bun_jsc, bun_core, the bindgenv2 codegen, 15 error types, and the C++ bindings #40915, Remove dead code from bun_runtime, bun_core, bun_ast, bun_io, and an orphaned package #40969, Remove dead code from the inspector protocol package, bun_jsc, bun_install, and the Windows named pipe modules #40557, Remove dead code from bun_css, bun_react_compiler, bun_jsc, and smaller crates #40690 and Remove dead code from bun_js_parser, bun_sys, bun_perf, the bake router, and the root-certs script #41127. One pair among them is the sibling of a pair in this diff:DisplayandError for InvalidDate(src/bun_core/wtf.rs) are in Remove dead code from the WebCore bindings and 20 Rust crates #40525.Default for SendWindowandRecvWindow(h2/flow_control.rs): node:http2: withhold connection window credit after setLocalWindowSize() decreases it #41344 callsRecvWindow::default(), and Remove dead code from the code generators, the build scripts, and ten crate roots #45709 deletes both.Display for Part(bake router): Remove dead code from bun_js_parser, bun_sys, bun_perf, the bake router, and the root-certs script #41127 deletes it at the path it had before bake: move the code into folders by job (no behavior change) #45489.EqandHash for PropertyId: css: compare custom property names inPropertyIdequality and importance in duplicate-rule detection #33684 callsproperty_id().hash(..).EqandPartialEqpartners of such an impl:Display for CowSliceZ,Default for Ordinal,From<T> for JsCell<T>,Display for Difference,FromIterator for IdMap.bun_lint:HashforSegment,EsNode,Reference,SignatureandTsSymbol, with theirEqimpls andPartialEq for Signature.From<Pat> for TargetandFrom<DeclarationKind> for DeclarationKinds.Default for AtomicCell<T>andRacyCell<T>. A unit test in open RacyCell: replace the unconditional Sync impl with an owner-thread runtime check #31178 callsRacyCell::default().PartialEqandEq for GenericIndexOptionalandIdBitSet.ExactSizeIteratorfor theBabyVecIntoIterand forIterMut,Keys,ValuesandValuesMutofzig_hash_map, and for five iterators ofbun_lint,bun_formatandbun_sema.PartialEqandEq for ExternalSlice(its comment calls the type comparable), for the lintSourceFile(the rule docs say every handle compares by identity), and forRefAccessRefType(react-compiler: fix N^4 compile time for N nested functions #43530 edits it).Display for ReturnCode(bun_libuv_sys, Windows only, and Remove libuv on Windows #42819 removes the crate),Eq for Reference(bun_sema) andError for SyntaxError(bun_lint), which are markers with no partner to delete, andDoubleEndedIterator for TypeIter(bun_lint).Eq for BorderStyle,BorderBlockStyleandBorderInlineStyle. TheirPartialEqis derived, soclippy::derive_partial_eq_without_eqasks for them.cargo checkdoes not see that.bun run rust:clippydoes.css_deriveandsrc/css/generics.rsname:CssEql for str,CssHash for boolandstr,IsCompatible for Option<T>,ParseWithOptions for Option<T>. The same forEmit for f64andLocated for NameOf<Node>inbun_lint.Write for Box<W>andLocated for &File: the self type derefs or is a reference.PoolStorage for UnwiredStorage: the documented panic for anObjectPoolwith no storage.Display for ErrorLocation: the only reader ofErrorLocation.filename.Functions, types and trait methods
#[deprecated(note = "DP:<id>")]on an item in a scratch tree makes rustc report each use site, with no change to name resolution.pub fn(10,838): 50 have no use on Linux, Windows, macOS, in tests and under the debug cfgs. Open pull requests delete 36 of them. Open pull requests use two more: fs.watch: watch symlink entries of a recursive tree on Linux #43090 callsposix::s_isdir, and Remove libuv on Windows #42819 renames the Windowsumaskand calls it. The other 12 are named bysrc/codegen/generate-classes.ts, used only under another cfg or in release builds, gated to FreeBSD or Android, or are small leads (is_not_semicolon_tokeninbun_lint, thewindows_enable_stdio_inheritancedeclaration inbun_windows_sys).pubstruct, enum, union, type alias, trait, const and static (7,347): 109 have no use outside their own definition and impls. They are constants of FFI tables, items gated to FreeBSD or wasm, and two types that Remove dead code from bun_parsers, bun_core, bun_css, and the base64-js fallback #41004 removes.pub trait(840): 11 have no caller. Open pull requests delete 9.src/js(216 files) andscripts/(175 files) were read for unused declarations. Two small leads that no open pull request covers:defaultCwdinsrc/js/builtins/shell.tsandassertEncodingForWindowsinsrc/js/node/fs.tsare never assigned.Concerns from the self-review that are not addressed here
HashandFromimpls of thebun_linthandles stay? The 34 impls of the two groups above would go into it.unused_pubwould stop the class from growing back.bun_lintimpls in this diff and ask at the top of the body. They are carved out instead, as in Remove dead code from bun_sema and bun_format: 6 trait impls nothing uses #45430.Verification
cargo check --workspace,bun run rust:clippy,cargo check --workspace --all-targetsandcargo fmt --all --checkran again and pass.bun run rust:check-all: 12 ok, 0 failed, 0 skipped.bun run rust:clippy,cargo check --workspace --all-targets,cargo check --workspace --releasewith the release codegen and--cfg bun_codegen_embed,cargo fmt --all --check.bun bd, then with the debug build:test/js/bun/css/css.test.ts,color.test.ts,css-loader.test.ts,test/bundler/bundler_banner.test.ts,bundler_splitting.test.ts,test/js/bun/toml/toml.test.ts,test/js/bun/resolve/toml/toml-parse.test.ts,test/js/bun/yaml/yaml-block-scalar-matrix.test.ts. All pass.test/js/bun/css/as a whole (20 files): 2512 pass, 3 fail. The three are the loops ofcss-fuzz.test.ts. The test gives each a fixed 10 s limit, and the debug build does not finish them inside it.StableRefoccurs in the added lines of 10 (import lists and two comments).InvalidCharacteroccurs in 8 (theParseIntErrorvariant, import lists, comments, and another struct of that name in compile: verify the downloaded target executable against the registry integrity #36173).PrintErroccurs in 27 (import lists, types, and onePrintErr::CSSPrintError).ColorErroroccurs in none.