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

Six exported types are unreachable from History — removing them shrinks the API for free #73

New Issue
Closed
opened 2026-09-09 17:55:48 +00:00 by logaritmisk · 2 comments
logaritmisk commented 2026-09-09 17:55:48 +00:00
Owner
Copy Link
Copy Source
Type Obtainable from History? Named by any reachable public signature?
TimeSlice<T> no — History::time_slices is pub(crate) no
EventKind no only by TimeSlice::add_events
KeyTable<K> no — History::keys is private no
storage::CompetitorStore no — History::agents is pub(crate) no
Competitor<T, D> no — only via CompetitorStore no
Index yes — intern/lookup return it nothing public consumes one

Verified by constructing each from a consumer crate. This compiles and runs, and can never be connected to anything:

let ts: TimeSlice<i64> = TimeSlice::new(1, 0.0, ConvergenceOptions::default());
println!("{:?}", ts.get_composition());     // []
let cs: CompetitorStore<i64, ConstantDrift> = CompetitorStore::new();
let mut kt: KeyTable<&'static str> = KeyTable::new();

TimeSlice is the worst: new, add_events, iteration, get_composition, get_results are all pub on a type you can only build standalone and never feed back. TimeSlice::add_events takes &CompetitorStore and Vec<EventKind> — a public method that runs engine work into a void. EventKind is also a near-duplicate of Outcome (same Ranked/Scored variant names, different payloads), which is a second way to confuse a reader.

Competitor publicly exposes raw temporal EP state (rating, message, last_time) with no reachable consumer, and both its methods are pub(crate) — a public type with public fields and no behaviour.

The whole cluster's sole external consumer in the repo is benches/batch.rs. No test, example, README block or doc comment touches any of them. A benchmark harness is dictating the crate's public surface; moving that bench in-crate frees all six.

Index is the interesting one

intern and lookup return it, and no reachable public API accepts one. KeyTable::key(idx), CompetitorStore::get(idx) and SkillStore::* all take Index — and none is obtainable from a History. key_table.rs:20 states the intent:

Power users can promote &K to Index via get_or_create and skip the lookup on subsequent hot-path calls.

That is not achievable through the public API. intern and lookup have no doc comments of their own. So Index is a handle with nowhere to go.

Either make it pay — current_skill_at(Index), learning_curve_at(Index), predict_* accepting &[&[Index]] — or make Index pub(crate) and drop intern/lookup. As it stands it is surface with no purpose, and it also shadows std::ops::Index, which CompetitorStore literally implements, so use trueskill_tt::* alongside use std::ops::* collides. If it stays, CompetitorId is the better name and would leave the crate with exactly two words for the entity: K (user key) and CompetitorId (interned handle).

Also dead

  • N01 — zero references in the entire repository, including inside the crate.
  • N00, N_INF — internal EP identities, cross-module inside src/ only. See #71.
  • TimeSlice::get_composition / get_results — pub, and their only callers anywhere are #[cfg(test)] code in src/history.rs. They are also the crate's only two C-GETTER violations, which disappear with the type.
  • Gaussian::damp_natural — pub, called only from src/factor/.

Confirmed not leaking: SkillStore (pub(crate) use), Matrix (never exported), ReadmeDoctests (#[cfg(doctest)], unnameable by design).

Fix

Drop all six from lib.rs's export list; pub mod storage → pub(crate); downgrade the pub fns in time_slice.rs and key_table.rs; move benches/batch.rs's Batch::iteration measurement in-crate.

src/storage/mod.rs also violates the project's own "no mod.rs" convention while we are here.

Breaks: anyone naming these — but since none is obtainable from a History, there is nothing they could have been doing with them.

Found by an API audit, 2026-09-09; construction reproduced independently.

| Type | Obtainable from `History`? | Named by any reachable public signature? | |---|---|---| | `TimeSlice<T>` | no — `History::time_slices` is `pub(crate)` | no | | `EventKind` | no | only by `TimeSlice::add_events` | | `KeyTable<K>` | no — `History::keys` is private | no | | `storage::CompetitorStore` | no — `History::agents` is `pub(crate)` | no | | `Competitor<T, D>` | no — only via `CompetitorStore` | no | | `Index` | **yes** — `intern`/`lookup` return it | **nothing public consumes one** | Verified by constructing each from a consumer crate. This compiles and runs, and can never be connected to anything: ```rust let ts: TimeSlice<i64> = TimeSlice::new(1, 0.0, ConvergenceOptions::default()); println!("{:?}", ts.get_composition()); // [] let cs: CompetitorStore<i64, ConstantDrift> = CompetitorStore::new(); let mut kt: KeyTable<&'static str> = KeyTable::new(); ``` `TimeSlice` is the worst: `new`, `add_events`, `iteration`, `get_composition`, `get_results` are all `pub` on a type you can only build standalone and never feed back. `TimeSlice::add_events` takes `&CompetitorStore` and `Vec<EventKind>` — a public method that runs engine work into a void. `EventKind` is also a near-duplicate of `Outcome` (same `Ranked`/`Scored` variant names, different payloads), which is a second way to confuse a reader. `Competitor` publicly exposes raw temporal EP state (`rating`, `message`, `last_time`) with no reachable consumer, and both its methods are `pub(crate)` — a public type with public fields and no behaviour. **The whole cluster's sole external consumer in the repo is `benches/batch.rs`.** No test, example, README block or doc comment touches any of them. A benchmark harness is dictating the crate's public surface; moving that bench in-crate frees all six. ## `Index` is the interesting one `intern` and `lookup` return it, and **no reachable public API accepts one**. `KeyTable::key(idx)`, `CompetitorStore::get(idx)` and `SkillStore::*` all take `Index` — and none is obtainable from a `History`. `key_table.rs:20` states the intent: > Power users can promote `&K` to `Index` via `get_or_create` and skip the lookup on subsequent hot-path calls. That is not achievable through the public API. `intern` and `lookup` have no doc comments of their own. So `Index` is a handle with nowhere to go. Either make it pay — `current_skill_at(Index)`, `learning_curve_at(Index)`, `predict_*` accepting `&[&[Index]]` — or make `Index` `pub(crate)` and drop `intern`/`lookup`. As it stands it is surface with no purpose, and it also shadows `std::ops::Index`, which `CompetitorStore` literally implements, so `use trueskill_tt::*` alongside `use std::ops::*` collides. If it stays, `CompetitorId` is the better name and would leave the crate with exactly two words for the entity: `K` (user key) and `CompetitorId` (interned handle). ## Also dead - `N01` — **zero references in the entire repository**, including inside the crate. - `N00`, `N_INF` — internal EP identities, cross-module inside `src/` only. See #71. - `TimeSlice::get_composition` / `get_results` — `pub`, and their only callers anywhere are `#[cfg(test)]` code in `src/history.rs`. They are also the crate's only two C-GETTER violations, which disappear with the type. - `Gaussian::damp_natural` — `pub`, called only from `src/factor/`. Confirmed **not** leaking: `SkillStore` (`pub(crate) use`), `Matrix` (never exported), `ReadmeDoctests` (`#[cfg(doctest)]`, unnameable by design). ## Fix Drop all six from `lib.rs`'s export list; `pub mod storage` → `pub(crate)`; downgrade the `pub fn`s in `time_slice.rs` and `key_table.rs`; move `benches/batch.rs`'s `Batch::iteration` measurement in-crate. `src/storage/mod.rs` also violates the project's own "no `mod.rs`" convention while we are here. **Breaks:** anyone naming these — but since none is obtainable from a `History`, there is nothing they could have been doing with them. Found by an API audit, 2026-09-09; construction reproduced independently.
logaritmisk added the apibreakingrefactor labels 2026-09-09 17:58:16 +00:00
logaritmisk commented 2026-09-09 19:25:03 +00:00
Author
Owner
Copy Link
Copy Source

Most of this is now done; one item is left and it is a design call, not a cleanup.

Already gone (the api/cleanup work, which moved benches/batch.rs onto the public API and so freed the whole cluster): TimeSlice, EventKind, KeyTable, storage::CompetitorStore, Competitor are no longer exported — time_slice, key_table, competitor are private modules and storage is pub(crate). N01 is deleted. get_composition / get_results are #[cfg(test)].

Done in e4d6dc4 (merged as c3d1afe):

  • Gaussian::damp_natural → pub(crate). This one was a genuine leak, not just a stale export: gaussian is a pub mod, so an EP damping internal called only from src/factor/ was reachable from outside the crate.
  • The stray pub fns in time_slice.rs, key_table.rs and matrix.rs → pub(crate), so their visibility states what it means rather than relying on the module happening to be private.
  • storage/mod.rs → storage.rs, factor/mod.rs → factor.rs.

Left open: Index. Still pub, still returned by intern/lookup, still accepted by nothing public. The fork the issue lays out is real and I am not picking it unilaterally:

  1. Make it pay — current_skill_at(Index), learning_curve_at(Index), predict_* over &[&[Index]]. Delivers the hot-path story key_table.rs:20 already claims.
  2. Retire it — Index becomes pub(crate), intern/lookup go. Free, and removes the collision with std::ops::Index that CompetitorStore literally implements.

If (1), CompetitorId is the better name and leaves exactly two words for the entity: K and CompetitorId.

Most of this is now done; one item is left and it is a design call, not a cleanup. **Already gone** (the `api/cleanup` work, which moved `benches/batch.rs` onto the public API and so freed the whole cluster): `TimeSlice`, `EventKind`, `KeyTable`, `storage::CompetitorStore`, `Competitor` are no longer exported — `time_slice`, `key_table`, `competitor` are private modules and `storage` is `pub(crate)`. `N01` is deleted. `get_composition` / `get_results` are `#[cfg(test)]`. **Done in e4d6dc4** (merged as c3d1afe): - `Gaussian::damp_natural` → `pub(crate)`. This one was a genuine leak, not just a stale export: `gaussian` is a `pub mod`, so an EP damping internal called only from `src/factor/` was reachable from outside the crate. - The stray `pub fn`s in `time_slice.rs`, `key_table.rs` and `matrix.rs` → `pub(crate)`, so their visibility states what it means rather than relying on the module happening to be private. - `storage/mod.rs` → `storage.rs`, `factor/mod.rs` → `factor.rs`. **Left open: `Index`.** Still `pub`, still returned by `intern`/`lookup`, still accepted by nothing public. The fork the issue lays out is real and I am not picking it unilaterally: 1. **Make it pay** — `current_skill_at(Index)`, `learning_curve_at(Index)`, `predict_*` over `&[&[Index]]`. Delivers the hot-path story `key_table.rs:20` already claims. 2. **Retire it** — `Index` becomes `pub(crate)`, `intern`/`lookup` go. Free, and removes the collision with `std::ops::Index` that `CompetitorStore` literally implements. If (1), `CompetitorId` is the better name and leaves exactly two words for the entity: `K` and `CompetitorId`.
logaritmisk commented 2026-09-09 21:19:07 +00:00
Author
Owner
Copy Link
Copy Source

Option 2 — retired. faa25fb (merged as 6e2ce69). This closes the issue.

Index, History::intern and History::lookup are all pub(crate) or gone. intern stays internal because ingestion needs it; lookup is deleted outright, since current_skill, rating and learning_curve already answer "does this history know this key" and (since #72) all three take a borrowed key.

The three tests that used them were asserting things a caller cannot observe, which is itself the argument for the removal:

  • comparing two opaque indices for inequality → comparing the two posteriors, and asserting the winner is ahead
  • intern_is_idempotent → a_repeated_key_is_one_competitor: a key in two events yields one competitor with a two-point learning curve, the observable form of the same claim
  • lookup_returns_none_for_missing → an_unknown_key_is_unknown

Also cleaned up in passing: UnknownKey's doc linked crate::Index (now a private-item link warning), and three # Preconditions blocks plus the README told readers to "pre-filter with lookup or current_skill". They say current_skill now.

The whole issue is done — the five unreachable types went with the api/cleanup work, Gaussian::damp_natural and the stray pub fns in private modules were tightened in e4d6dc4, and mod.rs is gone from storage/ and factor/.

Option 2 — retired. faa25fb (merged as 6e2ce69). This closes the issue. `Index`, `History::intern` and `History::lookup` are all `pub(crate)` or gone. `intern` stays internal because ingestion needs it; `lookup` is deleted outright, since `current_skill`, `rating` and `learning_curve` already answer "does this history know this key" and (since #72) all three take a borrowed key. The three tests that used them were asserting things a caller cannot observe, which is itself the argument for the removal: - comparing two opaque indices for inequality → comparing the two posteriors, and asserting the winner is ahead - `intern_is_idempotent` → `a_repeated_key_is_one_competitor`: a key in two events yields one competitor with a two-point learning curve, the observable form of the same claim - `lookup_returns_none_for_missing` → `an_unknown_key_is_unknown` Also cleaned up in passing: `UnknownKey`'s doc linked `crate::Index` (now a private-item link warning), and three `# Preconditions` blocks plus the README told readers to "pre-filter with `lookup` or `current_skill`". They say `current_skill` now. The whole issue is done — the five unreachable types went with the `api/cleanup` work, `Gaussian::damp_natural` and the stray `pub fn`s in private modules were tightened in e4d6dc4, and `mod.rs` is gone from `storage/` and `factor/`.
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 refactor
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#73