Repository navigation
Conversation
…arms that cannot run
|
Updated 11:32 AM PT - Oct 20th, 2026
✅ @robobun, your commit 0f2f9e31839015a7c586bdf134b2ee0280903472 passed in 🧪 To try this PR locally: bunx bun-pr 45974That installs a local version of the PR into your bun-45974 --bun |
|
Status: ready for review. CI is green.
|
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.
Beyond the inline comment, I also checked a few of the deletions for hidden behaviour changes and found none: both register_for_epoll callers passed one_shot=true and the Flags::OneShot reads that remain are on the separate posix_event_loop flag set; HostedGitInfo::from_url on git:///git+http(s):// input never yields a Shortcut representation, so hgi_to_tag could only return Tag::Git; and Bits::drain in the GIF decoder only advanced a cursor that decode_frame returns without reading.
Extended reasoning...
Dead-code sweep across 40 Rust files plus one C++ extern removal, with no security-sensitive surface touched (the cron.rs changes only drop unreachable OOM arms around infallible Vec writes). The one posted finding is a stale SAFETY comment in linker.rs that still names the deleted preserve_extensions field; the ruled-out items above were re-verified from the code rather than the PR description.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe pull request removes a progress option and several public error variants. Git dependency inference no longer looks up hosted-provider metadata for specified URL forms. S3 signing results no longer store the requester-payer option. ChangesProgress Rendering
Git Dependency Tag Inference
Public Error Variants
S3 Signing Result
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
…n bun_core::printer
|
The review found one stale comment: the SAFETY comment of 341e416 answers the two comment-length notes: the doc comment of No code changed in these two commits. The diff is now 41 files, +93 and -435 lines. |
There was a problem hiding this comment.
I re-reviewed the current head after the follow-up commits and found no bugs; the stale preserve_extensions reference in the Linker::options SAFETY comment is now updated. Because this sweep spans 41 files including macOS/Windows-only cron paths and Linux epoll code that cannot be type-checked from one target, a human look is still worthwhile.
What was reviewed:
Tag::infergit-URL fallback: tracedHostedGitInfo::from_url→hgi_to_tag;Representation::Shortcut(the only path toTag::Github) requires agithub:-style protocol, whichgit:///git+http(s)://inputs never produce, soTag::Gitis the only outcome.Flags::OneShotremoval insrc/io/lib.rs: the remainingFlags::OneShothits are on the separateposix_event_loop::Flagsenum;register_for_epollispub(crate)with only the twotruecallers.- Grepped the whole tree for
Bun__allocUint8ArrayForCopy,IS_MAIN_THREAD_VM,ImportWatcher::None,ArrayBuffer::alloc::<, the removedjsc::Errorvariants andstrings::Encoding::{Ascii,Utf16}— no remaining references (theEncoding::Utf16hits are the separatejs_printer,bun_ast::e::StrEncodingandStandaloneModuleGraphenums). codec_gifBits::drain,ansi_renderersrc/title restores (fields are.take()n before), the non-Windowsdrop(iter)inprune.rs, and thenode_fsdid_succeed/phaselocals: confirmed nothing reads the removed state afterwards.
Extended reasoning...
The change is a dead-code removal sweep across 41 files (+93/-435): write-only struct fields, never-constructed enum variants, infallible functions losing Result wrappers, a thread-local, one C++ export, and an always-true epoll parameter. It touches no auth, crypto, or input-validation logic; the s3_signing change only drops two unread SignResult fields and the cron.rs changes only remove unreachable OOM arms. Every removal I could trace statically checks out as dead, and the one nit from the prior review was addressed in f43c374. Deferral rather than approval is because of the size and the amount of cfg-gated macOS/Windows/Linux-only code whose compilation I could not verify here; no CODEOWNERS entry covers the changed files and no third-party objection is open.
|
I moved this PR back to draft. Do not merge the current head (341e416). A review of the diff against the heads of the open pull requests found that some of the removed items are used by open work. My first check compared only the lines that open PRs remove or change. It did not search the lines that they add, and it did not run trial merges. Open PRs that use an item this PR removes (each one merges without a conflict and then does not compile):
Removals that add a merge conflict to open PRs: the parser's I will put these items back on this branch, run the checks again, check the trimmed diff with a trial merge against every open PR head, and then rewrite the description. |
There was a problem hiding this comment.
LGTM — the trimmed-down removal set is behavior-preserving; each deletion checks out against the code.
What was reviewed:
- Traced
Tag::infer's droppedHostedGitInfo::from_urlfallback: forgit://andgit+http(s)://inputs,normalize_protocolyieldsGit/GitPlusHttp(s),is_github_shorthandrejects on the early:, andcorrect_urlcan only produceGitPlusSsh, soRepresentation::Shortcut(the only route toTag::Github) is unreachable and the result was alwaysTag::Git. - Read
ModuleInfo::finalizeend to end: no?orErr, so the three dropped error arms (Allocmapping,Nonereturn,module_info = None; continue) could not run. - Confirmed the remaining
MissingDebugInfo/EndOfFileuses insrc/jsc/btjs.rsarebun_crash_handler::Error, and the"InvalidDataURL"/"TarballFailedToExtract"string matches inResolveMessage.rs/AsyncModule.rscome frombun_install::Error::name(), not the removedbun_jscvariants. - Checked
SignResult.request_payerreaders (only thesign_requestlocal and the cache-key struct) anddont_print_on_dumb(no setter anywhere in the repo).
Extended reasoning...
The change is a pure deletion (+4/-58) across six Rust files: a write-only Progress flag and its early-return, an infallible Result wrapper on ModuleInfo::finalize plus its three dead error arms, two always-equivalent hosted-git lookups in Tag::infer, five never-constructed bun_jsc::Error variants, and a write-only SignResult field. It touches the S3 request-signing file but only removes a field that is written and never read; the x-amz-request-payer header emission still reads the local variable and the credentials/options structs keep their own field. Every claimed-dead item was verified by grep and by reading the hosted_git_info parsing path, so there is no behavior change to test. The diff has shrunk substantively since the earlier reviews (later commits restored items colliding with open PRs, mooting the earlier SAFETY-comment nit), no CODEOWNERS entry covers these files, and no CHANGES_REQUESTED review is outstanding.
|
The trim is done, and the PR is ready for review again at c2b7b22. The title and the description now describe this head.
|
|
I merged
|
There was a problem hiding this comment.
LGTM — since the last review the branch merged main and put ModuleInfo::finalize back, leaving a 43-line deletion-only diff across four files, and each removed item checks out as dead.
What was reviewed:
Tag::infer: tracedHostedGitInfo::from_url→hgi_to_tagforgit://andgit+http(s)://input;Representation::Shortcutonly arises from a shortcut protocol oris_github_shorthand, which rejects any input with a:before#, andcorrect_urlcan only yieldGitPlusSsh, so the removed lookups could never returnTag::Github.bun_jsc::Errorvariants: grepped forError::,CrateError::andbun_jsc::CrateError::constructors/matches workspace-wide; the onlyMissingDebugInfo/EndOfFilehits are inbtjs.rs, which importsbun_crash_handler::Error, andResolveMessage.rs/AsyncModule.rsmatch onname()strings sourced from the resolver/install enums.SignResult.request_payer(pub(crate)) has no reader insrc/s3_signing; thex-amz-request-payerheader is still emitted from the local.dont_print_on_dumbhas no reference anywhere in the repo, and bothProgressliterals use..Default::default().- Minor: the PR description still says 58 lines in 6 files including
finalize; the diff is now 43 lines in 4 files.
Extended reasoning...
The diff removes a never-set Progress field and its early return, two redundant HostedGitInfo::from_url lookups in Tag::infer, five never-constructed bun_jsc::Error variants, and a write-only SignResult.request_payer field. It touches S3 request signing, but only by dropping a field that nothing read; the header emission path is unchanged, and no struct involved is repr(C) or mirrored in C++. The bug hunt ran dry, no CODEOWNERS entry covers these files, and the timeline shows no CHANGES_REQUESTED review; the commits since the last bot review narrowed scope (reverted the finalize change) rather than adding risk. Each deletion was independently confirmed dead by grep and by tracing the hosted-git-info protocol/representation logic, which is what decided approve.
Behaviour change: none
Problem
Progress.dont_print_on_dumbisfalsein everyProgress.Tag::infermakes two hosted-git lookups that cannot change its answer. Five variants ofbun_jsc::Errorare never constructed.SignResult.request_payeris written and never read.dead_codelint skipspubitems and counts an assignment to a field as a use.Fix
mainreads, sets or constructs them, and the 12 CI targets compile without them.bun bd,cargo check --workspace,cargo clippy,cargo fmt --check, 5 test files.bun run rust:check-alland--all-targetsran on the head before the merge ofmain(Notes).Background
Tag::inferclassifies a dependency specifier. For agit://orgit+http(s)://URL it also askedHostedGitInfo::from_url.hgi_to_tagmaps that result toTag::Githubonly for a shortcut such asgithub:user/repo, which these URLs never are. Every other result isTag::Git, which the next statement returns.Downsides
Notes
Removed
bun_core::Progress.dont_print_on_dumb(src/bun_core/Progress.rs): the field, its default, and the early return inrefresh_with_held_lockthat tests it. No code sets it, so it isfalsein everyProgress. The last tree with the Zig sources (d4514457e8^) never set it either.Tag::inferinsrc/install/dependency.rs: the twoHostedGitInfo::from_urllookups forgit://andgit+http(s)://inputs.hgi_to_tagreturnsTag::Githubonly when the representation isShortcut.from_urlgivesShortcutonly for the protocolsgithub:,gist:,gitlab:,bitbucket:andsourcehut:, or for auser/reposhorthand.is_github_shorthandrejects an input with a:before a#, andnormalize_protocolreadsgit:,git+http:orgit+https:from these inputs.Tag::Gitor fails, and both cases reachreturn Tag::Git.bun_jsc::Errorthat nothing constructs (src/jsc/error.rs):InvalidDataURL,InvalidURL,TarballFailedToExtract,MissingDebugInfo,EndOfFile, with theirname()arms. Other crates have variants with the same names in their ownErrorenums. The enum has noreprand no conversion from a number or a string.bun_s3_signing::SignResult.request_payer(src/s3_signing/credentials.rs): the field, its default and its one store.sign_requestwrites thex-amz-request-payerheader from its local of the same name.Open pull requests
git merge-tree) shows that the branch adds no conflicted file and no conflict hunk.dont_print_on_dumb,request_payeron aSignResult, and the five variant names inbun_jsc.Found dead in this run, and left alone
The first push of this PR removed 24 items (435 lines). My check of the open pull requests was too weak then: it compared only the lines that they remove or change. The trial merge above, run for that first head, adds a conflicted file to 32 open pull requests (11 of them merge clean today) and more conflict hunks to 20, 49 pull requests in total. Open pull requests also use eight of the removed items. Each item below has no reader or constructor on
mainand compiled away on 12 targets. A later run can take it when its blocker lands or closes.An open pull request uses the item:
ResultofModuleInfo::finalizeinjs_printer, which is neverErr, with the error arms of its three callers. It was in this PR until 2026-10-20. Build every runtime ES module's record from the transpiler output instead of a second JSC parse #35605 now adds a caller that tests theResult, and it edits the function.BundleOptions.code_coverage, whichbun test --coveragesets and nothing reads: bun test: count code that runs in a Worker under --coverage #43283 and bun test: ignore the next original source line in coverage #44228 read it. Remove dead code from the class generator: the unreachable DOMJIT paths #43840 held it for the same reason.IS_MAIN_THREAD_VM,set_is_main_thread_vmand its six writes: fuzzilli: run every REPRL program on a fresh global object #42027 adds a call.bun_jsc::Error::ParserError: Import C:.cfiles as modules, compiled by Bun and JavaScriptCore's B3 #42671 constructs it.ImportWatcher::Noneinsrc/jsc/hot_reloader.rs: watcher: watch a file before it is read, not after it is parsed #42749, watcher: add a stat-polling backend for --watch and --hot #42750 and bun test --watch: run a test file that is added after the first run #42810 each add an arm for it.picohttp::Request.minor_versionandbytes_read: HTTP client: remove unsafe from src/http (h1/h2/h3 client, proxy tunnel, HTTP thread, keep-alive pool) #40385 and picohttp: add lifetime parameter to Header #31179 build aRequestwith both. http: replace picohttpparser with a Rust parser; classify headers once and build fetch Response headers lazily #37132 rewrites the struct.bun_core::strings::Encoding::{Ascii, Utf16}: Escape 16-bit strings and object keys like 8-bit ones (console.log, bun:test, JS printer) #42320 and Deduplicate bundler dispatch and output glue, the JS string escaper, and the exports-map retry #31998 add arms for them.src/runtime/api/cron.rs, theResultofxml_escape,alloc_print_zand the XML and calendar helpers, with nine "Out of memory" arms (about 120 lines): fix(cron): preserve DOM/DOW syntactic restriction in OS-registered jobs #29143, fix(cron): write macOS launchd logs to ~/Library/Logs, not /tmp #29144 and fix(cron): set working directory to caller's cwd at registration time #29147 call them with?ormap_err.filter_crontabhas the same shape.The removal adds a merge conflict to an open pull request:
Options.transform_only: bun test: Vitest compatibility #44833, bun:dom and the Vitest work in one tree (not for merging) #45415, Remove dead code from bun_bundler, the JS parser, the help command, and PackageJSONEditor #44687, parser: require the tsconfig options in ParserOptions::init, fix scanImports on decorators and accessor fields #45557.Options.keep_names(alwaystrue): js_parser: validate TypeScript class member modifiers like esbuild #40817, Delete the schema::api mirror types and bun_api; one loader numbering across Rust/C++/JS #37095.one_shotparameter ofio::Poll::register_for_epollandFlags::OneShot: Remove dead code from js_printer, bun_io, and UpdateRequest #44293, Replace boolean flag parameters with two-variant enums #37180, FileSink, Bun.write: remove unsafe from FileSink.rs and blob/write_file.rs #40213, Blob, Bun.file: remove unsafe from Blob.rs, read_file.rs, copy_file.rs and Store.rs #40221, S3 client + Bun.WebView host: remove unsafe from webcore/s3 and webview #40252, webcore: remove unsafe from Response/Request/Body/ReadableStream/FileReader/Sink #40411, fix(io): emulate EPOLLONESHOT with DEL + ADD #40473, io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787.BundleOptions.preserve_extensions: Make --preserve-symlinks and --preserve-symlinks-main work in the runtime and bundler #36757.SignResult.acl: s3: sign without fixed-size buffers, write each output once #45440.SignResult.storage_classsits between the same lines and is also only written.EventLoop.waker: Remove libuv on Windows #42819.KINDofArrayBuffer::allocwithBun__allocUint8ArrayForCopy: Make unrooted JSValues, views across JS re-entry, and JSC buffer-ownership contracts compile errors #34230, Bun.DurableObject #42984, Rename Zig-prefixed identifiers to Bun/Rust equivalents #30747, Remove cross-crate layering workarounds #33152. Remove dead code from react_compiler, the WebCore/IDL bindings, install_jsc, node-fallbacks, and misc crates #39420 kept theUint8Arrayarm on purpose, so thatallocmirrorscreate. Remove dead code from JSWebSocket, ArrayBuffer::alloc, and Subprocess #46140 is now open and removesBun__allocUint8ArrayForCopy.Scanner.exclusion_namesinbun test: bun test --watch: run a test file that is added after the first run #42810.phaseanddid_succeedinread_file_with_options: fs.readFile: honor AbortSignal between read chunks #36259, node:fs: stop a readFile of a pipe or device when its worker is terminated #40207.WrappedIterator.name_filteron non-Windows targets: Resolve readdir entries of unknown type in the remaining consumers (create, bin links, bunx, fs.watch, cp, ls, init) #38961.Bits::drain()incodec_gif.rs: Add space, channels, and hasAlpha to Bun.Image metadata() #31899 edits the test line beside the comment that names it.EXCEPTION_HOLDERinjs_parser_jsc/Macro.rs: macros: run every macro on one dedicated VM thread #40059.image_srcandimage_titlerestores inmd/ansi_renderer.rs: Bun.markdown.ansi: show image URL and cap Kitty image width #29131.G::Comment.loc: js_parser: keep @license and @preserve comments, and keep legal comments across import() #41013.Framework.is_built_in_reactandFrameworkServerComponents.separate_ssr_graphin the parser's copy of the framework options: Remove cross-crate layering workarounds #33152.How the candidates were found
#[deprecated]tag, andcargo check --workspacefor Linux, Windows and macOS listed every use site. A field with only initializer and assignment sites is write-only.BundleOptions.debuggerandSignResult.storage_class.--gc-sections --print-gc-sections(131 unreachable C-linkage functions, 101 of them already in open pull requests, the rest platform code), clang with-Wunused-functionover the C++ bindings, files in no mod tree or build target, unused Cargo dependencies.Leads, not applied
State with a missing reader or writer. The audit reported these, and I did not verify them:
virtual_machine::Options.dns_result_orderis set for compiled executables and never copied into the VM. Forward --inspect and --dns-result-order to the VM in compiled executables #40703 is open for it.[debug].editoris stored and never read. Bun.openInEditor: use the [debug] editor setting from bunfig.toml #38059 is open for it.js_parser: the deferredawaitandyielderrors of arrow arguments go into a copy that is thrown away.js_parser:PropertyOpts.declare_rangeis never set.ParseResult.pending_importsis always empty, so nothing feeds theAsyncModulequeue.Bun.build:Config.dirandConfig.css_chunkingare never set from JS.bun:fficc():library_dirsis never assigned. bunfig[bundle].packagesis stored and never read.extract_blobinspawn/stdio.rscompares the stdio slot index with itself.Unfinished work, not dead code:
P::keep_expr_symbol_namereturns its argument unchanged. It is the--keep-nameshook, and #40503 implements that feature.The same class as
ModuleInfo::finalize, for a later run:Stdio::as_spawn_option,Log::clone_to_with_recycled, and theStreamBufferandByteVecExtwrites never returnErr.JsSinkType::HAS_FLUSH_FROM_JSistruein all six impls.Stdio::Memfdexists only as stdin. Open pull requests edit the same lines (#42819, #43896, #44301, #44493, #45591, #43709, #43737).Verification
mainat ea6a2c7):bun bd,cargo check --workspace,cargo clippy --workspace --no-deps,cargo fmt --all -- --check.finalizeitem on top of 5900354:bun run rust:check-all(12 ok, 0 failed, 0 skipped (of 12)) andcargo check --workspace --all-targets. The 13 commits ofmainsince then add no line that names a removed item.Tests, debug build of this head
test/cli/install/hosted-git-info/from-url.test.ts: 685 pass, 32 skip.test/cli/install/bun-install-git-deps.test.ts: 201 pass, 1 skip.test/cli/install/migration/yarn-lock-migration.test.ts: 69 pass.test/cli/install/bun-add.test.ts: 219 pass.test/js/bun/s3/s3-requester-pays.test.ts: 10 pass.no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check