log_evidence takes &mut self but mutates nothing #29

New Issue

2026-08-27T14:48:55Z

logaritmisk commented

2026-08-27 14:48:55 +00:00

History::log_evidence and log_evidence_for take &mut self while mutating nothing. The filtered_* methods added in #19 take &self. The asymmetry is worth removing, but it is an API break, so it should ride along with another one.

`History::log_evidence` and `log_evidence_for` take `&mut self` while mutating nothing. The `filtered_*` methods added in #19 take `&self`. The asymmetry is worth removing, but it is an API break, so it should ride along with another one.

logaritmisk referenced this issue

2026-08-27 15:12:48 +00:00

`HistoryBuilder::online(true)` is non-functional — `Skill.online` is never written #19

logaritmisk commented

2026-08-27 15:21:35 +00:00

Fixed in eeb43e3. log_evidence, log_evidence_for and log_evidence_internal now take &self.

It was not the mechanical change it looked like. Under the rayon feature the closure in log_evidence_internal captured all of &self rather than just the competitor store:

error[E0277]: `K` cannot be shared between threads safely
note: required because it appears within the type `KeyTable<K>`
note: required because it appears within the type `History<T, D, O, K>`

Capturing &History drags KeyTable<K> in and so demands K: Sync from every caller — a bound the key type need not satisfy. That compiled while the method took &mut self (the reborrow narrowed the capture to the store) and stopped compiling the instant it didn't.

Fix is let agents = &self.agents; before the closure, with a comment saying why, since anyone inlining it would reintroduce the bound. Only the rayon feature combination catches this, so it is invisible to a default cargo check.

Bundled with the other small fixes rather than held for a breaking release: loosening &mut self to &self is not source-breaking for ordinary callers, since &mut reborrows as & transparently. The only breakage would be a caller relying on the exclusive borrow for its own aliasing reasons, which is not a thing to rely on here.

Fixed in `eeb43e3`. `log_evidence`, `log_evidence_for` and `log_evidence_internal` now take `&self`. **It was not the mechanical change it looked like.** Under the `rayon` feature the closure in `log_evidence_internal` captured all of `&self` rather than just the competitor store: ``` error[E0277]: `K` cannot be shared between threads safely note: required because it appears within the type `KeyTable<K>` note: required because it appears within the type `History<T, D, O, K>` ``` Capturing `&History` drags `KeyTable<K>` in and so demands `K: Sync` from every caller — a bound the key type need not satisfy. That compiled while the method took `&mut self` (the reborrow narrowed the capture to the store) and stopped compiling the instant it didn't. Fix is `let agents = &self.agents;` before the closure, with a comment saying why, since anyone inlining it would reintroduce the bound. Only the `rayon` feature combination catches this, so it is invisible to a default `cargo check`. Bundled with the other small fixes rather than held for a breaking release: loosening `&mut self` to `&self` is not source-breaking for ordinary callers, since `&mut` reborrows as `&` transparently. The only breakage would be a caller relying on the exclusive borrow for its own aliasing reasons, which is not a thing to rely on here.

logaritmisk closed this issue

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

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 added the api label 2026-09-07 13:53:33 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#29