Validate from_parts_unchecked inputs in debug builds#18
Closed
thinkingfish wants to merge 1 commit intomainfrom
Closed
Validate from_parts_unchecked inputs in debug builds#18thinkingfish wants to merge 1 commit intomainfrom
thinkingfish wants to merge 1 commit intomainfrom
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
debug_assert!(Self::validate(...).is_ok())insidefrom_parts_uncheckedon 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:
It was missed in the v1.3.1 landing. This PR closes the gap.
The motivation is consumer-facing safety: columnar consumers like
metriken-querybuild 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 adebug_assertis the right shape — devs/tests catch invariant violations immediately, production pays nothing.Changes
src/cumulative.rs:debug_assertin thefrom_parts_uncheckedmacro 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: samedebug_assert. Existing sparse tests remain unchanged.from_parts_uncheckedmethods to clarify the contract and the debug-build semantics.CHANGELOG.md: entry under[Unreleased].Cargo.toml: bump to1.3.2-alpha.0per the project's feature-PR versioning rule.Test plan
cargo test— 114 unit + 5 doctest, all pass (was 110 + 5; +4 new debug-onlyshould_panictests)cargo clippy --all-targets --all-features -- -D warningscleancargo fmt --checkcleancargo doc --no-depscleanOut of scope
from_parts_uncheckedasunsafe 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) reservesunsafefor the latter; staying with safepub fn+ debug-assertion validation matches that bar.CumulativeRoView). Considered in the plan as an alternative to the macro duplication; v1.3.1 went with macros and this PR keeps that.