A dropped EventBuilder silently ingests nothing — no #[must_use] #67

New Issue

2026-09-09T17:52:53Z

logaritmisk commented

2026-09-09 17:52:53 +00:00

EventBuilder is the crate's headline ingestion ergonomic, and forgetting the terminal .commit() is a silent no-op.

Measured

let mut h = History::builder().build();
h.event(1).team(["x"]).team(["y"]).ranking([0, 1]);   // no .commit()

time_slices_len  = 0
current_skill(x) = None

No warning, no error. The event is simply not there, and the next thing the caller does is converge an empty history and read None skills.

ConvergenceReport carries a #[must_use] with an excellent message for exactly this class of mistake. EventBuilder — where the mistake is easier to make and the consequence is silent rather than merely unchecked — has none.

The coverage is inconsistent generally

Present: ConvergenceReport, Gaussian's accessors, Rating's getters, ConstantDrift::new/gamma, Outcome::winner/draw, Joint::variables, History::builder/rating/observer/into_observer/filtered_log_evidence/time_slices_len.

Absent: EventBuilder and every one of its setters; HistoryBuilder and all nine of its setters — so builder.p_draw(0.1); is also a silent no-op; History::log_evidence, log_evidence_for, current_skill, learning_curve(s), filtered_learning_curve(s), lookup, joint, every predict_*, posterior_of*, expected_*; the types Prediction, Gaussian, Joint, OwnedGame; Member::with_*, Team::with_members, Outcome::ranking/scores/try_winner.

filtered_log_evidence has it and log_evidence does not. rating has it and current_skill does not. Rating::with_drift_scale has it and Member::with_drift_scale does not. There is no rule, which is what makes it worth fixing in one pass rather than case by case.

A second, opposite problem: `ConvergenceReport`'s `#[must_use]` misfires on `converge()`

Its message reads:

from converge_partial this may describe a fit that stopped at max_iter …

but it fires on converge() too, where that reason is false — converge returns Err(NotConverged) in exactly that case. So every quickstart has to write let _ = h.converge()?;, and the crate's own examples disagree about it: src/lib.rs writes let report = …; assert!(report.converged), the README writes h.converge().unwrap(); (which warns), and several doctests write let _ =.

Fix

#[must_use] on the types EventBuilder, HistoryBuilder, Prediction, Gaussian, Joint, OwnedGame, which covers most method returns at once, plus the individual accessors listed above.

EventBuilder gets a message in ConvergenceReport's style:

#[must_use = "an event is only recorded by `.commit()`; a dropped builder \
              silently ingests nothing"]

Move ConvergenceReport's #[must_use] off the type and onto converge_partial's return, so the warning only fires where its reason is true. Then fix the three disagreeing quickstarts to one spelling.

Breaks: nothing at compile time — warnings only. Downstream -D warnings builds will need updating, which is worth a changelog line.

Found by an API audit, 2026-09-09; the no-op reproduced independently.

`EventBuilder` is the crate's headline ingestion ergonomic, and forgetting the terminal `.commit()` is a silent no-op. ## Measured ```rust let mut h = History::builder().build(); h.event(1).team(["x"]).team(["y"]).ranking([0, 1]); // no .commit() ``` ``` time_slices_len = 0 current_skill(x) = None ``` No warning, no error. The event is simply not there, and the next thing the caller does is converge an empty history and read `None` skills. `ConvergenceReport` carries a `#[must_use]` with an excellent message for exactly this class of mistake. `EventBuilder` — where the mistake is easier to make and the consequence is silent rather than merely unchecked — has none. ## The coverage is inconsistent generally Present: `ConvergenceReport`, `Gaussian`'s accessors, `Rating`'s getters, `ConstantDrift::new`/`gamma`, `Outcome::winner`/`draw`, `Joint::variables`, `History::builder`/`rating`/`observer`/`into_observer`/`filtered_log_evidence`/`time_slices_len`. Absent: **`EventBuilder`** and every one of its setters; **`HistoryBuilder`** and all nine of its setters — so `builder.p_draw(0.1);` is also a silent no-op; `History::log_evidence`, `log_evidence_for`, `current_skill`, `learning_curve(s)`, `filtered_learning_curve(s)`, `lookup`, `joint`, every `predict_*`, `posterior_of*`, `expected_*`; the types `Prediction`, `Gaussian`, `Joint`, `OwnedGame`; `Member::with_*`, `Team::with_members`, `Outcome::ranking`/`scores`/`try_winner`. `filtered_log_evidence` has it and `log_evidence` does not. `rating` has it and `current_skill` does not. `Rating::with_drift_scale` has it and `Member::with_drift_scale` does not. There is no rule, which is what makes it worth fixing in one pass rather than case by case. ## A second, opposite problem: `ConvergenceReport`'s `#[must_use]` misfires on `converge()` Its message reads: > from `converge_partial` this may describe a fit that stopped at `max_iter` … but it fires on `converge()` too, where that reason is **false** — `converge` returns `Err(NotConverged)` in exactly that case. So every quickstart has to write `let _ = h.converge()?;`, and the crate's own examples disagree about it: `src/lib.rs` writes `let report = …; assert!(report.converged)`, the README writes `h.converge().unwrap();` (which warns), and several doctests write `let _ =`. ## Fix - `#[must_use]` on the types `EventBuilder`, `HistoryBuilder`, `Prediction`, `Gaussian`, `Joint`, `OwnedGame`, which covers most method returns at once, plus the individual accessors listed above. - `EventBuilder` gets a message in `ConvergenceReport`'s style: ```rust #[must_use = "an event is only recorded by `.commit()`; a dropped builder \ silently ingests nothing"] ``` - Move `ConvergenceReport`'s `#[must_use]` off the **type** and onto `converge_partial`'s return, so the warning only fires where its reason is true. Then fix the three disagreeing quickstarts to one spelling. **Breaks:** nothing at compile time — warnings only. Downstream `-D warnings` builds will need updating, which is worth a changelog line. Found by an API audit, 2026-09-09; the no-op reproduced independently.

logaritmisk added the api bug labels 2026-09-09 17:57:54 +00:00

logaritmisk referenced this issue from a commit

2026-09-09 18:57:42 +00:00

feat: add the missing trait impls and make `#[must_use]` consistent

logaritmisk referenced this issue from a commit

2026-09-09 19:24:40 +00:00

fix: warn on dropped builders and values; stop exporting EP internals

logaritmisk closed this issue

2026-09-09 19:24:40 +00:00

logaritmisk referenced this issue from a commit

2026-09-09 19:24:41 +00:00

Merge api/must-use-and-visibility (#67, #73)

logaritmisk commented

2026-09-09 19:24:49 +00:00

Fixed in e4d6dc4 (merged as c3d1afe).

The rule the issue asked for: #[must_use] goes on the type, not on each method. That covers every constructor and builder setter in one place, so there is nothing to keep in sync. EventBuilder, HistoryBuilder, Prediction, Gaussian and OwnedGame already had it; this adds Joint, Team, Member and Outcome, plus Prediction::outcomes (an iterator, so the type rule does not reach it). The now-redundant per-method attributes on Team::new, Outcome::winner and Outcome::draw came off — clippy's double_must_use flags them.

Everything else in the "absent" list turned out to be a Result, which is already #[must_use].

Verified by compiling a program that drops each value and reading the warnings back, rather than assuming the attribute took:

warning: unused `EventBuilder` that must be used
warning: unused `Team` that must be used
warning: unused `Member` that must be used
warning: unused `Outcome` that must be used
warning: unused return value of `History::current_skills` that must be used

The ConvergenceReport half was already resolved in the earlier api/cleanup work: the attribute now sits on converge_partial's return with the "may have stopped at max_iter" reason, not on the type, so it no longer fires on converge where that reason is false.

Fixed in e4d6dc4 (merged as c3d1afe). The rule the issue asked for: `#[must_use]` goes on the **type**, not on each method. That covers every constructor and builder setter in one place, so there is nothing to keep in sync. `EventBuilder`, `HistoryBuilder`, `Prediction`, `Gaussian` and `OwnedGame` already had it; this adds `Joint`, `Team`, `Member` and `Outcome`, plus `Prediction::outcomes` (an iterator, so the type rule does not reach it). The now-redundant per-method attributes on `Team::new`, `Outcome::winner` and `Outcome::draw` came off — clippy's `double_must_use` flags them. Everything else in the "absent" list turned out to be a `Result`, which is already `#[must_use]`. Verified by compiling a program that drops each value and reading the warnings back, rather than assuming the attribute took: ``` warning: unused `EventBuilder` that must be used warning: unused `Team` that must be used warning: unused `Member` that must be used warning: unused `Outcome` that must be used warning: unused return value of `History::current_skills` that must be used ``` The `ConvergenceReport` half was already resolved in the earlier `api/cleanup` work: the attribute now sits on `converge_partial`'s return with the "may have stopped at `max_iter`" reason, not on the type, so it no longer fires on `converge` where that reason is false.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#67

A dropped EventBuilder silently ingests nothing — no #[must_use] #67

Measured

The coverage is inconsistent generally

A second, opposite problem: ConvergenceReport's #[must_use] misfires on converge()

Fix

A dropped `EventBuilder` silently ingests nothing — no `#[must_use]` #67

A second, opposite problem: `ConvergenceReport`'s `#[must_use]` misfires on `converge()`