Skip to content

Validate from_parts_unchecked inputs in debug builds#18

Closed
thinkingfish wants to merge 1 commit intomainfrom
feature/from-parts-unchecked-debug-assert
Closed

Validate from_parts_unchecked inputs in debug builds#18
thinkingfish wants to merge 1 commit intomainfrom
feature/from-parts-unchecked-debug-assert

Conversation

@thinkingfish
Copy link
Copy Markdown
Member

Summary

Adds debug_assert!(Self::validate(...).is_ok()) inside from_parts_unchecked on the four borrowed-view types (CumulativeROHistogramRef, CumulativeROHistogram32Ref, SparseHistogramRef, SparseHistogram32Ref).

Debug builds now panic at the call site when a caller violates the slice invariants. Release builds are unchanged — the assertion compiles away, so the hot-path zero-allocation property the borrowed views are designed for still holds.

Why

The slice-based quantile API design (the one v1.3.1 implemented) explicitly called for this:

The unsafe from_parts_unchecked constructor needs debug-assertion-only validation under cfg(debug_assertions).

It was missed in the v1.3.1 landing. This PR closes the gap.

The motivation is consumer-facing safety: columnar consumers like metriken-query build a fresh ref per snapshot per query (potentially hundreds of thousands of refs per percentile query). Misuse silently produces incorrect quantile output rather than memory unsafety, so a debug_assert is the right shape — devs/tests catch invariant violations immediately, production pays nothing.

Changes

  • src/cumulative.rs: debug_assert in the from_parts_unchecked macro body, plus four #[should_panic] tests under #[cfg(debug_assertions)] covering length mismatch, non-ascending indices, decreasing cumulative counts, and zero count (the last on the u32 variant).
  • src/sparse.rs: same debug_assert. Existing sparse tests remain unchanged.
  • Improved rustdoc on both from_parts_unchecked methods to clarify the contract and the debug-build semantics.
  • CHANGELOG.md: entry under [Unreleased].
  • Cargo.toml: bump to 1.3.2-alpha.0 per the project's feature-PR versioning rule.

Test plan

  • cargo test — 114 unit + 5 doctest, all pass (was 110 + 5; +4 new debug-only should_panic tests)
  • cargo clippy --all-targets --all-features -- -D warnings clean
  • cargo fmt --check clean
  • cargo doc --no-deps clean

Out of scope

  • Marking from_parts_unchecked as unsafe fn. The plan offers this as a conservative option but notes the misuse penalty is incorrect quantile output, not memory unsafety. Standard library precedent (e.g. str::from_utf8_unchecked) reserves unsafe for the latter; staying with safe pub fn + debug-assertion validation matches that bar.
  • Trait-based view abstraction (CumulativeRoView). Considered in the plan as an alternative to the macro duplication; v1.3.1 went with macros and this PR keeps that.

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.
@thinkingfish thinkingfish deleted the feature/from-parts-unchecked-debug-assert branch April 30, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant