Remove stale TODO(port) comments and delete dead code#31746
Conversation
An audit of all TODO(port)/TODO(b2-*)/TODO(audit) comments found 804 whose described work has already landed. Remove those comments (and only the comments — trailing comments are trimmed from otherwise-unchanged lines). Comments describing genuinely remaining work are untouched.
|
Updated 4:04 AM PT - Jun 3rd, 2026
✅ @robobun, your commit 655f9cd9329e92a653306963a9d97ebce392cafa passed in 🧪 To try this PR locally: bunx bun-pr 31746That installs a local version of the PR into your bun-31746 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Warning Review limit reached
More reviews will be available in 6 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Expr::as_string_z, implements destructuring-to-binding conversion and JSX closing-tag validation, finalizes CLI option parsing, improves Windows errno mapping, adjusts help formatting, updates user-agent export Sync, refactors FakeTimers assertions, removes Linux splice/RWF/ioctl wrappers, prunes modules, and broadly deletes porting comments. ChangesCore parser/runtime updates and cleanup
Possibly related PRs
Suggested reviewers
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun adopt and delete all the dead code |
|
✅ Adopted and done — pushed
Verified: All review feedback addressed, latest in While validating the ratchet test's regenerate-then-check flow, found an unrelated pre-existing bug: a plain |
|
@robobun push |
Audited every #[allow(dead_code)] in the workspace by stripping the attribute and running cargo check for all 10 CI target triples, in both dev and release profiles: - Deleted items that are dead on every target that compiles them: unused Windows errno consts and get_errno_ntstatus, os_environ_ptr, the x86 cpuid/sysctlbyname TSC-frequency leftovers, platform::linux splice/RWFFlagSupport/ioctl_ficlone, Mutex ExternImpl aliases, INotifyWatcher::unwatch, WindowsWatcher EventListIndex and Timeout::Minimal, WindowsBufferedReader::enable_keeping_process_alive, WindowsHandlers::on_read, the non-macOS on_machport_change stub, unused encoding discriminant consts, FakeTimers AssertMode::Unlocked (enum collapsed into assert_locked), O_DIRECT, shell WatchFd, Path OsUnit, IN4ADDR_LOOPBACK, ssl_renegotiate_mode_t, SSL_CTX_up_ref, and the never-wired jsc config.rs / static_export.rs modules plus subprocess Poll/WaitThreadPoll (marked dead-per-LIFETIMES.tsv). - Dropped ~170 stale attributes whose items are in fact used on every target (the escape had outlived its reason). - Kept the attribute only where it is load-bearing: items used on a subset of platforms (CoreGraphics image backend, libinfo DNS, ConPTY paths, posix pollables compiled-but-unused on Windows), debug_assert-only helpers that are dead in release, test-only items, and macro-template hygiene in derive/FFI macros. Verified: cargo check --workspace clean for all 10 rust targets in dev and release profiles, clippy clean, debug build runs.
robobun
changed the title
The workspace denies dead_code, so every item-level #[allow(dead_code)] is a deliberate escape hatch. The sweep audited all of them against per-target cargo check (dev and release profiles), deleted the code that was dead everywhere, and kept only escapes whose items are live on some target or profile. Pin that inventory per file, following the ban-words.test.ts ratchet pattern: a new escape (or one reappearing in a cleaned file) fails the test with instructions to delete the dead code or justify and re-inventory; deletions must update the snapshot so it stays accurate.
|
@robobun fix the commetns |
The NB comment claimed bun_core::getenv_z is stubbed to None on
cfg(windows); it now walks the WTF-8 env block via getenv_z_any_case,
so the std::env::var("LOCALAPPDATA") workaround is unnecessary. Unify
both platforms onto getenv_z, matching the Zig reference
(ChromeProcess.zig readDevToolsActivePort).
The lifetime decision it tracked has landed: the field is an owned ZBox populated by ensure_cache_directory, with process lifetime via the leaked PackageManager singleton. Keep the invariant as plain prose.
…getenv claim - dead-code-escapes ratchet: scan whole files so rustfmt-wrapped #[cfg_attr(...)] attributes are counted (strip full-line comments first); regenerated inventory is byte-identical - schema.rs: drop deleted jsc/config.rs from the ResolveMode doc - init_command.rs: delete the false 'getenv_z_any_case is a TODO stub on Windows' claim and use bun_core::getenv_z_any_case(USER) instead of the std::env::var workaround, matching the ChromeProcess.rs fix
Editors and git stash round-trips can leave stray .rs files in the working tree (e.g. files this branch deletes being temporarily restored); those must not fail the escape ratchet. Skip anything not in `git ls-tree -r HEAD`, falling back to counting everything when git is unavailable. CI runs against the committed tree, so every real file stays covered.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/test_runner/timers/FakeTimers.rs`:
- Around line 113-122: The assert_locked function currently calls
owner.lock.try_lock(), which if successful acquires the lock and is never
released; change the logic to capture the result of try_lock(), and if it
succeeds explicitly release/drop the acquired guard before performing the
assertion failure path. Concretely, in assert_locked (referencing
owner.lock.try_lock()) store the guard/option/result, if it indicates the lock
was acquired drop/unlock it immediately, then run the debug_assert that the lock
should not have been acquirable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e33b0e1-3fca-4d04-a0a1-8acf7b633025
📒 Files selected for processing (41)
src/bun_alloc/heap_breakdown.rssrc/bun_alloc/lib.rssrc/bun_bin/lib.rssrc/bun_core/lib.rssrc/css/properties/border_image.rssrc/css/properties/box_shadow.rssrc/errno/windows_errno.rssrc/install/PackageManager.rssrc/install/dependency.rssrc/install/isolated_install.rssrc/install/windows-shim/bun_shim_impl.rssrc/io/PipeReader.rssrc/io/PipeWriter.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/js_parser/visit/visit_expr.rssrc/jsc/config.rssrc/jsc/lib.rssrc/jsc/static_export.rssrc/options_types/schema.rssrc/paths/Path.rssrc/paths/string_paths.rssrc/perf/hw_timer.rssrc/platform/lib.rssrc/platform/linux.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/subprocess.rssrc/runtime/cli/init_command.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/jsc_hooks.rssrc/runtime/node/fs_events.rssrc/runtime/node/node_fs_constant.rssrc/runtime/node/win_watcher.rssrc/runtime/shell/util.rssrc/runtime/socket/SocketAddress.rssrc/runtime/socket/tls_socket_functions.rssrc/runtime/test_runner/timers/FakeTimers.rssrc/runtime/webcore/encoding.rssrc/runtime/webview/ChromeProcess.rs
💤 Files with no reviewable changes (22)
- src/css/properties/box_shadow.rs
- src/jsc/config.rs
- src/io/posix_event_loop.rs
- src/runtime/shell/util.rs
- src/paths/string_paths.rs
- src/css/properties/border_image.rs
- src/jsc/static_export.rs
- src/runtime/api/bun/subprocess.rs
- src/io/lib.rs
- src/install/dependency.rs
- src/runtime/node/win_watcher.rs
- src/runtime/node/fs_events.rs
- src/install/isolated_install.rs
- src/runtime/cli/test/parallel/Channel.rs
- src/jsc/lib.rs
- src/install/windows-shim/bun_shim_impl.rs
- src/runtime/node/node_fs_constant.rs
- src/perf/hw_timer.rs
- src/runtime/dns_jsc/dns.rs
- src/io/PipeWriter.rs
- src/io/PipeReader.rs
- src/errno/windows_errno.rs
…a items The whole-file scan's [\s\S]+? predicate could extend past an unrelated cfg_attr's closing bracket and swallow every intervening escape into a single match (undercount), and the tail rejected allow(dead_code) followed by another meta item. Use [^\]] in both spots so a match can never cross a ] and trailing meta items before the attribute close are accepted. Inventory regenerates byte-identical.
2 participants
What
Two commits:
TODO(port)/TODO(b2-*)/TODO(audit)comments whose described work has already landed — out of 3,276 such comments audited acrosssrc/.#[allow(dead_code)]escapes, and drops the escapes that had gone stale.Plus one review-driven follow-up:
ChromeProcess.rsDevToolsActivePort discovery carried an NB comment claimingbun_core::getenv_zis stubbed toNoneon Windows — no longer true (it walks the WTF-8 env block viagetenv_z_any_case). Deleted the stale comment and unified the Windows arm ontogetenv_z(zstr!("LOCALAPPDATA")), matching the Zig reference'sbun.getenvZ("LOCALAPPDATA")and dropping thestd::env::varworkaround.How
Comment sweep (commit 1): every tagged TODO was checked against the current code (and the
.zigreference sibling where relevant). Only comments confirmed stale were removed. 2,472 TODOs describing genuinely remaining work are untouched.Dead-code sweep (commit 2): stripped all 261 item-level
#[allow(dead_code)]attributes and rancargo check --workspacefor every CI target triple (linux gnu/musl ×2 arches, macOS ×2, windows ×2, freebsd, android) in both dev and release profiles, then triaged every diagnostic:os_environ_ptr, x86cpuid/sysctlbynameTSC leftovers,platform::linuxsplice/RWFFlagSupport/ioctl_ficlone,Mutex::ExternImpl,INotifyWatcher::unwatch,WindowsWatcher::{EventListIndex, Timeout::Minimal}, the non-macOSon_machport_changestub, unused encoding discriminants,FakeTimers::AssertMode(collapsed intoassert_locked), the never-wiredjsc/config.rsandjsc/static_export.rsmodules, andsubprocess::Poll/WaitThreadPoll(explicitly marked "dead code per LIFETIMES.tsv").sys/windows/mod.rs).debug_assert!-only helpers, and macro-template hygiene attrs in derive/FFI macros.Test
test/internal/dead-code-escapes.test.tspins the audited per-file inventory of item-level#[allow(dead_code)]escapes (ratchet, same pattern asban-words.test.ts): a new escape — or one reappearing in a cleaned file — fails with instructions to delete the dead code or justify and re-inventory; deletions must update the snapshot. The test fails on the pre-sweep tree (31 files over limit) and passes after it.Verification
cargo check --workspaceclean (zero warnings,dead_code = denyintact) for all 10 target triples, dev and release profilescargo check -p bun_shim_implclean with and withoutshim_standalonecargo clippy --workspace --no-depscleanbun bddebug build links and runs; spot-ran fake-timers, dns, watcher, encoding, and spawn test files