chore: delete 257 lines of dead code, and the 25 allows that were hiding nothing

[doublegate]

A remediation pass over the project's dead-code suppressions. 257 lines removed, nothing added.

The question, and why the method matters

The project rule is that every local #[allow] is justified. This asks the prior question — is it still doing anything? — by stripping every #[allow(dead_code)] and re-running clippy.

A single-combo probe would prove nothing. An item can be live by default and dead on wasm or under --no-default-features, which is precisely the situation that would have earned the attribute in the first place. So every combination this repo gates was run: default, debug-hooks, full, the no_std thumbv7em target, and all four wasm32 variants (default, wasm-canvas, browser-cheevos, script-wasm).

Of 29 attributes, 25 suppressed nothing in any combination. Each was a permanently-disabled dead-code check on an item that is used — the opposite of what it looks like, and a place a future regression could hide unnoticed.

The two islands that were real

Both are leftovers from the mc-r1-* staged master-clock migration, whose cargo features exist nowhere in the workspace.

The APU pair (dmc_abort_delay_for + schedule_explicit_dmc_abort_if_needed) — its own comment said it was "retained for the flag-off path", and that flag was removed in #237 (v2.0.1). 34 lines.

The four LockstepBus DMA-service methods — a closed island: each called the next and nothing outside called any. That is why grepping a single name found a caller and looked reassuring, and why removing one attribute made all four report at once. 183 lines.

What deleting the island exposed

drain_dma — called on every CPU read, every CPU write, and every bus cycle — had already been reduced to an empty body: let _ = read_addr; and forty lines of comments describing what it used to do.

Its #[allow] list was the tell: unused_self, needless_pass_by_ref_mut and missing_const_for_fn fire together exactly when a method does nothing. Its comments still claimed "the legacy service below stays active for the default build" — untrue since v2.0.0 — and at the call site it read as though DMA were being serviced there. Deleted with its three call sites.

Verification

rustynes-apu and rustynes-core both change, so the accuracy battery is VERIFIED, not asserted — and re-run after the island deletion, not only after the first commit:

AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests

That is the authoritative decoder, quoted case-sensitively; the framebuffer decoder reports 120 and is known-buggy. nestest passes. Both with a confirmed non-zero test count, since a filter matching nothing exits 0.

Also clean: no_std cross-compile, workspace clippy, debug-hooks, full, all four wasm32 combos, rustdoc, and every crate suite.

Behaviour is unchanged by construction: the compiler proves the island was unreachable — that is what made it dead — and an empty function called three times is three no-ops.

Left alone deliberately

Fifteen comments across rustynes-core, rustynes-cpu and rustynes-apu still name the removed functions while describing the historical mechanism. Rewriting fifteen technical comments to avoid a name risks changing what they say about the hardware, which is a worse trade than a reader grepping and finding the explanation here.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4f5eac09-eba0-441a-bd10-42844741e31c

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR removes stale #[allow(dead_code)] suppressions and deletes now-unreachable code paths, tightening the project’s “every allow must be justified” rule and reducing the surface area where regressions can hide behind blanket lints.

Changes:

• Removed 25 #[allow(dead_code)] attributes that were no longer suppressing anything across core/frontend/PPU code.
• Deleted an unreachable “LockstepBus DMA service” island in rustynes-core (including an already-empty drain_dma and its call sites).
• Deleted an unreachable DMC-abort scheduling island in rustynes-apu.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/rustynes-ppu/src/ppu.rs	Removes an unnecessary dead_code allow on a live PPU struct field.
crates/rustynes-frontend/src/save_states_ui.rs	Removes an unnecessary dead_code allow from a public save-state slot path helper.
crates/rustynes-frontend/src/save_state.rs	Removes stale dead_code allows from save-state helper APIs.
crates/rustynes-frontend/src/input.rs	Removes an unnecessary dead_code allow from a test-used defaults constructor.
crates/rustynes-frontend/src/gfx.rs	Removes unnecessary dead_code allows from post-processing disable helpers.
crates/rustynes-frontend/src/debugger/mod.rs	Removes unnecessary dead_code allows from retained overlay state fields.
crates/rustynes-frontend/src/config.rs	Removes unnecessary dead_code allows from config save APIs.
crates/rustynes-frontend/src/audio.rs	Removes unnecessary dead_code allows from audio queue/output inspection helpers.
crates/rustynes-core/src/bus.rs	Deletes unreachable DMA-service code and removes drain_dma call sites that were no-ops.
crates/rustynes-apu/src/apu.rs	Deletes unreachable DMC-abort scheduling helpers and removes a stale dead_code allow.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[doublegate]

The blocking finding is declined as not reproducible, and the quote it rests on is the false comment this PR removes.

drain_dma is not dead code; it is actively called in cpu_read and cpu_write. Its internal comments note it "stays active for the default build". Deleting it and its invocations will break DMA timing and synchronization.

drain_dma was called — and its entire body was this:

fn drain_dma(&mut self, read_addr: Option<u16>) {
    let _ = read_addr;
}

Nothing else. Forty lines of comments describing what it used to do, and one statement discarding its argument. Deleting an empty function and its three call sites is three no-ops removed; there is no timing to break because there was no work being done.

The sentence you quote is the evidence, not the counter-evidence. "The legacy service below stays active for the default build" refers to the mc-r1-* staged migration's bus-side DMA service. Those cargo features exist nowhere in the workspace and the master-clock scheduler has been the only path since v2.0.0, so the claim has been false for five releases. A comment asserting a behaviour the code does not have is exactly what this pass is removing — reading it as authority inverts the finding.

The #[allow] list on that function was the tell: unused_self, needless_pass_by_ref_mut and missing_const_for_fn fire together precisely when a method does nothing.

The suggestion is also measured, not assumed

Removing #[allow(dead_code)] from private struct fields … will likely trigger compiler warnings. If the project enforces #![deny(warnings)] in CI, this will break the build.

That was the first thing checked, and it is why only 25 of 29 attributes were removed rather than all of them. Every gated combination was run with -D warnings: workspace, debug-hooks, full, no_std thumbv7em, and all four wasm32 variants. Zero errors. The four attributes that were still doing work are the two islands, and they are the reason the sweep took the shape it did.

And the accuracy battery, after the deletion

AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests

Re-run after the island and drain_dma were removed, not only after the first commit — plus nestest, with a confirmed non-zero test count.

Both Copilot findings on this PR were real and are fixed in 9374bf60; they were comments that outlived what they described, which is the same class as the one quoted above.

[doublegate]

The blocking finding is not reproducible — try_default was never touched.

$ grep -n "try_default" crates/rustynes-frontend/src/audio.rs
616:    pub fn try_default() -> Result<Self, AudioError> {

$ git diff origin/main...HEAD -- crates/rustynes-frontend/src/audio.rs | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)'
-    #[allow(dead_code)] // read in tests + as a UI mirror.
-    #[allow(dead_code)] // Used by tests + the Performance panel.
-    #[allow(dead_code)] // Used by tests + the Performance panel.
-    #[allow(dead_code)] // wasm builds render the panel without native audio.
-    #[allow(dead_code)] // wasm builds render the panel without native audio.
-    #[allow(dead_code)]
-    #[allow(dead_code)]
-    #[allow(dead_code)]

Eight attribute lines removed from that file and nothing else — no function deleted, in audio.rs or anywhere. Removing those lines shifted everything below them upward, which is the likeliest way pub fn try_default() came to sit next to a - line in a diff view.

Worth noting the finding's own internal check would have caught it: it says the deletion "directly contradicts the rationale you used to keep pub fn slot_path_for". That rationale is applied consistently here — pub items in a lib target are exempt from dead-code analysis, which is exactly why neither try_default nor slot_path_for needed an attribute and why neither was removed. The code this PR deletes is private and provably unreachable; the compiler said so, and that is what made it safe to cut.

The suggestion is taken (9582fcf9)

You are right that my rewrite of those two notes kept more history than a docstring needs. What a reader must know is the mechanism — pub items are exempt from dead-code analysis, so neither item needs an attribute — not how many attributes this pass removed or which review prompted the wording. Both notes now carry the former and drop the latter.

[github-actions]

Antigravity review (Gemini via Ultra)

This PR removes 257 lines of unused legacy DMA/scheduling logic and drops 25 #[allow(dead_code)] attributes across the codebase.

Blocking issues

None found.

Suggestions

• crates/rustynes-frontend/src/debugger/mod.rs (lines 837, 844): You removed #[allow(dead_code)] from the private fps and movie fields. As your new comment on slot_exists correctly points out, pub items in a library are exempt from dead-code analysis, but private and pub(crate) fields are not. If these fields are set but never read, removing the attribute will introduce dead_code compiler warnings.
• crates/rustynes-core/src/bus.rs (line 329), crates/rustynes-ppu/src/ppu.rs (line 594), and crates/rustynes-apu/src/apu.rs (line 179): The same compiler warning risk applies here. Removing #[allow(dead_code)] from the pub(crate) fields (cart, secondary_oam, subpos_arm_countdown) will trigger lints if they lack in-crate readers. Consider restoring the allows for these internal fields if they are genuinely unread.

Nitpicks

• The PR title states the allows "were hiding nothing," but they were likely actively suppressing warnings for the internal and private fields.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}