From 9c29b41121a321aa75135edd586fb8d3edcbc3d0 Mon Sep 17 00:00:00 2001 From: Yao Yue Date: Thu, 30 Apr 2026 02:29:22 -0700 Subject: [PATCH] validate from_parts_unchecked inputs in debug builds Adds debug_assert!(Self::validate(...).is_ok()) inside from_parts_unchecked on CumulativeROHistogram*Ref and SparseHistogram*Ref. Debug builds get a safety net that catches invariant violations at the call site; release builds remain allocation-free since debug_assert is elided. This addresses the testing requirement called out in the borrowed-view slice API design document: hot-path consumers (e.g. metriken-query's columnar histogram storage) construct a fresh ref per snapshot and benefit from the dev-time check without paying for it in production. Bumps to 1.3.2-alpha.0 per the project's feature-PR versioning rule. --- CHANGELOG.md | 8 +++++++ Cargo.toml | 2 +- src/cumulative.rs | 56 +++++++++++++++++++++++++++++++++++++++++++++-- src/sparse.rs | 19 ++++++++++++++-- 4 files changed, 80 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f679f6..d1697d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `from_parts_unchecked` on `CumulativeROHistogramRef` / + `CumulativeROHistogram32Ref` / `SparseHistogramRef` / + `SparseHistogram32Ref` now runs the same validation as `from_parts` + inside a `debug_assert!`. Debug builds catch invariant violations at + the call site; release builds are unchanged (validation is elided). + ## [1.3.1] - 2026-04-29 ### Added diff --git a/Cargo.toml b/Cargo.toml index c0cc121..2f87638 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "histogram" -version = "1.3.1" +version = "1.3.2-alpha.0" edition = "2024" authors = ["Brian Martin ", "Yao Yue "] license = "MIT OR Apache-2.0" diff --git a/src/cumulative.rs b/src/cumulative.rs index aff9b8f..dc63ceb 100644 --- a/src/cumulative.rs +++ b/src/cumulative.rs @@ -360,15 +360,30 @@ macro_rules! define_cumulative_histogram { /// Creates a borrowed view without validating invariants. /// - /// # Safety + /// Skips the O(n) validation pass that `from_parts` performs. + /// Intended for hot paths where the caller already guarantees the + /// invariants (e.g. data produced by this crate or validated once + /// at load time). + /// + /// # Contract /// /// Caller must ensure `index` and `count` satisfy the same invariants - /// as the owned `from_parts`. + /// as the owned `from_parts`. Misuse produces incorrect quantile + /// output rather than memory unsafety, which is why this is a safe + /// `fn`. In debug builds, the invariants are checked via + /// `debug_assert!`; release builds skip the check entirely. pub fn from_parts_unchecked( config: Config, index: &'a [u32], count: &'a [$count], ) -> Self { + debug_assert!( + Self::validate(&config, index, count).is_ok(), + concat!( + stringify!($ref_name), + "::from_parts_unchecked called with invalid inputs" + ), + ); Self { config, index, @@ -1130,4 +1145,41 @@ mod tests { SampleQuantiles::quantiles(&owned, qs).unwrap() ); } + + // Debug-only safety net: `from_parts_unchecked` should panic in debug + // builds when handed inputs that violate the invariants. Release builds + // skip the check entirely (no panic), so these tests only run when + // debug assertions are enabled. + + #[cfg(debug_assertions)] + #[test] + #[should_panic(expected = "from_parts_unchecked called with invalid inputs")] + fn ref_from_parts_unchecked_debug_panics_on_length_mismatch() { + let config = Config::new(7, 32).unwrap(); + let _ = CumulativeROHistogramRef::from_parts_unchecked(config, &[1, 3], &[10]); + } + + #[cfg(debug_assertions)] + #[test] + #[should_panic(expected = "from_parts_unchecked called with invalid inputs")] + fn ref_from_parts_unchecked_debug_panics_on_non_ascending_indices() { + let config = Config::new(7, 32).unwrap(); + let _ = CumulativeROHistogramRef::from_parts_unchecked(config, &[3, 1], &[10u64, 40]); + } + + #[cfg(debug_assertions)] + #[test] + #[should_panic(expected = "from_parts_unchecked called with invalid inputs")] + fn ref_from_parts_unchecked_debug_panics_on_decreasing_counts() { + let config = Config::new(7, 32).unwrap(); + let _ = CumulativeROHistogramRef::from_parts_unchecked(config, &[1, 3], &[40u64, 10]); + } + + #[cfg(debug_assertions)] + #[test] + #[should_panic(expected = "from_parts_unchecked called with invalid inputs")] + fn ref32_from_parts_unchecked_debug_panics_on_zero_count() { + let config = Config::new(7, 32).unwrap(); + let _ = CumulativeROHistogram32Ref::from_parts_unchecked(config, &[1], &[0u32]); + } } diff --git a/src/sparse.rs b/src/sparse.rs index b3c1a77..93646d8 100644 --- a/src/sparse.rs +++ b/src/sparse.rs @@ -466,15 +466,30 @@ macro_rules! define_sparse_histogram { /// Creates a borrowed view without validating invariants. /// - /// # Safety + /// Skips the O(n) validation pass that `from_parts` performs. + /// Intended for hot paths where the caller already guarantees the + /// invariants (e.g. data produced by this crate or validated once + /// at load time). + /// + /// # Contract /// /// Caller must ensure `index` and `count` satisfy the same invariants - /// as the owned `from_parts`. + /// as the owned `from_parts`. Misuse produces incorrect quantile + /// output rather than memory unsafety, which is why this is a safe + /// `fn`. In debug builds, the invariants are checked via + /// `debug_assert!`; release builds skip the check entirely. pub fn from_parts_unchecked( config: Config, index: &'a [u32], count: &'a [$count], ) -> Self { + debug_assert!( + Self::validate(&config, index, count).is_ok(), + concat!( + stringify!($ref_name), + "::from_parts_unchecked called with invalid inputs" + ), + ); Self { config, index,