From 2fff745c3bceb7783a6789609eb0a68fe0ec915d Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Mon, 7 Sep 2026 15:13:28 +0200 Subject: [PATCH] feat: let observers be shared, boxed, or borrowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `History` takes its observer by value and never hands it back, so a caller who wanted to read what an observer recorded had no way to keep a handle to it. The natural spelling did not compile: let recorder = Arc::new(Recorder::default()); History::builder().observer(Arc::clone(&recorder)) // error[E0277]: `Arc: Observer` is not satisfied The workaround was for every observer to wrap each of its own fields in an `Arc` and derive `Clone` — one allocation and one lock per field, a pattern each implementor had to rediscover, and nothing documenting it. Adds blanket `Observer` impls for `Arc`, `Box` and `&O`. All are `?Sized`, so `Arc>` and `Box>` work too and an observer can be chosen at runtime. Also adds `History::observer()` and `into_observer()`, so a non-shared observer's state can be inspected in place or reclaimed after `converge` without needing interior mutability at all. `tests/observer.rs` is simplified to the shared spelling, so the recommended pattern is the one demonstrated rather than the workaround. Closes #40 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/history.rs | 20 ++++++++++++ src/observer.rs | 77 ++++++++++++++++++++++++++++++++++++++++++++ tests/observer.rs | 82 +++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 166 insertions(+), 13 deletions(-) diff --git a/src/history.rs b/src/history.rs index 4b2eb6d..36d025e 100644 --- a/src/history.rs +++ b/src/history.rs @@ -538,6 +538,26 @@ impl, O: Observer, K: Eq + Hash + Clone> History &O { + &self.observer + } + + /// Consume the history and return its observer. + /// + /// Useful for reclaiming a non-shared observer's accumulated state after + /// `converge` without needing interior mutability. + #[must_use] + pub fn into_observer(self) -> O { + self.observer + } + /// Every team's member skills, validated. /// /// # Errors diff --git a/src/observer.rs b/src/observer.rs index 851d194..ea9ff42 100644 --- a/src/observer.rs +++ b/src/observer.rs @@ -26,6 +26,83 @@ pub trait Observer: Send + Sync { fn on_converged(&self, _iters: usize, _final_step: (f64, f64), _converged: bool) {} } +/// Shared and boxed observers forward to what they point at. +/// +/// `History` takes its observer by value, so a caller who wants to *read* what +/// an observer recorded has to keep a handle to it. Without these impls the +/// natural spelling does not compile: +/// +/// ``` +/// # use std::sync::{Arc, Mutex}; +/// # use trueskill_tt::{History, Observer}; +/// #[derive(Default)] +/// struct Recorder { +/// iterations: Mutex>, +/// } +/// +/// impl Observer for Recorder { +/// fn on_iteration_end(&self, iter: usize, _step: (f64, f64)) { +/// self.iterations.lock().unwrap().push(iter); +/// } +/// } +/// +/// let recorder = Arc::new(Recorder::default()); +/// let mut h = History::builder().observer(Arc::clone(&recorder)).build(); +/// h.record_winner(&"a", &"b", 1).unwrap(); +/// h.converge().unwrap(); +/// +/// // The caller's handle sees what the history's copy recorded. +/// assert!(!recorder.iterations.lock().unwrap().is_empty()); +/// ``` +/// +/// The alternative was for every observer to wrap each of its own fields in an +/// `Arc` and derive `Clone` — one allocation and one lock per field, and a +/// pattern each implementor had to rediscover. +/// +/// `?Sized` is deliberate: it makes `Arc>` and +/// `Box>` work, so observers can be chosen at runtime. +impl + ?Sized> Observer for std::sync::Arc { + fn on_iteration_end(&self, iter: usize, max_step: (f64, f64)) { + (**self).on_iteration_end(iter, max_step); + } + + fn on_slice_processed(&self, time: &T, slice_idx: usize, n_events: usize) { + (**self).on_slice_processed(time, slice_idx, n_events); + } + + fn on_converged(&self, iters: usize, final_step: (f64, f64), converged: bool) { + (**self).on_converged(iters, final_step, converged); + } +} + +impl + ?Sized> Observer for Box { + fn on_iteration_end(&self, iter: usize, max_step: (f64, f64)) { + (**self).on_iteration_end(iter, max_step); + } + + fn on_slice_processed(&self, time: &T, slice_idx: usize, n_events: usize) { + (**self).on_slice_processed(time, slice_idx, n_events); + } + + fn on_converged(&self, iters: usize, final_step: (f64, f64), converged: bool) { + (**self).on_converged(iters, final_step, converged); + } +} + +impl + ?Sized> Observer for &O { + fn on_iteration_end(&self, iter: usize, max_step: (f64, f64)) { + (**self).on_iteration_end(iter, max_step); + } + + fn on_slice_processed(&self, time: &T, slice_idx: usize, n_events: usize) { + (**self).on_slice_processed(time, slice_idx, n_events); + } + + fn on_converged(&self, iters: usize, final_step: (f64, f64), converged: bool) { + (**self).on_converged(iters, final_step, converged); + } +} + /// ZST no-op observer; the default when none is configured. #[derive(Copy, Clone, Debug, Default)] pub struct NullObserver; diff --git a/tests/observer.rs b/tests/observer.rs index 94d37fb..cd6adef 100644 --- a/tests/observer.rs +++ b/tests/observer.rs @@ -8,14 +8,13 @@ use std::sync::{Arc, Mutex}; use trueskill_tt::{History, Observer}; -/// `History` takes its observer by value and never hands it back, so a test -/// that wants to read what was recorded shares the storage rather than the -/// observer: the handles are cloned, the buffers are not. -#[derive(Clone, Default)] +/// Plain fields. `Arc` implements `Observer`, so the caller shares the +/// observer itself rather than wrapping each field in its own `Arc`. +#[derive(Default)] struct Recorder { - iterations: Arc>>, - slices: Arc>>, - converged: Arc>>, + iterations: Mutex>, + slices: Mutex>, + converged: Mutex>, } impl Observer for Recorder { @@ -37,8 +36,8 @@ impl Observer for Recorder { #[test] fn every_observer_callback_fires() { - let recorder = Recorder::default(); - let mut h = History::builder().observer(recorder.clone()).build(); + let recorder = Arc::new(Recorder::default()); + let mut h = History::builder().observer(Arc::clone(&recorder)).build(); h.record_winner(&"a", &"b", 1).unwrap(); h.record_winner(&"b", &"c", 2).unwrap(); @@ -61,8 +60,8 @@ fn every_observer_callback_fires() { #[test] fn slice_callbacks_report_the_slice_they_swept() { - let recorder = Recorder::default(); - let mut h = History::builder().observer(recorder.clone()).build(); + let recorder = Arc::new(Recorder::default()); + let mut h = History::builder().observer(Arc::clone(&recorder)).build(); h.record_winner(&"a", &"b", 10).unwrap(); h.record_winner(&"a", &"b", 20).unwrap(); @@ -90,8 +89,8 @@ fn slice_callbacks_report_the_slice_they_swept() { #[test] fn a_single_slice_history_still_reports_its_sweep() { - let recorder = Recorder::default(); - let mut h = History::builder().observer(recorder.clone()).build(); + let recorder = Arc::new(Recorder::default()); + let mut h = History::builder().observer(Arc::clone(&recorder)).build(); h.record_winner(&"a", &"b", 1).unwrap(); h.converge().unwrap(); @@ -103,3 +102,60 @@ fn a_single_slice_history_still_reports_its_sweep() { ); assert!(slices.iter().all(|&(t, idx, _)| t == 1 && idx == 0)); } + +/// The gap #40 closed: without `impl Observer for Arc`, an observer that +/// accumulates anything had to wrap every field in its own `Arc` and derive +/// `Clone`, because `History` consumes the observer and never hands it back. +#[test] +fn a_shared_observer_reaches_the_callers_handle() { + let recorder = Arc::new(Recorder::default()); + let mut h = History::builder().observer(Arc::clone(&recorder)).build(); + + h.record_winner(&"a", &"b", 1).unwrap(); + h.converge().unwrap(); + + assert!(!recorder.iterations.lock().unwrap().is_empty()); + assert!(!recorder.slices.lock().unwrap().is_empty()); + assert!(!recorder.converged.lock().unwrap().is_empty()); +} + +/// `?Sized` on the blanket impls means the observer can be chosen at runtime. +#[test] +fn a_trait_object_observer_works() { + let boxed: Box> = Box::new(Recorder::default()); + let mut h = History::builder().observer(boxed).build(); + h.record_winner(&"a", &"b", 1).unwrap(); + h.converge().unwrap(); + + let shared: Arc> = Arc::new(Recorder::default()); + let mut h = History::builder().observer(Arc::clone(&shared)).build(); + h.record_winner(&"a", &"b", 1).unwrap(); + h.converge().unwrap(); +} + +/// A non-shared observer can be reclaimed after convergence instead. +#[test] +fn into_observer_returns_the_accumulated_state() { + let mut h = History::builder().observer(Recorder::default()).build(); + h.record_winner(&"a", &"b", 1).unwrap(); + h.converge().unwrap(); + + // Readable in place... + assert!(!h.observer().iterations.lock().unwrap().is_empty()); + + // ...and reclaimable by value. + let recorder = h.into_observer(); + assert!(!recorder.slices.lock().unwrap().is_empty()); +} + +/// Borrowing works too, for an observer that outlives the history. +#[test] +fn a_borrowed_observer_works() { + let recorder = Recorder::default(); + { + let mut h = History::builder().observer(&recorder).build(); + h.record_winner(&"a", &"b", 1).unwrap(); + h.converge().unwrap(); + } + assert!(!recorder.iterations.lock().unwrap().is_empty()); +}