Repository navigation
Conversation
Remove two lsquic settings setters that no code calls, with their C shims: Settings::scid_len (us_nq_settings_set_scid_len) and Settings::delay_onclose (us_nq_settings_set_delay_onclose). Remove two Cargo dependencies that no file of the crate names: thiserror in bun_trim and bun_sema_parser in bun_sema_standalone.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
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 |
|
Updated 8:06 AM PT - Oct 29th, 2026
❌ @robobun, your commit 3b37d3a has some failures in 🧪 To try this PR locally: bunx bun-pr 47355That installs a local version of the PR into your bun-47355 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM, a small mechanical dead-code removal that is consistent on both sides of the FFI boundary.
What was reviewed:
- Grepped the whole repo for
scid_len/delay_oncloseafter the change: the only remaining hit is an unrelated local variable insrc/runtime/node/quic/endpoint.rs, so no Rust, C, C++, or cfg-gated caller references the removed shims orSettingssetters. - Checked
src/trim/error.rs: its error enums use hand-writtenFromimpls andstrum, nothiserrorderive or#[from], and no file undersrc/trim/(includingtests/) namesthiserror. - Checked
bun_sema_standalone, including the#[path]-includedsrc/runtime/cli/typescript_libs.rs: nothing namesbun_sema_parser. Cargo.lockremoves exactly the two dependency edges; bothbun_sema_parserandthiserrorpackages remain for their other dependents, with no stray churn.
Extended reasoning...
The change is a single commit of 10 deletions across five files: two unused QUIC engine-settings setters removed from the C shim (packages/bun-usockets/src/node_quic_shim.c), their matching extern "C" declarations and settings_setters! rows in src/lsquic_sys/lib.rs, and two unused crate dependencies dropped from bun_trim and bun_sema_standalone with the corresponding Cargo.lock edges. It touches the lsquic FFI surface only by deleting unreferenced symbols; no security-sensitive logic, auth, crypto, or input handling changes. Approval rests on a full-repo grep confirming no remaining reference to any removed item and that the two crates have no derive-macro or test-only use of the dropped dependencies, plus the absence of any CODEOWNERS coverage or outstanding objections on the changed paths.
|
Status: ready for review. The diff is green on every job that ran. CI (build 135129):
How this was checked:
The pull request body has the full list of what the sweep covered. |
Behaviour change: none
Problem
Settings::scid_lenandSettings::delay_onclosehave no caller (src/lsquic_sys/lib.rs). node:quic on lsquic — Node v26 compat, HTTP/3 #32602 added them with their C shims,us_nq_settings_set_scid_lenandus_nq_settings_set_delay_onclose. No commit ever called them.bun_trimdepends onthiserror, andbun_sema_standalonedepends onbun_sema_parser. No file of either crate names the dependency.Fix
settings_setters!rows, their twoextern "C"declarations, and the twoNQ_SET(...)lines inpackages/bun-usockets/src/node_quic_shim.c.Cargo.locklines.rg 'scid_len|delay_onclose' src packagesnow finds one local variable and nothing else. The linker finds no reference to either shim.bun bdbuilds andbun run rust:check-allpasses on 12 targets.bun bd testrantest/js/node/quic/(3 files) andtest/js/bun/http/serve-http3.test.ts.Background
bun_lsquic_syswraps lsquic fornode:quicand HTTP/3. Eachsettings_setters!rowname => c_function : typebecomes aSettings::name(v)method that calls the C shim.NQ_SET(field, type)defines that shim.--gc-sections --print-gc-sections. The output lists each function that no root reaches. Both shims are on that list.Downsides
rg, the history of the two setters withgit log -S, and the opennode:quicpull requests (quic: run lsquic_global_init once per process #42913, node:quic: open a uni stream requested before the handshake, keep pending streams paired with lsquic's requests #45342, node:quic: create the endpoint socket without IP_RECVERR and count a refused datagram as sent #44800). None of them calls a removed setter.Notes
Removed, one line each
Settings::scid_lenandus_nq_settings_set_scid_len(table row,extern "C"declaration,NQ_SET(scid_len, unsigned)): on main,rg scid_len src packagesfinds these three lines and a local variable insrc/runtime/node/quic/endpoint.rs. Build passes.Settings::delay_oncloseandus_nq_settings_set_delay_onclose(table row,extern "C"declaration,NQ_SET(delay_onclose, int)): on main,rg delay_onclose src packagesfinds these three lines and nothing else. Build passes.thiserrorinsrc/trim/Cargo.toml: on main,rg -w thiserror src/trimfinds only the manifest line.cargo check -p bun_trim --all-targetspasses.bun_sema_parserinsrc/sema/standalone/Cargo.toml: on main,rg -w bun_sema_parser src/sema/standalone src/runtime/cli/typescript_libs.rsfinds only the manifest line (the second path is the file thatlib.rsincludes with#[path]).cargo check -p bun_sema_standalone --all-targetspasses.cargo metadata --lockedaccepts the editedCargo.lock.Test and build runs
bun bd, the tests andbun run rust:check-allran on e64b59d. After the rebase onto da867c6,cargo check --all-targetsran again for the three crates andbun_runtime.test/js/node/quic/quic-stream.test.ts: 13 pass.quic-endpoint.test.ts: 9 pass andquic-sni.test.ts: 11 pass, each on the second run. The first run, directly after the build, had 2 and 1 failures.test/js/bun/http/serve-http3.test.ts: 82 of 84 pass in the full run. The other two hit the 5 s timeout of the debug build.basic GETpasses alone.Response(subprocess.stdout) streams over H3passes with--timeout 60000, in 4.2 s.What the sweep covered
__wrap_*shims, the vendored llhttp API, destructors, or code that lldb calls (dumpBtjsTrace).comptime_string_map!,env_varaccessors, the generated AOT schema),const fnthat run at compile time, or code for other targets.pubitem that no other crate names becamepub(crate). Thendead_coderan with default features, with all features, and with all features in the test profile. An item counts only if all three runs report it.bun_sema: 484 items demoted, 0 dead.bun_exe_format: 0 dead.bun lint,bun fmt,bun_trim,bun_tar,bun_md,bun_parsers: 2 findings, both in Remove dead code from bun_bundler, bun_install, bun_runtime, bun_parsers, and the JSC private host functions #41088.WriteKind,MaybeExtandEmptyCopyFileStatehave callers on other targets.CowSliceZ::init_dupeexists for a unit test.Tag::fchownatis a row of a frozen table.bun_sourcemapreads the two fields ofSerializedSourceMapLoadedthrough a struct with the same layout. The eighth is thefallthroughfield below..rsfiles outside every module tree (0), workspace crates with no dependent (0), unused[workspace.dependencies]entries (0), Cargo features that nothing enables (4, all intended),tsc --noUnusedLocalsonsrc/js,src/codegen,src/runtime/bakeandscripts, and blocks of commented-out code (12 blocks, the largest is in Remove dead code from the build scripts, the vscode and debug-adapter packages, the bake overlay, and two WebCore files #40122).Seen, not changed
ScopeBlockInfo::Begin.fallthrough(src/react_compiler/hir/visitors.rs) is written and never read. It is probably dead. It is not in this diff because the removal also edits each writer.cargo checkwarnsunused dependencyon Windows and FreeBSD targets forbun_semver(inbun_analytics),bun_which(inbun_crash_handler),bun_spawn_sys(inbun_io) andbun_analytics(inbun_spawn_sys). Other targets use them, so they are not dead. A target-specific dependency table would silence the warnings.