Challenge 12: Verify safety of NonZero#565
Challenge 12: Verify safety of NonZero#565Samuelsills wants to merge 1 commit intomodel-checking:mainfrom
Conversation
938fdd1 to
32bbacc
Compare
Verify all 38 NonZero functions listed in the challenge specification. 432 Kani proof harnesses across all 12 integer types, 0 failures. Resolves model-checking#71 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
32bbacc to
71e54d0
Compare
Verification Coverage ReportPart 1:
|
| # | Function | Verified | # | Function | Verified |
|---|---|---|---|---|---|
| 1 | max |
✅ | 19 | saturating_add |
✅ |
| 2 | min |
✅ | 20 | unchecked_add |
✅ |
| 3 | clamp |
✅ | 21 | checked_next_power_of_two |
✅ |
| 4 | bitor (3 impls) |
✅ | 22 | midpoint |
✅ |
| 5 | count_ones |
✅ | 23 | isqrt |
✅ |
| 6 | rotate_left |
✅ | 24 | abs |
✅ |
| 7 | rotate_right |
✅ | 25 | checked_abs |
✅ |
| 8 | swap_bytes |
✅ | 26 | overflowing_abs |
✅ |
| 9 | reverse_bits |
✅ | 27 | saturating_abs |
✅ |
| 10 | from_be |
✅ | 28 | wrapping_abs |
✅ |
| 11 | from_le |
✅ | 29 | unsigned_abs |
✅ |
| 12 | to_be |
✅ | 30 | checked_neg |
✅ |
| 13 | to_le |
✅ | 31 | overflowing_neg |
✅ |
| 14 | checked_mul |
✅ | 32 | wrapping_neg |
✅ |
| 15 | saturating_mul |
✅ | 33 | from_mut |
✅ |
| 16 | unchecked_mul |
✅ | 34 | from_mut_unchecked |
✅ |
| 17 | checked_pow |
✅ | |||
| 18 | saturating_pow |
✅ | |||
| — | neg |
✅ | — | checked_add |
✅ |
Total: 38/38 functions verified (2 Part 1 + 36 Part 2)
UBs Checked
- ✅ Invoking UB via compiler intrinsics
- ✅ Reading from uninitialized memory
- ✅ Producing an invalid value
Verification Approach
- Tool: Kani Rust Verifier
- 432 proof harnesses across all 12 NonZero integer types
- Verified for all NonZero types (i8, u8, i16, u16, i32, u32, i64, u64, i128, u128, isize, usize)
There was a problem hiding this comment.
Pull request overview
Adds/expands Kani verification coverage for core::num::NonZero per Challenge 12, and adjusts checked_pow loop invariants in integer macros to enable proving NonZero-preservation properties.
Changes:
- Strengthens
#[safety::loop_invariant]annotations inchecked_powfor signed/unsigned integer macro implementations to maintain nonzero-related facts through the loop. - Adds a large set of Kani proof harnesses (macros + instantiations) covering the remaining Challenge 12
NonZeroAPIs across all integer types. - Documents the verification approach and harness matrix in the Challenge 12 markdown page.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| library/core/src/num/uint_macros.rs | Tightens checked_pow loop invariant for unsigned integers to support NonZero proofs. |
| library/core/src/num/int_macros.rs | Tightens checked_pow loop invariant for signed integers to support NonZero proofs. |
| library/core/src/num/nonzero.rs | Adds Kani proof harnesses validating NonZero safety across bitwise, conversion, arithmetic, abs, and negation APIs. |
| doc/src/challenges/0012-nonzero.md | Adds a verification summary documenting harness coverage and approach for Challenge 12. |
feliperodri
left a comment
There was a problem hiding this comment.
Overall, the PR is a solid first submission and covers all required functions. The main concern for acceptance is that most Part 2 harnesses are thin proof-of-non-UB rather than contract-level specifications, which the challenge spirit and Part 1 criterion 2b suggest should be stronger.
| let x: $nonzero_type = kani::any(); | ||
| let y: $nonzero_type = kani::any(); | ||
| let result = x | y; | ||
| let _ = result.get(); |
There was a problem hiding this comment.
Most harnesses do little more than call the function and call .get() on the result. This proves the result is non-zero (UB-free), but does not verify correctness or any semantic contract. The challenge says:
"write and verify contracts specifying the following..."
Examples of thin harnesses:
swap_bytes,reverse_bits,from_be,from_le,to_be,to_le— only dolet _ = result.get(). There is no assertion that the transformation is correct (e.g.,from_be(to_be(x)) == x).rotate_left/rotate_right— does assert the inverse roundtrip (result.rotate_right(n) == x), which is good.bitorharnesses (all 3) — only call.get(), with no assertion that the result equalsx | y.count_ones— assertsresult.get() > 0, which is the only meaningful property here.- Arithmetic ops like
checked_mul,saturating_mul— only verify no UB/non-zero result, but don't assert any arithmetic property.
The challenge's Part 1 criterion 2b explicitly requires asserting the output value equals expected results. Applying this spirit to Part 2 means harnesses should verify semantic correctness, not just non-zero-ness.
| // --- new (Part 1: iff assertion) --- | ||
| macro_rules! nonzero_check_new { | ||
| ($t:ty, $nonzero_type:ty, $harness_name:ident) => { | ||
| #[kani::proof] |
There was a problem hiding this comment.
The challenge says:
"write and verify contracts specifying..."
The new and from_mut harnesses use plain #[kani::proof] with inline assertions rather than #[requires]/#[ensures] contracts on the function itself. This means no contract is attached to the source function; only the harness is verified. This is a weaker form of verification since the contracts aren't reusable/compositional. Consider using #[kani::proof_for_contract] consistently.
| let mut acc: Self = 1; | ||
|
|
||
| #[safety::loop_invariant(true)] | ||
| #[safety::loop_invariant(self == 0 || (acc != 0 && base != 0))] |
There was a problem hiding this comment.
For signed integers, acc != 0 && base != 0 doesn't actually establish that the loop will produce a non-zero result, e.g., for self = -1, checked_pow(-1, exp) alternates between -1 and 1, both non-zero, so this is fine. But the invariant uses self == 0 as an escape hatch, which is actually unreachable (since self is NonZero). The condition self == 0 is dead for inputs from a NonZero context. This is harmless but logically misleading. A comment would help.
For the unsigned case in uint_macros.rs:
#[safety::loop_invariant(self == 0 || (acc > 0 && base > 0))]Same issue. self == 0 is dead code in this context. The invariant is valid but could be tightened to just acc > 0 && base > 0 with a precondition self > 0, right?
| macro_rules! nonzero_check_isqrt { | ||
| ($nonzero_type:ty, $harness_name:ident) => { | ||
| #[kani::proof] | ||
| #[kani::unwind(65)] |
There was a problem hiding this comment.
The value 65 is unexplained. This controls how many times loops are unrolled for verification. If the unwind bound is too small, Kani may miss behaviors (incomplete verification). The PR should explain why 65 is sufficient (e.g., tied to u128 bit-width). Ideally there's a comment like // 64 iterations max for u128 isqrt loop + 1. Most importantly, why do we even need this bound?
| let x: $nonzero_type = kani::any(); | ||
| let y: $nonzero_type = kani::any(); | ||
| let result = x.saturating_mul(y); | ||
| let _ = result.get(); |
There was a problem hiding this comment.
There is no assertion that result == NonZero::MIN or NonZero::MAX when overflow occurs. This is a correctness gap. It only verifies the result is non-zero (which follows from the NonZero invariant on saturating ops), but not that saturation happened at the correct boundary.
|
|
||
| // --- abs --- | ||
| // abs() uses #[rustc_inherit_overflow_checks] which panics on MIN in debug. | ||
| // The MIN case is verified separately by overflowing_abs/wrapping_abs harnesses. |
There was a problem hiding this comment.
But wrapping_neg of i8::MIN returns i8::MIN (i.e., -128), which is still non-zero, so the NonZero invariant holds. However, you do not explicitly assert that wrapping_neg(MIN) == MIN or that the overflowed result is correct. The harness only does let _ = result.get(). If the goal is to "separately verify" the MIN case's behavior, the assertion should be made explicit.
Summary
Verify all 38 NonZero functions listed in Challenge 12. 432 Kani proof harnesses across all 12 integer types, 0 failures.
Part 1: Harnesses for
new(iff + value equality assertions) andfrom_mut(iff + dereference). Pre-existing contracts and harnesses fornew_uncheckedandfrom_mut_unchecked.Part 2: Harnesses for all 34 listed functions including bitor (3 impls), count_ones, rotate_left/right, swap_bytes, reverse_bits, endianness conversions, checked/saturating mul/pow/add, checked_next_power_of_two, midpoint, isqrt, abs variants (6), and neg variants (4).
Strengthened loop invariant on
checked_powinuint_macros.rsandint_macros.rsfromtrueto a property that preserves the nonzero invariant through loop iterations. This is a verification-only annotation (no-op at runtime).absandnegharnesses excludeT::MIN(documented overflow behavior). The MIN case is separately verified bywrapping_abs,overflowing_abs,wrapping_neg, andoverflowing_negwhich pass for all inputs.Resolves #71
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.