Remove dead Windows error tables - #31995
Conversation
|
Updated 4:58 AM PT - Jul 13th, 2026
❌ @robobun, your commit db908ad has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31995That installs a local version of the PR into your bun-31995 --bun |
|
@robobun adopt |
b500c6b to
4e22c28
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request refactors Windows errno enum generation to use a unified X-macro scheme combining Linux and UV error lists, removes the ChangesWindows errno enum generation refactoring
Windows error API surface updates
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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/windows_sys/externs.rs`:
- Around line 952-954: Update the doc comment in externs.rs so it correctly
describes the tiering: state that the subset referenced by lower-tier crates is
the named subset, while errno/bun_errno mapping is a higher-tier concern (i.e.,
T0 must not depend on T1), not the other way around; specifically, replace the
sentence that says "`errno` is a lower-tier crate" with wording that identifies
`bun_errno`/`errno` as the higher-tier mapping and clarifies that new MS-ERREF
consts may be added without ABI change, matching the T0/T1 contract referenced
elsewhere.
🪄 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: 4fd69d21-3e89-4ca9-a634-5e407a3ff86e
📒 Files selected for processing (3)
src/errno/windows_errno.rssrc/sys/windows/mod.rssrc/windows_sys/externs.rs
…nts (#32519) ## What Cleans up comments left behind from the incremental Zig→Rust port that claim code is "cfg-gated", "blocked_on X", or "will un-gate when Y lands", where X/Y have since landed and the claim is no longer true. Also collapses a handful of local shims whose only justification was one of those stale comments. Before: 158 such markers across 87 files. After: 8 remaining, all of which accurately describe real platform/feature `#[cfg(...)]` gating (macOS/Windows image clipboard, POSIX/Windows stdio alias, WASM timer selection, Win32 x64 callconv extern blocks, debug-assertions context, the `#[cfg(any())]` Win32Error table handled by #31995). ## Shims collapsed into canonical impls - **`src/runtime/cli/repl.rs`**: removed `global_clear_exception`, `global_to_js_value`, `vm_set_execution_forbidden`, `vm_mut` (48 lines). Comment claimed "the canonical impls live in cfg-gated JSGlobalObject.rs / VM.rs (see src/jsc/lib.rs \`_gated\`)"; no `_gated` module exists and `JSGlobalObject::clear_exception` / `::to_js_value`, `VM::set_execution_forbidden`, `VirtualMachine::as_mut` are all public and un-gated. 25 call sites rewired. - **`src/standalone_graph/StandaloneModuleGraph.rs`**: replaced hand-inlined open+mkdir+retry with `bun_sys::File::make_open`. Comment claimed "`src/sys/File.rs` is still cfg-gated upstream"; `make_open` is public at `src/sys/file.rs:112` and already used from `build_command.rs`. - **`src/js_parser/visit/mod.rs`**: removed local `stmts_to_single_stmt_` (duplicate of `P::stmts_to_single_stmt`). Comment claimed it was "``-gated (P.rs:6267, blocked on S::Block Default)". - **`src/js_parser/lower/lower_esm_exports_hmr.rs`**: removed local `generate_temp_ref` (duplicate of `P::generate_temp_ref`). Comment claimed "`P::generate_temp_ref` is ``-gated in P.rs (round-6 re-gate)". - **`src/js_parser/visit/mod.rs`**: removed dead `let _ = &mut j;` keep-alive whose comment said "keep 'outer label live until #[cfg] un-gates". - **`src/css/selectors/selector.rs`**: removed two dead `let _ = arguments;` lines after real `arguments.to_css_raw(dest)?` calls. ## Comments rewritten or deleted The rest are comment-only changes removing port-progress narrative ("cycle-5", "round-D/E/G/H", "phase-c/d", "tier-0", "reconciler-6 re-gate", "`` gates carry blocked_on notes") and stale blocker lists that name dependencies which now exist. Where a comment carried useful non-port information (aliasing rationale, init-order invariants, layering notes), that part is kept; only the stale gating claim is removed. A few `blocked_on` comments in `src/css/` described genuinely incomplete code (e.g. `Property::longhand`'s no-op `lh()` stub, the CSS-modules ref-arm in `selector_has_composes_for_property`); those were reworded to plain TODOs rather than deleted. ## Verification ``` cargo check --workspace # clean bun run rust:check-all # 10/10 targets ok bun bd test test/js/bun/repl/repl.test.ts # 117 pass bun bd test test/js/bun/util/csrf.test.ts # 24 pass bun bd test test/bundler/esbuild/css.test.ts # 53 pass bun bd test test/bundler/css/css-modules.test.ts # 4 pass bun bd test test/bake/dev/bundle.test.ts # 20 pass ``` 80 files changed, +171/-796. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…tables # Conflicts: # src/sys/windows/mod.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the dead-code deletions are clearly safe and the -Zunpretty=expanded equivalence proof for the X-macro refactor is convincing, but since this restructures the load-bearing E/SystemErrno discriminant generation it's worth a quick human glance before merge.
Extended reasoning...
Overview
This PR touches three Windows-only Rust files:
src/sys/windows/mod.rs— deletes a#[cfg(any())]-parked 1,188-variant MS-ERREF reference table (never compiled into any target) and the uncalledwin_sock_error_to_zig_errorconverter.src/windows_sys/externs.rs— removes theWinsockErrornewtype and its ~85WSA*consts (sole consumer was the deleted converter;Win32Erroralready carries the WSA consts that are actually used), and rewords a tiering comment per resolved CodeRabbit feedback.src/errno/windows_errno.rs— replaces two hand-maintained 138-variant enum bodies (EandSystemErrno) with a singlefor_each_linux_errno!X-macro list, chained through the existingfor_each_uv_errno!tail and a new__errno_enum!emitter with@bare/@prefixedarms.
I verified that no remaining Rust code references WinsockError or win_sock_error_to_zig_error, and traced the macro expansion: the 138 [BARE PREFIXED = N] rows match the original variant names/values exactly, the #[strum(serialize = "2BIG")] attribute is correctly applied only to the @bare arm (matching the original where only E::_2BIG carried it), and the derive list / #[repr(u16)] are unchanged.
Security risks
None. This is dead-code removal plus a macro-driven dedup of two enum definitions; no auth, crypto, input parsing, or trust-boundary code is touched.
Level of scrutiny
Moderate. The pure deletions (#[cfg(any())]-gated table, unreferenced WinsockError newtype) are trivially safe. The X-macro refactor is well-verified — the author diffed -Zunpretty=expanded output and confirmed identical variant names, values, and order — but it restructures the single source of truth for Windows errno discriminants, where order is explicitly load-bearing (enum_map::Enum ordinals + SystemErrno::to_e transmute). A subtle drift here would mis-map every Windows syscall error. The 3-level macro chain (for_each_linux_errno! → __errno_enum_add_uv_tail! → for_each_uv_errno! → __errno_enum!) is also non-trivial enough that a maintainer should confirm they're comfortable with the pattern going forward.
Other factors
- Bug-hunting system found nothing.
- CodeRabbit's only finding (doc-comment tiering wording) was addressed in f6f3c1b and the thread is resolved.
- Windows CI lanes passed; the red lanes are documented Linux-only flakes unrelated to this Windows-gated diff.
- No CODEOWNERS cover these paths.
- This is a split-out from a larger closed PR (#31912), so it's already been scoped down for reviewability.
…tables # Conflicts: # src/sys/windows/mod.rs
There was a problem hiding this comment.
I didn't find any issues. The dead-code deletions (#[cfg(any())] MS-ERREF table, win_sock_error_to_zig_error, WinsockError) are trivially safe — I confirmed zero remaining references. The for_each_linux_errno! refactor looks correct and follows the existing for_each_uv_errno! pattern, but since it replaces the hand-listed bodies of two enums whose discriminant lockstep backs an unsafe transmute (SystemErrno::to_e), it's worth a maintainer sign-off on the macro approach.
Checked: all 138 [bare prefixed = N] rows match both original enum bodies (names, values, order); the #[strum(serialize = "2BIG")] attr correctly applies only to E::_2BIG via the @bare arm; the @emit arm's derives/#[repr(u16)] match the originals; the ENOTSUP doc-comment loss is cosmetic only.
Extended reasoning...
Overview
Three files touched, net −3.2k lines:
src/sys/windows/mod.rs: deletes a#[cfg(any())]-gated 1188-const MS-ERREF reference table (never compiled) andwin_sock_error_to_zig_error(no callers — verified via grep on current HEAD). Updates one doc comment.src/windows_sys/externs.rs: deletes theWinsockErrornewtype + consts (only consumer was the deleted converter — verified). Rewords one doc comment per resolved CodeRabbit feedback.src/errno/windows_errno.rs: replaces two hand-maintained 138-variant enum bodies (EandSystemErrno) with a sharedfor_each_linux_errno!X-macro plus a__errno_enum!emitter that has@bare/@prefixedarms selecting the first vs. second ident from each row. Chains through the pre-existingfor_each_uv_errno!for the tail.
Security risks
None. Windows-only error-code enum definitions and dead-code removal; no input parsing, auth, crypto, or permission surfaces.
Level of scrutiny
The deletions are mechanical and trivially verifiable (I grepped: no references to win_sock_error_to_zig_error or WinsockError remain in src/; the #[cfg(any())] block was never compiled). Those alone would be an easy approve.
The windows_errno.rs refactor is where the substance is. SystemErrno::to_e does transmute::<u16, E>(self as u16), so the two enums' discriminants and enum_map::Enum ordinals must stay identical — that invariant is now enforced by macro-generating both from one list rather than by hand-maintaining parallel listings. The PR author and robobun both verified via -Zunpretty=expanded on x86_64-pc-windows-msvc that the expanded enum bodies are byte-identical to before (224 variants each, same names/values/order), and I spot-checked the 138 macro rows against both deleted enum bodies: all match. The pattern is not new — for_each_uv_errno! already drives the tail of both enums the same way.
That said, this introduces ~100 lines of new declarative-macro machinery (for_each_linux_errno!, __errno_enum_add_uv_tail!, __errno_enum! with three arms) governing a safety-critical invariant. It's a design choice about how these enums are maintained going forward, and per the approval guidelines that's the kind of thing a maintainer should explicitly sign off on rather than a bot.
Other factors
- No CODEOWNERS coverage for these paths.
- CodeRabbit's one inline comment (tiering wording in a doc comment) was addressed in f6f3c1b and marked resolved.
- robobun adopted, re-merged main (resolving a conflict from #33909 rewriting the deleted function's signature), and re-verified expansion equivalence + Windows
cargo checkafter the merge. - No prior review from me on this PR.
- The bug-hunting system found nothing.
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
|
Closing: the dead code this PR removes has since landed on main independently. The Two small pieces of this PR are not on main: the |
2 participants
What this does
Removes dead Windows error-handling code and dedups the errno enum listings. Zero intended behavior change.
src/sys/windows/mod.rs: deletes the#[cfg(any())]-parked 1,188-variant MS-ERREF reference table (never compiled) andwin_sock_error_to_zig_error, which had no callers.src/windows_sys/externs.rs: deletes theWinsockErrornewtype, whose only consumer was that converter.bun_windows_sys::Win32Errorremains the single compiled WSA table (already the onebun_errnouses).src/errno/windows_errno.rs:enum Eandenum SystemErrnowere two hand-maintained 138-variant listings that must stay in lockstep (ordinals driveSystemErrno::to_e). Both are now emitted from one sharedfor_each_linux_errno!X-macro list, the same pattern the existingfor_each_uv_errno!tail already uses.Net -3.2k lines. Split from #31912 (closed in favor of module-scoped splits).
Verification
win_sock_error_to_zig_errorandWinsockErrorhave no references outside their own definition sites on main.bun_errno(-Zunpretty=expanded,x86_64-pc-windows-msvc) before and after: theenum Eandenum SystemErrnobodies are identical in variant names, values, and order. The only textual difference is a doc comment onENOTSUPthat became a regular comment in the macro list.cargo check --workspacepasses forx86_64-pc-windows-msvcandaarch64-pc-windows-msvc, the only targets that compilewindows_errno.rsandsys/windows/mod.rs(both#[cfg(windows)]-gated; theexterns.rshunk removes declarations only).rust-check-allalso passed all 10 targets on the original commit.No runtime test accompanies this PR because there is no behavior to pin down: the deleted items had zero references, and the enum dedup expands to identical code, so no test can distinguish before from after on any platform. The expansion diff above is the equivalence proof.