Dead and duplicated code: a stale iterate_to_convergence, a copy-pasted sweep, and #[allow(dead_code)] used as a mute button #23

New Issue

2026-08-04T19:20:44Z

logaritmisk commented

2026-08-04 19:20:44 +00:00

Cleanup pass. Nothing here is a live bug, but the accumulated #[allow(dead_code)] is actively hiding things — the Skill.online defect in #19 is exactly the kind of thing it conceals.

1. `TimeSlice::iterate_to_convergence` is dead and ignores its own config

src/time_slice.rs:457-483, marked #[allow(dead_code)], with hardcoded constants:

let epsilon = 1e-6;
let iterations = 20;

Neither matches ConvergenceOptions (epsilon 1e-6 coincidentally, but max_iter is 30 — and EpsilonOrMax::max is 10; see #22). TimeSlice gained a convergence field in 872f917 and callsites were moved to read it in 824b7f5, but this function was left behind with its literals. It is called only from a test (src/time_slice.rs:707).

Either delete it, or make it read self.convergence and use it.

2. The sequential sweep is duplicated verbatim

TimeSlice::iteration's from > 0 branch (src/time_slice.rs:323-357) and Event::iteration_direct (src/time_slice.rs:131-165) are the same logic written twice: build within_priors, dispatch on EventKind to ranked_with_arena/scored_with_arena, then walk teams/items updating skills[…].likelihood and item.likelihood, then store event.evidence.

The 30-line update loop is character-for-character identical. Any fix to one — for instance the log-space evidence change in #12 — has to be made twice, and a divergence between them would be silent.

iteration should call iteration_direct per event.

3. `#[allow(dead_code)]` is applied broadly

src/color_group.rs — on ColorGroups::new, n_colors, is_empty, total_events, color_range, and color_greedy itself, all of which are used (color_range is load-bearing for the unsafe parallel path in #13). The attribute appears to have been added defensively and never removed, so it now suppresses warnings for the members that genuinely aren't used.
src/storage/skill_store.rs — on contains, len, is_empty.
src/game.rs:87 — #[allow(dead_code)] on the whole OwnedGame struct, masking that teams is the only field ever read: result, weights, p_draw, and convergence are stored and never touched. Either expose them (they'd be useful — see the Rating accessor gap in #21) or stop storing them.

Removing these and letting the compiler speak would have surfaced #19 immediately.

4. Smaller items

Competitor::receive uses float equality as a presence check. if self.message != N_INF (src/competitor.rs:24, and again at :38) treats "message equals the improper Gaussian" as "no message yet". It works because N_INF is {pi: 0, tau: 0} and a real message is unlikely to land exactly there — but it is a sentinel comparison on floats standing in for Option<Gaussian>. Clippy's pedantic lint flags it (strict comparison of f32 or f64, 29 occurrences crate-wide).
Vec::is_empty() as an optional. add_events_with_prior treats empty results/weights as "not supplied" (src/history.rs:472, 486, 572-582) and TimeSlice::add_events does the same (src/time_slice.rs:279, 288). Option<Vec<_>> says it directly and distinguishes "absent" from "supplied but empty".
OwnedGame::new clones the whole team structure. Game::ranked_with_arena(teams.clone(), …) (src/game.rs:107) deep-clones Vec<Vec<Rating>> on every public Game construction just to keep an owned copy alongside. Restructuring so the owned data is moved once would drop the clone.
add_events_with_prior deep-clones its inputs per slice group — composition[o[e]].clone(), results[o[e]].clone(), weights[o[e]].clone() (src/history.rs:568-582) rebuild nested Vecs that were just constructed by add_events. Draining or indexing in place would avoid a full second copy of the ingestion payload.
Matrix::adjugate and Matrix::minor are dead — see #9, which proposes replacing that code path entirely.
compute_elapsed clamps negatives silently (src/time_slice.rs:620). Time::elapsed_to is documented as non-negative for self <= later, so a negative here means a Time impl broke its contract — worth a debug_assert! rather than a silent .max(0).

Acceptance

cargo clippy --all-targets --all-features clean with no #[allow(dead_code)] outside genuinely-reserved API.
No duplicated sweep logic.
Numerical goldens unchanged.

Cleanup pass. Nothing here is a live bug, but the accumulated `#[allow(dead_code)]` is actively hiding things — the `Skill.online` defect in #19 is exactly the kind of thing it conceals. ## 1. `TimeSlice::iterate_to_convergence` is dead and ignores its own config `src/time_slice.rs:457-483`, marked `#[allow(dead_code)]`, with hardcoded constants: ```rust let epsilon = 1e-6; let iterations = 20; ``` Neither matches `ConvergenceOptions` (epsilon `1e-6` coincidentally, but `max_iter` is 30 — and `EpsilonOrMax::max` is 10; see #22). `TimeSlice` gained a `convergence` field in 872f917 and callsites were moved to read it in 824b7f5, but this function was left behind with its literals. It is called only from a test (`src/time_slice.rs:707`). Either delete it, or make it read `self.convergence` and use it. ## 2. The sequential sweep is duplicated verbatim `TimeSlice::iteration`'s `from > 0` branch (`src/time_slice.rs:323-357`) and `Event::iteration_direct` (`src/time_slice.rs:131-165`) are the same logic written twice: build `within_priors`, dispatch on `EventKind` to `ranked_with_arena`/`scored_with_arena`, then walk teams/items updating `skills[…].likelihood` and `item.likelihood`, then store `event.evidence`. The 30-line update loop is character-for-character identical. Any fix to one — for instance the log-space evidence change in #12 — has to be made twice, and a divergence between them would be silent. `iteration` should call `iteration_direct` per event. ## 3. `#[allow(dead_code)]` is applied broadly - `src/color_group.rs` — on `ColorGroups::new`, `n_colors`, `is_empty`, `total_events`, `color_range`, **and `color_greedy` itself**, all of which are used (`color_range` is load-bearing for the unsafe parallel path in #13). The attribute appears to have been added defensively and never removed, so it now suppresses warnings for the members that genuinely aren't used. - `src/storage/skill_store.rs` — on `contains`, `len`, `is_empty`. - `src/game.rs:87` — `#[allow(dead_code)]` on the whole `OwnedGame` struct, masking that `teams` is the only field ever read: `result`, `weights`, `p_draw`, and `convergence` are stored and never touched. Either expose them (they'd be useful — see the `Rating` accessor gap in #21) or stop storing them. Removing these and letting the compiler speak would have surfaced #19 immediately. ## 4. Smaller items - **`Competitor::receive` uses float equality as a presence check.** `if self.message != N_INF` (`src/competitor.rs:24`, and again at `:38`) treats "message equals the improper Gaussian" as "no message yet". It works because `N_INF` is `{pi: 0, tau: 0}` and a real message is unlikely to land exactly there — but it is a sentinel comparison on floats standing in for `Option<Gaussian>`. Clippy's pedantic lint flags it (`strict comparison of f32 or f64`, 29 occurrences crate-wide). - **`Vec::is_empty()` as an optional.** `add_events_with_prior` treats empty `results`/`weights` as "not supplied" (`src/history.rs:472`, `486`, `572-582`) and `TimeSlice::add_events` does the same (`src/time_slice.rs:279`, `288`). `Option<Vec<_>>` says it directly and distinguishes "absent" from "supplied but empty". - **`OwnedGame::new` clones the whole team structure.** `Game::ranked_with_arena(teams.clone(), …)` (`src/game.rs:107`) deep-clones `Vec<Vec<Rating>>` on every public `Game` construction just to keep an owned copy alongside. Restructuring so the owned data is moved once would drop the clone. - **`add_events_with_prior` deep-clones its inputs per slice group** — `composition[o[e]].clone()`, `results[o[e]].clone()`, `weights[o[e]].clone()` (`src/history.rs:568-582`) rebuild nested `Vec`s that were just constructed by `add_events`. Draining or indexing in place would avoid a full second copy of the ingestion payload. - **`Matrix::adjugate` and `Matrix::minor` are dead** — see #9, which proposes replacing that code path entirely. - **`compute_elapsed` clamps negatives silently** (`src/time_slice.rs:620`). `Time::elapsed_to` is documented as non-negative for `self <= later`, so a negative here means a `Time` impl broke its contract — worth a `debug_assert!` rather than a silent `.max(0)`. ## Acceptance - `cargo clippy --all-targets --all-features` clean with no `#[allow(dead_code)]` outside genuinely-reserved API. - No duplicated sweep logic. - Numerical goldens unchanged.

logaritmisk referenced this issue

2026-08-04 20:11:28 +00:00

rayon path creates multiple aliasing `&mut SkillStore` through a usize-laundered pointer (UB) #13

logaritmisk commented

2026-08-04 20:13:12 +00:00

Mostly done — staying open for the remainder.

Done:

iterate_to_convergence reads self.convergence instead of its hard-coded epsilon and 20-iteration cap, and is #[cfg(test)] — tests were its only caller.
The duplicated sweep is gone. TimeSlice::iteration's from > 0 branch was a verbatim copy of iteration_direct; both now go through the compute/apply split introduced for #13.
Every #[allow(dead_code)] is removed. What they were hiding, and what happened to it:
- four OwnedGame fields stored and never read → removed
- ColorGroups::{n_colors, total_events}, SkillStore::{contains, len, is_empty} → test-only, now #[cfg(test)] (is_empty was genuinely unused and is gone)
- color_greedy and color_range were being masked despite being load-bearing — the attribute had been applied module-wide and never revisited
Matrix::adjugate / minor removed with the LU rewrite (#9).

cargo clippy --all-targets --all-features -- -D warnings is clean with no allow(dead_code) anywhere in the crate.

Still open (all in section 4, none behaviour-affecting):

Competitor::receive still uses self.message != N_INF as an unset sentinel rather than Option<Gaussian>
results/weights emptiness is still used as "not supplied" instead of Option<Vec<_>>
OwnedGame::new still clones the whole team structure
add_events_with_prior still deep-clones its inputs per slice group
compute_elapsed still clamps a negative elapsed silently

These are all mechanical and independent; none of them is load-bearing for anything else on the list.

Mostly done — **staying open** for the remainder. Done: 1. `iterate_to_convergence` reads `self.convergence` instead of its hard-coded epsilon and 20-iteration cap, and is `#[cfg(test)]` — tests were its only caller. 2. The duplicated sweep is gone. `TimeSlice::iteration`'s `from > 0` branch was a verbatim copy of `iteration_direct`; both now go through the `compute`/`apply` split introduced for #13. 3. Every `#[allow(dead_code)]` is removed. What they were hiding, and what happened to it: - four `OwnedGame` fields stored and never read → **removed** - `ColorGroups::{n_colors, total_events}`, `SkillStore::{contains, len, is_empty}` → test-only, now `#[cfg(test)]` (`is_empty` was genuinely unused and is gone) - `color_greedy` and `color_range` were being masked despite being load-bearing — the attribute had been applied module-wide and never revisited 4. `Matrix::adjugate` / `minor` removed with the LU rewrite (#9). `cargo clippy --all-targets --all-features -- -D warnings` is clean with no `allow(dead_code)` anywhere in the crate. **Still open** (all in section 4, none behaviour-affecting): - `Competitor::receive` still uses `self.message != N_INF` as an unset sentinel rather than `Option<Gaussian>` - `results`/`weights` emptiness is still used as "not supplied" instead of `Option<Vec<_>>` - `OwnedGame::new` still clones the whole team structure - `add_events_with_prior` still deep-clones its inputs per slice group - `compute_elapsed` still clamps a negative elapsed silently These are all mechanical and independent; none of them is load-bearing for anything else on the list.

logaritmisk commented

2026-08-27 15:22:20 +00:00

One more item off this list in eeb43e3: color_greedy carried #[allow(dead_code)] while being called by recompute_color_groups — a mute button on a live function, which is the specific complaint here. Removed; just lint (clippy, -D warnings) is clean without it.

grep -rn "allow(dead_code)" src/ now returns nothing.

Leaving this open — the remaining items from your own follow-up list are untouched:

Competitor::receive float-sentinel comparison against N_INF
results / weights as Option<Vec<_>> rather than empty-Vec-as-sentinel
OwnedGame::new clone
add_events_with_prior deep-clones
compute_elapsed silent clamp

All five are mechanical and independent, as you noted.

One more item off this list in `eeb43e3`: `color_greedy` carried `#[allow(dead_code)]` while being called by `recompute_color_groups` — a mute button on a live function, which is the specific complaint here. Removed; `just lint` (clippy, `-D warnings`) is clean without it. `grep -rn "allow(dead_code)" src/` now returns nothing. Leaving this open — the remaining items from your own follow-up list are untouched: - `Competitor::receive` float-sentinel comparison against `N_INF` - `results` / `weights` as `Option<Vec<_>>` rather than empty-`Vec`-as-sentinel - `OwnedGame::new` clone - `add_events_with_prior` deep-clones - `compute_elapsed` silent clamp All five are mechanical and independent, as you noted.

logaritmisk referenced this issue from a commit

2026-08-27 15:37:44 +00:00

fix: close out four small issues and pin #27's repro

logaritmisk commented

2026-08-27 15:50:55 +00:00

The remaining five are done — 06ed24b, aff3fb9, 4e04336. Closing.

Item	Commit	Note
`Competitor::receive` float sentinel	`06ed24b`	`message` is now `Option<Gaussian>`
`results`/`weights` emptiness sentinel	`aff3fb9`	both now `Option`
`OwnedGame::new` clone	`4e04336`	clone eliminated, not reduced
`add_events_with_prior` deep-clones	`4e04336`	moved with `mem::take`
`compute_elapsed` silent clamp	`06ed24b`	release clamps, debug trips

Three things the changes surfaced that a sentinel would have kept hidden:

Switching message to Option turned every read site into a compile error — there were eight. The interesting one is new_backward_info: skill.backward = agents[agent].message needed unwrap_or(N_INF) rather than an unwrap, because an absent message genuinely does mean the improper identity there. A sentinel-based refactor would have had to find that by reading.

The tie pre-check silently conflated two states. It iterated results directly, so an empty vec skipped the loop and looked identical to "checked, found no ties". Under Option it reads .iter().flatten(), which makes "nothing to check" explicit.

OwnedGame::new's clone was never necessary. Game takes the teams by value and is dropped at the end of the constructor, so the vec can be taken back out of it. The clone existed because nobody looked at the lifetime, not because both needed ownership.

On the mem::take change: its soundness rests entirely on o being a permutation. Visiting an index twice would take an already-emptied vec and produce an event with no teams — silently, not as a failure. There is now a debug_assert checking the permutation property directly, beside the comment explaining why the code depends on it.

One near-miss worth recording. My first draft of the Option validations used let-chains. Those need Rust 1.88; this crate pins 1.85. It compiled locally on 1.98 and would have failed only in the MSRV CI job. Rewritten with is_some_and, and verified by installing 1.85.0 and building against it rather than by assuming the removal was complete.

Two of these are breaking — Competitor.message and TimeSlice::add_events are both public — so they ride the next minor bump rather than 0.2.x.

The remaining five are done — `06ed24b`, `aff3fb9`, `4e04336`. Closing. | Item | Commit | Note | |---|---|---| | `Competitor::receive` float sentinel | `06ed24b` | `message` is now `Option<Gaussian>` | | `results`/`weights` emptiness sentinel | `aff3fb9` | both now `Option` | | `OwnedGame::new` clone | `4e04336` | clone eliminated, not reduced | | `add_events_with_prior` deep-clones | `4e04336` | moved with `mem::take` | | `compute_elapsed` silent clamp | `06ed24b` | release clamps, debug trips | Three things the changes surfaced that a sentinel would have kept hidden: **Switching `message` to `Option` turned every read site into a compile error — there were eight.** The interesting one is `new_backward_info`: `skill.backward = agents[agent].message` needed `unwrap_or(N_INF)` rather than an unwrap, because an absent message genuinely *does* mean the improper identity there. A sentinel-based refactor would have had to find that by reading. **The tie pre-check silently conflated two states.** It iterated `results` directly, so an empty vec skipped the loop and looked identical to "checked, found no ties". Under `Option` it reads `.iter().flatten()`, which makes "nothing to check" explicit. **`OwnedGame::new`'s clone was never necessary.** `Game` takes the teams by value and is dropped at the end of the constructor, so the vec can be taken back out of it. The clone existed because nobody looked at the lifetime, not because both needed ownership. On the `mem::take` change: its soundness rests entirely on `o` being a permutation. Visiting an index twice would take an already-emptied vec and produce an event with no teams — silently, not as a failure. There is now a `debug_assert` checking the permutation property directly, beside the comment explaining why the code depends on it. **One near-miss worth recording.** My first draft of the `Option` validations used let-chains. Those need Rust 1.88; this crate pins 1.85. It compiled locally on 1.98 and would have failed only in the MSRV CI job. Rewritten with `is_some_and`, and verified by installing 1.85.0 and building against it rather than by assuming the removal was complete. Two of these are breaking — `Competitor.message` and `TimeSlice::add_events` are both public — so they ride the next minor bump rather than 0.2.x.

logaritmisk closed this issue

2026-08-27 15:50:58 +00:00

logaritmisk referenced this issue from a commit

2026-08-27 16:41:51 +00:00

refactor!: make Competitor::message an Option, and compute_elapsed loud

logaritmisk referenced this issue from a commit

2026-08-27 16:41:51 +00:00

refactor!: replace emptiness-as-sentinel with Option for results and weights

logaritmisk referenced this issue from a commit

2026-08-27 16:41:51 +00:00

perf: stop cloning inference inputs in OwnedGame and ingestion

logaritmisk added the refactor label 2026-09-07 13:53:25 +00:00