logaritmisk/trueskill-tt
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Activity

InferenceError: stringly-typed discriminators, and #[non_exhaustive] on the enum buys nothing while the variants lack it #74

New Issue
Closed
opened 2026-09-09 17:56:14 +00:00 by logaritmisk · 2 comments
logaritmisk commented 2026-09-09 17:56:14 +00:00
Owner
Copy Link
Copy Source

1. The variants are not #[non_exhaustive], so every future field is a breaking change

error.rs:43 marks the enum. No variant is marked. From a consumer crate, both of these compile today:

if let InferenceError::NotConverged { iterations, final_step, epsilon } = &e { .. }
let g = InferenceError::GridTooCoarse { needed: 9, max: 3 };   // constructed downstream

#[non_exhaustive] on the enum only stops an exhaustive match over variants. Adding a field to any variant, and downstream construction of any variant, are both in the public contract. This crate added NotConverged and GridTooCoarse in the last two days, so that matters.

Same for Outcome::Scored { scores, sigma } despite the enum being marked, and for ConvergenceOptions, GameOptions, ConvergenceReport, Event, Team, Member, all of which have public fields and no marker.

Fix: #[non_exhaustive] on every struct variant and on the options/report structs. They already impl Default, so ..Default::default() gives the ergonomic path. This is the single change most worth bundling now — every future field addition is otherwise a major bump.

2. Six of fifteen variants use a &'static str discriminator, carrying ~30 magic strings

Variant distinct string values
MismatchedShape { kind } 10
InvalidParameter { name } 9
WrongOutcomeKind { context, expected, got } 3 fields × 2 sites
JointUnavailable { reason } 3 prose sentences
ConflictingCompetitorConfig { field } 2
NonFiniteResult { context } 1

Three concrete problems:

(a) MismatchedShape.kind mixes two levels. Four values name an internal array — "results", "times", "kinds", from the private add_events_with_prior chokepoint a caller has never heard of. Two are full English sentences duplicating the numeric fields: "predict_margin takes exactly 2 teams" alongside expected: 2. Nothing programmatic can be done with any of them, and Display renders kinds: expected length 3, got 2 — meaningless in a log.

(b) InvalidParameter { name, value: f64 } cannot carry a non-scalar. Outcome::try_winner already contorts through it with value: f64::from(winner) for a u32 index. A prior check has no representable value at all.

(c) InvalidProbability { value } is a bespoke variant for one scalar while every other scalar shares InvalidParameter — and it omits the parameter name. Meanwhile HistoryBuilder::p_draw panics on the identical value, so the same parameter has two mechanisms and one has its own variant.

(d) JointUnavailable { reason } is prose where three variants belong. The three reasons are conditions a caller would branch on differently: empty history (add events), ranked events present (use predict_win_probabilities), not positive-definite (numerical). Today matches!(e, JointUnavailable { .. }) is all you can do; distinguishing them means string-matching English.

Proposed:

#[non_exhaustive]
pub enum Parameter { Mu, Sigma, Beta, PDraw, ScoreSigma, Alpha, Epsilon,
                     Weight, DriftScale, DriftVariance, Score, Rank, Prior, WinnerIndex }

#[non_exhaustive]
pub enum InferenceError {
    #[non_exhaustive] InvalidParameter { parameter: Parameter, value: f64 },  // absorbs InvalidProbability
    #[non_exhaustive] MismatchedShape { what: Shape, expected: usize, got: usize },
    #[non_exhaustive] WrongOutcomeKind { expected: OutcomeKind, got: OutcomeKind },  // drop `context`
    #[non_exhaustive] EmptyHistory,                    // was JointUnavailable
    #[non_exhaustive] JointRequiresScoredEvents,       // was JointUnavailable
    #[non_exhaustive] NotPositiveDefinite,             // was JointUnavailable
    ..
}

Keep the Display prose identical — it is genuinely good — but move the discriminating text into the Display match arms rather than into the data.

3. Three Display impls do not meet the standard the others set

The good ones tell the caller what to do, and are why this is worth fixing rather than accepting:

did not converge in {iterations} iterations: final step {final_step:?} is still above epsilon {epsilon}; raise max_iter, or damp with alpha < 1.0 if it is oscillating

the prediction grid needs {needed} nodes … but may hold only {max}; … Use predict_win_probabilities, which is accurate here

The exceptions:

  • InvalidParameter → {name} is invalid: {value}. Renders drift variance is invalid: NaN — no valid range, no remedy, no location. Every neighbour states one.
  • MismatchedShape → {kind}: expected length {expected}, got {got} — see (a).
  • WrongOutcomeKind → accurate, but does not say the obvious next step (call Game::scored), unlike every other message in the file.

4. UnknownKeys is #[non_exhaustive] but wildcarded internally

history.rs:850 and :1104 both match UnknownKeys::Prior => … then _ => Err(UnknownKey). Adding a third variant compiles silently and behaves as Reject. #[non_exhaustive] on your own enum gives no exhaustiveness safety net if you then wildcard it. Spell Reject explicitly at both sites — free, breaks nothing.

Breaks: downstream match on these variants' fields; struct-literal construction of ConvergenceOptions/GameOptions (needs ..Default::default()). This is the largest break in the audit and the reason to do it in one release.

Found by an API audit, 2026-09-09.

## 1. The variants are not `#[non_exhaustive]`, so every future field is a breaking change `error.rs:43` marks the **enum**. No variant is marked. From a consumer crate, both of these compile today: ```rust if let InferenceError::NotConverged { iterations, final_step, epsilon } = &e { .. } let g = InferenceError::GridTooCoarse { needed: 9, max: 3 }; // constructed downstream ``` `#[non_exhaustive]` on the enum only stops an exhaustive `match` over variants. Adding a field to any variant, and downstream construction of any variant, are both in the public contract. This crate added `NotConverged` and `GridTooCoarse` in the last two days, so that matters. Same for `Outcome::Scored { scores, sigma }` despite the enum being marked, and for `ConvergenceOptions`, `GameOptions`, `ConvergenceReport`, `Event`, `Team`, `Member`, all of which have public fields and no marker. **Fix:** `#[non_exhaustive]` on every struct variant and on the options/report structs. They already `impl Default`, so `..Default::default()` gives the ergonomic path. This is the single change most worth bundling now — every future field addition is otherwise a major bump. ## 2. Six of fifteen variants use a `&'static str` discriminator, carrying ~30 magic strings | Variant | distinct string values | |---|---| | `MismatchedShape { kind }` | **10** | | `InvalidParameter { name }` | **9** | | `WrongOutcomeKind { context, expected, got }` | 3 fields × 2 sites | | `JointUnavailable { reason }` | 3 prose sentences | | `ConflictingCompetitorConfig { field }` | 2 | | `NonFiniteResult { context }` | 1 | Three concrete problems: **(a) `MismatchedShape.kind` mixes two levels.** Four values name an internal array — `"results"`, `"times"`, `"kinds"`, from the private `add_events_with_prior` chokepoint a caller has never heard of. Two are full English sentences duplicating the numeric fields: `"predict_margin takes exactly 2 teams"` alongside `expected: 2`. Nothing programmatic can be done with any of them, and `Display` renders `kinds: expected length 3, got 2` — meaningless in a log. **(b) `InvalidParameter { name, value: f64 }` cannot carry a non-scalar.** `Outcome::try_winner` already contorts through it with `value: f64::from(winner)` for a `u32` index. A `prior` check has no representable value at all. **(c) `InvalidProbability { value }` is a bespoke variant for one scalar** while every other scalar shares `InvalidParameter` — and it omits the parameter name. Meanwhile `HistoryBuilder::p_draw` *panics* on the identical value, so the same parameter has two mechanisms and one has its own variant. **(d) `JointUnavailable { reason }` is prose where three variants belong.** The three reasons are conditions a caller would branch on differently: empty history (add events), ranked events present (use `predict_win_probabilities`), not positive-definite (numerical). Today `matches!(e, JointUnavailable { .. })` is all you can do; distinguishing them means string-matching English. **Proposed:** ```rust #[non_exhaustive] pub enum Parameter { Mu, Sigma, Beta, PDraw, ScoreSigma, Alpha, Epsilon, Weight, DriftScale, DriftVariance, Score, Rank, Prior, WinnerIndex } #[non_exhaustive] pub enum InferenceError { #[non_exhaustive] InvalidParameter { parameter: Parameter, value: f64 }, // absorbs InvalidProbability #[non_exhaustive] MismatchedShape { what: Shape, expected: usize, got: usize }, #[non_exhaustive] WrongOutcomeKind { expected: OutcomeKind, got: OutcomeKind }, // drop `context` #[non_exhaustive] EmptyHistory, // was JointUnavailable #[non_exhaustive] JointRequiresScoredEvents, // was JointUnavailable #[non_exhaustive] NotPositiveDefinite, // was JointUnavailable .. } ``` Keep the `Display` prose identical — it is genuinely good — but move the discriminating text into the `Display` match arms rather than into the data. ## 3. Three `Display` impls do not meet the standard the others set The good ones tell the caller what to do, and are why this is worth fixing rather than accepting: > `did not converge in {iterations} iterations: final step {final_step:?} is still above epsilon {epsilon}; raise max_iter, or damp with alpha < 1.0 if it is oscillating` > `the prediction grid needs {needed} nodes … but may hold only {max}; … Use predict_win_probabilities, which is accurate here` The exceptions: - `InvalidParameter` → `{name} is invalid: {value}`. Renders `drift variance is invalid: NaN` — no valid range, no remedy, no location. Every neighbour states one. - `MismatchedShape` → `{kind}: expected length {expected}, got {got}` — see (a). - `WrongOutcomeKind` → accurate, but does not say the obvious next step (*call `Game::scored`*), unlike every other message in the file. ## 4. `UnknownKeys` is `#[non_exhaustive]` but wildcarded internally `history.rs:850` and `:1104` both match `UnknownKeys::Prior => …` then `_ => Err(UnknownKey)`. Adding a third variant compiles silently and behaves as `Reject`. `#[non_exhaustive]` on your *own* enum gives no exhaustiveness safety net if you then wildcard it. Spell `Reject` explicitly at both sites — free, breaks nothing. **Breaks:** downstream `match` on these variants' fields; struct-literal construction of `ConvergenceOptions`/`GameOptions` (needs `..Default::default()`). This is the largest break in the audit and the reason to do it in one release. Found by an API audit, 2026-09-09.
logaritmisk added the apibreaking labels 2026-09-09 17:58:19 +00:00
logaritmisk commented 2026-09-09 20:05:33 +00:00
Author
Owner
Copy Link
Copy Source

Items 1 and 4 are resolved; 2 and 3 are a design change I am not making unilaterally.

1 — variants done, options structs deliberately not

Every struct variant of InferenceError carries #[non_exhaustive] (that landed in the earlier api/cleanup work), and Outcome::Scored does too. ConvergenceReport is marked as of 6a893ff (merged as da55d2a) — it is only ever constructed by converge / converge_partial, so it costs a caller nothing.

ConvergenceOptions and GameOptions are staying constructible, against this issue's recommendation. I tried it, and it turns up a cost the issue did not anticipate:

error[E0639]: cannot create non-exhaustive struct using struct expression
  --> tests/competitor_config.rs:13:41
   |
13 | const CONVERGENCE: ConvergenceOptions = ConvergenceOptions {

Default::default is not a const fn, so the ..Default::default() escape hatch — the whole reason marking an options struct is supposed to be cheap — does not exist in a const. ConvergenceOptions is Copy and a natural const; there is no workaround from outside the crate. Weighed against that, adding a field later is a one-time major bump, and this crate has been cutting those anyway. The reasoning is recorded on the type so it does not get rediscovered.

The other structs the issue lists — Event, Team, Member — are worse candidates still: struct-literal construction of Event is the documented ingestion path, it appears in the README, and Event has no Default to fall back to.

4 — already fine

Both sites (member_skills and the linear-combination path) spell UnknownKeys::Reject explicitly. No wildcard remains.

2 and 3 — left open, and they are one change not two

The typed-discriminator redesign is right, and item 3 depends on it: InvalidParameter's Display cannot state a valid range or a remedy without knowing which parameter, and getting that from a &'static str means matching on magic strings in the Display impl — trading one stringly-typed site for another. Parameter, Shape and OutcomeKind are what make item 3 writable.

Two notes from working nearby:

  • NonFiniteResult { context, step } picked up a second meaning in #78: from converge the pair is the sweep step, from a prediction it is the offending skill's own (mu, sigma). The field name step is now wrong half the time. Whatever this issue does to the enum should split that or rename the field.
  • UnknownKey { team, member, key } carries team: 0 on the flat-key paths (resolve_targets, the linear-combination path), where member indexes a flat key list and there is no team. Documented as such for now, but it is the same shape of problem: one variant serving two situations.

Three new &'static str values have appeared since the audit — "prediction read a skill with no usable mean or variance…" and "beta with point-mass skills" — so the count in the table is a floor, not a ceiling.

Items 1 and 4 are resolved; 2 and 3 are a design change I am not making unilaterally. ## 1 — variants done, options structs deliberately not Every struct variant of `InferenceError` carries `#[non_exhaustive]` (that landed in the earlier `api/cleanup` work), and `Outcome::Scored` does too. `ConvergenceReport` is marked as of 6a893ff (merged as da55d2a) — it is only ever constructed by `converge` / `converge_partial`, so it costs a caller nothing. **`ConvergenceOptions` and `GameOptions` are staying constructible, against this issue's recommendation.** I tried it, and it turns up a cost the issue did not anticipate: ``` error[E0639]: cannot create non-exhaustive struct using struct expression --> tests/competitor_config.rs:13:41 | 13 | const CONVERGENCE: ConvergenceOptions = ConvergenceOptions { ``` `Default::default` is not a `const fn`, so the `..Default::default()` escape hatch — the whole reason marking an options struct is supposed to be cheap — **does not exist in a `const`**. `ConvergenceOptions` is `Copy` and a natural const; there is no workaround from outside the crate. Weighed against that, adding a field later is a one-time major bump, and this crate has been cutting those anyway. The reasoning is recorded on the type so it does not get rediscovered. The other structs the issue lists — `Event`, `Team`, `Member` — are worse candidates still: struct-literal construction of `Event` is the documented ingestion path, it appears in the README, and `Event` has no `Default` to fall back to. ## 4 — already fine Both sites (`member_skills` and the linear-combination path) spell `UnknownKeys::Reject` explicitly. No wildcard remains. ## 2 and 3 — left open, and they are one change not two The typed-discriminator redesign is right, and item 3 depends on it: `InvalidParameter`'s `Display` cannot state a valid range or a remedy without knowing *which* parameter, and getting that from a `&'static str` means matching on magic strings in the `Display` impl — trading one stringly-typed site for another. `Parameter`, `Shape` and `OutcomeKind` are what make item 3 writable. Two notes from working nearby: - `NonFiniteResult { context, step }` picked up a second meaning in #78: from `converge` the pair is the sweep step, from a prediction it is the offending skill's own `(mu, sigma)`. The field name `step` is now wrong half the time. Whatever this issue does to the enum should split that or rename the field. - `UnknownKey { team, member, key }` carries `team: 0` on the flat-key paths (`resolve_targets`, the linear-combination path), where `member` indexes a flat key list and there is no team. Documented as such for now, but it is the same shape of problem: one variant serving two situations. Three new `&'static str` values have appeared since the audit — `"prediction read a skill with no usable mean or variance…"` and `"beta with point-mass skills"` — so the count in the table is a floor, not a ceiling.
logaritmisk commented 2026-09-10 05:31:50 +00:00
Author
Owner
Copy Link
Copy Source

Items 2 and 3 are done, so this closes. 061c481 (merged as 61da3ac).

Four enums replace the thirty-odd magic strings, close to what you sketched:

Parameter        // 13 variants, replacing 9 strings in InvalidParameter
Shape            //  4 variants, replacing 10 in MismatchedShape
OutcomeKind      //  2 variants, replacing WrongOutcomeKind's three fields
CompetitorField  //  2 variants, replacing ConflictingCompetitorConfig's

InvalidProbability folds into InvalidParameter as Parameter::PDraw, as you proposed — it was a bespoke variant for one scalar that also omitted the parameter name.

JointUnavailable splits into EmptyHistory, JointRequiresScoredEvents, NotPositiveDefinite. One test proved immediately that the distinction was load-bearing: my blanket conversion mapped the empty-history case onto the ranked one, and an_empty_history_has_no_joint failed on the spot. Under the old prose field that would have been a passing test.

Two splits beyond the issue

NonFiniteResult → NonFiniteStep { context, step } and NonFiniteSkill { mu, sigma }, as flagged in my earlier comment. One step: (f64, f64) was carrying a sweep step from converge and a skill's own moments from a prediction; no field name is right for both.

InvalidParameter { name: "beta with point-mass skills" } → NoPerformanceVariance. That was never a parameter out of range — both values are individually valid, and it is their combination that leaves nothing varying. Putting it in Parameter would have made the enum a bag of conditions.

Item 3, which item 2 is what makes possible

before   drift variance is invalid: NaN
after    drift variance must be finite and non-negative (got NaN)

before   kinds: expected length 3, got 2
after    the outcome describes a different number of teams than the event has:
         expected 3, got 2

before   Game::ranked: expected Outcome::Ranked, got Outcome::Scored
after    expected Outcome::Ranked, got Outcome::Scored; call Game::scored for a
         scored outcome

Parameter::range() states each parameter's actual bounds, which is exactly what a &'static str name could not do. error::message_tests renders every message and asserts each is a sentence rather than a label — including that none of them still contains the old is invalid: shape — and that the three above now carry a range or a next step.

The four internal MismatchedShape kinds

results, times, kinds and the weights array collapse to Shape::Internal, whose Display says plainly that reaching it is a bug in this crate. They are checks on add_events_with_prior's own parallel arrays and unreachable through the public API. I kept them as checked errors rather than turning them into debug_assert!s: release is where this crate's defects have tended to hide, and the CLAUDE.md rule about running tests in release exists for that reason.

Item 1 (#[non_exhaustive]) and item 4 (the UnknownKeys wildcard) were resolved earlier — see the previous comment for why the options structs deliberately stay constructible.

Items 2 and 3 are done, so this closes. 061c481 (merged as 61da3ac). Four enums replace the thirty-odd magic strings, close to what you sketched: ```rust Parameter // 13 variants, replacing 9 strings in InvalidParameter Shape // 4 variants, replacing 10 in MismatchedShape OutcomeKind // 2 variants, replacing WrongOutcomeKind's three fields CompetitorField // 2 variants, replacing ConflictingCompetitorConfig's ``` `InvalidProbability` folds into `InvalidParameter` as `Parameter::PDraw`, as you proposed — it was a bespoke variant for one scalar that also omitted the parameter name. `JointUnavailable` splits into `EmptyHistory`, `JointRequiresScoredEvents`, `NotPositiveDefinite`. **One test proved immediately that the distinction was load-bearing:** my blanket conversion mapped the empty-history case onto the ranked one, and `an_empty_history_has_no_joint` failed on the spot. Under the old prose field that would have been a passing test. ## Two splits beyond the issue `NonFiniteResult` → `NonFiniteStep { context, step }` and `NonFiniteSkill { mu, sigma }`, as flagged in my earlier comment. One `step: (f64, f64)` was carrying a sweep step from `converge` and a skill's own moments from a prediction; no field name is right for both. `InvalidParameter { name: "beta with point-mass skills" }` → `NoPerformanceVariance`. That was never a parameter out of range — both values are individually valid, and it is their *combination* that leaves nothing varying. Putting it in `Parameter` would have made the enum a bag of conditions. ## Item 3, which item 2 is what makes possible ``` before drift variance is invalid: NaN after drift variance must be finite and non-negative (got NaN) before kinds: expected length 3, got 2 after the outcome describes a different number of teams than the event has: expected 3, got 2 before Game::ranked: expected Outcome::Ranked, got Outcome::Scored after expected Outcome::Ranked, got Outcome::Scored; call Game::scored for a scored outcome ``` `Parameter::range()` states each parameter's actual bounds, which is exactly what a `&'static str` name could not do. `error::message_tests` renders every message and asserts each is a sentence rather than a label — including that none of them still contains the old `is invalid:` shape — and that the three above now carry a range or a next step. ## The four internal `MismatchedShape` kinds `results`, `times`, `kinds` and the weights array collapse to `Shape::Internal`, whose `Display` says plainly that reaching it is a bug in this crate. They are checks on `add_events_with_prior`'s own parallel arrays and unreachable through the public API. I kept them as checked errors rather than turning them into `debug_assert!`s: release is where this crate's defects have tended to hide, and the CLAUDE.md rule about running tests in release exists for that reason. Item 1 (`#[non_exhaustive]`) and item 4 (the `UnknownKeys` wildcard) were resolved earlier — see the previous comment for why the options structs deliberately stay constructible.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
api
Shape or ergonomics of the public surface
 blocked
Waiting on another issue or an external decision
 breaking
Resolving it changes the public API; wants a major bump
 bug
Behaviour is wrong, including silently wrong
 decision
Needs a call to be made, not code to be written
 docs
Documentation, including doc comments and the README
 enhancement
New capability the crate does not have
 infra
CI, release process, packaging
 numerics
Floating-point accuracy, cancellation, convergence maths
 performance
Speed or memory
 refactor
Internal cleanup with no intended behaviour change
 testing
Test coverage and test infrastructure
No labels api breaking
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logaritmisk
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#74