Error API: two variants are never constructed, converge() never returns Err, and MismatchedShape is misused for type errors #20

New Issue

2026-08-04T19:19:30Z

logaritmisk commented

2026-08-04 19:19:30 +00:00

InferenceError (src/error.rs) promises more than the crate delivers, and one of its variants produces nonsense messages.

1. Two variants are never constructed

grep -rn "ConvergenceFailed\|NegativePrecision" src/ returns hits only in src/error.rs itself — the definitions and their Display arms. Neither is ever returned by any code path:

ConvergenceFailed { last_step, iterations } — never returned even though non-convergence is a real, reachable outcome.
NegativePrecision { pi } — never returned even though Gaussian::mu/sigma have explicit guards for exactly this state (src/gaussian.rs:60, 72).

They are public API surface advertising failure modes callers can't actually observe. Match arms written against them are dead.

2. `History::converge()` returns `Result` but is infallible

src/history.rs:432-459 ends with an unconditional Ok(ConvergenceReport { … }). Non-convergence is reported as converged: false inside the report, not as an error. That is a defensible design — but then the Result is noise, forcing every caller to .unwrap() something that can never fail.

Pick one:

Return ConvergenceReport directly and drop the Result; delete ConvergenceFailed. Callers inspect report.converged.
Or return Err(InferenceError::ConvergenceFailed { … }) when the iteration cap is hit without reaching epsilon, and keep the Result meaningful.

The first is probably right — a report carrying iterations, final_step, and timings is more useful than an error — but the current state is the one option that helps nobody. Note this interacts with #11 (empty history panics where it should return cleanly) and #8 (NaN currently reported as converged: true).

3. `MismatchedShape` is used for "wrong outcome variant"

src/game.rs:459-465:

let ranks = outcome.as_ranks().ok_or(crate::InferenceError::MismatchedShape {
    kind: "Game::ranked requires Outcome::Ranked",
    expected: 0,
    got: 0,
})?;

and the same pattern at src/game.rs:498-504. Its Display impl (src/error.rs) renders:

Game::ranked requires Outcome::Ranked: expected length 0, got 0

The "expected length 0, got 0" is meaningless filler appended to what is really a variant mismatch, not a shape mismatch. Add a dedicated variant, e.g.:

WrongOutcomeKind { expected: &'static str, got: &'static str },

4. Smaller items in the same area

Outcome::winner panics via assert! on an out-of-range index (src/outcome.rs:34) while its siblings return values — a public constructor that panics on bad input in a crate that has an error type.
InferenceError has no source() and no #[non_exhaustive]. Adding #[non_exhaustive] now, while the crate is at 0.1.x, keeps future variants from being breaking changes — Outcome and EventKind already do this.

Acceptance

No InferenceError variant is unconstructible from the public API (or unused ones are removed).
converge()'s signature matches its actual fallibility.
Wrong-variant errors render a sensible message.
CHANGELOG.md records the API changes.

`InferenceError` (`src/error.rs`) promises more than the crate delivers, and one of its variants produces nonsense messages. ## 1. Two variants are never constructed `grep -rn "ConvergenceFailed\|NegativePrecision" src/` returns hits only in `src/error.rs` itself — the definitions and their `Display` arms. Neither is ever returned by any code path: - `ConvergenceFailed { last_step, iterations }` — never returned even though non-convergence is a real, reachable outcome. - `NegativePrecision { pi }` — never returned even though `Gaussian::mu`/`sigma` have explicit guards for exactly this state (`src/gaussian.rs:60`, `72`). They are public API surface advertising failure modes callers can't actually observe. Match arms written against them are dead. ## 2. `History::converge()` returns `Result` but is infallible `src/history.rs:432-459` ends with an unconditional `Ok(ConvergenceReport { … })`. Non-convergence is reported as `converged: false` inside the report, not as an error. That is a defensible design — but then the `Result` is noise, forcing every caller to `.unwrap()` something that can never fail. Pick one: - Return `ConvergenceReport` directly and drop the `Result`; delete `ConvergenceFailed`. Callers inspect `report.converged`. - Or return `Err(InferenceError::ConvergenceFailed { … })` when the iteration cap is hit without reaching epsilon, and keep the `Result` meaningful. The first is probably right — a report carrying `iterations`, `final_step`, and timings is more useful than an error — but the current state is the one option that helps nobody. Note this interacts with #11 (empty history panics where it should return cleanly) and #8 (NaN currently reported as `converged: true`). ## 3. `MismatchedShape` is used for "wrong outcome variant" `src/game.rs:459-465`: ```rust let ranks = outcome.as_ranks().ok_or(crate::InferenceError::MismatchedShape { kind: "Game::ranked requires Outcome::Ranked", expected: 0, got: 0, })?; ``` and the same pattern at `src/game.rs:498-504`. Its `Display` impl (`src/error.rs`) renders: ``` Game::ranked requires Outcome::Ranked: expected length 0, got 0 ``` The "expected length 0, got 0" is meaningless filler appended to what is really a variant mismatch, not a shape mismatch. Add a dedicated variant, e.g.: ```rust WrongOutcomeKind { expected: &'static str, got: &'static str }, ``` ## 4. Smaller items in the same area - `Outcome::winner` panics via `assert!` on an out-of-range index (`src/outcome.rs:34`) while its siblings return values — a public constructor that panics on bad input in a crate that has an error type. - `InferenceError` has no `source()` and no `#[non_exhaustive]`. Adding `#[non_exhaustive]` now, while the crate is at 0.1.x, keeps future variants from being breaking changes — `Outcome` and `EventKind` already do this. ## Acceptance - No `InferenceError` variant is unconstructible from the public API (or unused ones are removed). - `converge()`'s signature matches its actual fallibility. - Wrong-variant errors render a sensible message. - `CHANGELOG.md` records the API changes.

logaritmisk referenced this issue

2026-08-04 19:21:19 +00:00

`cargo publish` would fail: no `description`, `license`, or `repository` — and the crate has no top-level docs #25

logaritmisk referenced this issue

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

`cargo publish` would fail: no `description`, `license`, or `repository` — and the crate has no top-level docs #25

logaritmisk commented

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

Partly done — staying open.

Done:

WrongOutcomeKind { context, expected, got } added and used at both sites, so the nonsense "expected length 0, got 0" message is gone
InferenceError is #[non_exhaustive], so future variants are not breaking
NonFiniteResult { context, step } added and returned by converge() when EP breaks down — so converge() is now genuinely fallible, and its Result means something
Outcome::scores_with_sigma no longer panics; the value is validated at ingestion

Still open:

ConvergenceFailed and NegativePrecision are still never constructed. ConvergenceFailed is the interesting one: it depends on the unresolved question of whether hitting max_iter should be an error or stay a converged: false report. I have not made that call — it changes what every caller has to handle, and the report carries genuinely useful data (iterations, final step, per-iteration timings) that an error would discard.
Outcome::winner still panics on an out-of-range index.
No source() on the error type.

Partly done — **staying open.** Done: - `WrongOutcomeKind { context, expected, got }` added and used at both sites, so the nonsense "expected length 0, got 0" message is gone - `InferenceError` is `#[non_exhaustive]`, so future variants are not breaking - `NonFiniteResult { context, step }` added and returned by `converge()` when EP breaks down — so `converge()` is now genuinely fallible, and its `Result` means something - `Outcome::scores_with_sigma` no longer panics; the value is validated at ingestion **Still open:** - `ConvergenceFailed` and `NegativePrecision` are still never constructed. `ConvergenceFailed` is the interesting one: it depends on the unresolved question of whether hitting `max_iter` should be an error or stay a `converged: false` report. I have not made that call — it changes what every caller has to handle, and the report carries genuinely useful data (iterations, final step, per-iteration timings) that an error would discard. - `Outcome::winner` still panics on an out-of-range index. - No `source()` on the error type.

logaritmisk referenced this issue from a commit

2026-09-07 13:42:40 +00:00

fix!: apply competitor configuration whenever it is supplied

logaritmisk commented

2026-09-07 13:42:56 +00:00

Item 1 is done in 8c087ad. Most of the rest turned out to be already resolved. Leaving this open for the one thing that isn't.

Item 1 — done

ConvergenceFailed and NegativePrecision are removed. Confirmed by grep that neither had a single construction site outside error.rs itself. Done now rather than later because removing a public variant is breaking even under #[non_exhaustive] — that attribute stops downstream from matching exhaustively, but a caller naming a removed variant still fails to compile. It had to ride the same major bump as the rest of this release.

Items 2–4 — mostly stale, verified against `main`

Item 2 (converge() returns Result but is infallible) — no longer true. It returns Err(InferenceError::NonFiniteResult { .. }) when a sweep produces NaN or infinity (src/history.rs). The Result is meaningful, and the design settled on the "report carries converged: false, errors are for numerical breakdown" split rather than either option listed above.
Item 3 (MismatchedShape misused for variant errors) — fixed. WrongOutcomeKind { context, expected, got } exists and is what Game::ranked / Game::scored return.
Item 4, second half (#[non_exhaustive]) — applied, to InferenceError, Outcome and EventKind.

Still open

Outcome::winner panics on an out-of-range index (src/outcome.rs:39):

pub fn winner(winner: u32, n: u32) -> Self {
    assert!(winner < n, "winner index {winner} out of range 0..{n}");

It is now documented as panicking, so it reads as a deliberate choice rather than an oversight — but the original observation stands: it is a public constructor that panics on bad input in a crate that has an error type, while its siblings return values.

This one is a genuine design call rather than a cleanup, which is why it was not swept in with the rest:

Outcome::winner(0, 2) appears throughout the test suite and the README as a plain expression. Returning Result would put a ? or .unwrap() on every one of them, for an argument pair that is almost always a literal.
The counter-argument is that n and winner are frequently computed from a caller's own data, which is exactly when a panic is the wrong answer.

If it is worth changing, this release is the moment — it is breaking, and another major bump for one constructor would be a poor trade. If not, this issue can close on the grounds that the panic is documented and intentional.

InferenceError::source() from item 4 was also never added; worth folding into whichever way the above goes.

**Item 1 is done** in `8c087ad`. Most of the rest turned out to be already resolved. Leaving this open for the one thing that isn't. ## Item 1 — done `ConvergenceFailed` and `NegativePrecision` are removed. Confirmed by grep that neither had a single construction site outside `error.rs` itself. Done now rather than later because removing a public variant is breaking even under `#[non_exhaustive]` — that attribute stops downstream from matching exhaustively, but a caller naming a removed variant still fails to compile. It had to ride the same major bump as the rest of this release. ## Items 2–4 — mostly stale, verified against `main` - **Item 2** (`converge()` returns `Result` but is infallible) — no longer true. It returns `Err(InferenceError::NonFiniteResult { .. })` when a sweep produces NaN or infinity (`src/history.rs`). The `Result` is meaningful, and the design settled on the "report carries `converged: false`, errors are for numerical breakdown" split rather than either option listed above. - **Item 3** (`MismatchedShape` misused for variant errors) — fixed. `WrongOutcomeKind { context, expected, got }` exists and is what `Game::ranked` / `Game::scored` return. - **Item 4, second half** (`#[non_exhaustive]`) — applied, to `InferenceError`, `Outcome` and `EventKind`. ## Still open **`Outcome::winner` panics on an out-of-range index** (`src/outcome.rs:39`): ```rust pub fn winner(winner: u32, n: u32) -> Self { assert!(winner < n, "winner index {winner} out of range 0..{n}"); ``` It is now *documented* as panicking, so it reads as a deliberate choice rather than an oversight — but the original observation stands: it is a public constructor that panics on bad input in a crate that has an error type, while its siblings return values. This one is a genuine design call rather than a cleanup, which is why it was not swept in with the rest: - `Outcome::winner(0, 2)` appears throughout the test suite and the README as a plain expression. Returning `Result` would put a `?` or `.unwrap()` on every one of them, for an argument pair that is almost always a literal. - The counter-argument is that `n` and `winner` are frequently *computed* from a caller's own data, which is exactly when a panic is the wrong answer. If it is worth changing, this release is the moment — it is breaking, and another major bump for one constructor would be a poor trade. If not, this issue can close on the grounds that the panic is documented and intentional. `InferenceError::source()` from item 4 was also never added; worth folding into whichever way the above goes.

logaritmisk added the api breaking labels 2026-09-07 13:52:04 +00:00

logaritmisk referenced this issue

2026-09-07 14:02:28 +00:00

The `Schedule` trait is public API that the engine never uses — wire it into `run_chain` or make it private #42

logaritmisk referenced this issue from a commit

2026-09-08 00:14:48 +00:00

refactor!: remove the factor-graph surface nothing used, add try_winner

logaritmisk closed this issue

2026-09-08 00:14:48 +00:00

Sign in to join this conversation.