Check every public routine for all inputs with no_panic

.github/workflows/ci.yml

@@ -27,6 +27,19 @@ jobs:
       - uses: actions/checkout@v4
       - uses: dtolnay/rust-toolchain@stable
       - run: cargo test --profile no-panic --features no-panic --test no_panic
+      # the check is only meaningful if it still detects a known panic
+      - name: canary must fail to link
+        run: |
+          log=$(mktemp)
+          if cargo test --profile no-panic --features no-panic --test no_panic_canary > "$log" 2>&1; then
+            echo "::error::the no_panic canary linked, so the check is no longer detecting panics"
+            exit 1
+          fi
+          if ! grep -q 'detected panic in function `canary`' "$log"; then
+            cat "$log"
+            echo "::error::the no_panic canary failed for an unexpected reason"
+            exit 1
+          fi
 
   msrv:
     name: MSRV (1.77)

CHANGELOG.md

@@ -1,7 +1,7 @@
 ### 0.5.1 ###
 * :bug: `ReadCursor::transaction` closures can now return data borrowed from the input. See [#12](https://github.com/stepfunc/scursor/issues/12).
 * :bug: `WriteCursor::at_pos` now accepts `FnOnce` closures. See [#13](https://github.com/stepfunc/scursor/issues/13).
-* :wrench: Replaced the Kani proofs with link-time `no_panic` checks on every public routine, run in CI.
+* :wrench: Replaced the Kani proofs with link-time `no_panic` checks, run in CI, covering every public routine for all inputs. A test fails if a new routine is left out of the check. A canary test confirms the check still detects panics.
 * :wrench: Added round-trip property tests and a CI workflow.
 * Declared the minimum supported Rust version (1.77).
 

Cargo.toml

@@ -29,6 +29,7 @@ no-panic = { version = "0.1", optional = true }
 
 [dev-dependencies]
 proptest = "1"
+syn = { version = "2", features = ["full", "visit"] }
 
 [features]
 # CI-only: fails to link if any public routine can panic. Requires an optimized
@@ -39,6 +40,11 @@ no-panic = ["dep:no-panic"]
 name = "no_panic"
 required-features = ["no-panic"]
 
+# must fail to link; CI checks that it does
+[[test]]
+name = "no_panic_canary"
+required-features = ["no-panic"]
+
 [profile.no-panic]
 inherits = "release"
 overflow-checks = true

README.md

@@ -71,13 +71,23 @@ cargo test --profile no-panic --features no-panic --test no_panic
 
 What the check does and doesn't tell you:
 
-* It covers every input, not a sample: it checks that no panic path survives in the compiled code.
+* Every public method is checked, for every input. `tests/no_panic.rs` takes the address of each
+  one, which forces the compiler to keep a standalone copy that has to handle any arguments. That
+  copy is what the linker checks. `tests/no_panic_coverage.rs` runs with the normal tests and fails
+  if a public method is missing the annotation or isn't listed in `tests/no_panic.rs`.
+* CI also links `tests/no_panic_canary.rs`, which contains a function that can panic, and
+  requires that link to fail. If a toolchain change ever stopped `no_panic` from detecting
+  panics, CI would fail rather than pass silently.
 * It applies to the configuration that was checked: an optimized build (`opt-level = 3`) with
   `overflow-checks = true` and `panic = "unwind"`. Debug builds and size-optimized builds keep
   some unreachable panic paths, so the check reports false failures there. Those builds are
   still panic-free; they just can't be checked this way.
-* The linker only examines code that is called, so `tests/no_panic.rs` calls every public routine.
-  `tests/no_panic_coverage.rs` fails if a routine is missing the annotation or the call.
+* Outside the check:
+  * Callbacks passed to `transaction` and `at_pos`. Those routines add no panics of their own,
+    but a panic in your callback still propagates.
+  * Derived trait impls (`Clone`, `Copy`, `Debug`, `PartialEq`, `Eq`), which can't be annotated.
+    Derived `Debug` writes to the formatter you supply.
+  * `read_array` is checked for a few sizes (0, 3 and 32). Every size runs the same code.
 * The `no-panic` feature exists for this crate's CI. Don't enable it in your own builds.
 
 ## License

tests/no_panic.rs

@@ -1,116 +1,128 @@
 //! Link-time panic check for every public routine.
 //!
 //! Every public function is annotated with `#[no_panic]` when the `no-panic`
-//! feature is enabled. The check happens when this test binary is linked, so
-//! each routine must be called here. `tests/no_panic_coverage.rs` verifies that
-//! this file stays in sync with the public API.
+//! feature is enabled. The check happens when this test binary is linked, and
+//! the linker only examines code that is linked, so this file takes the address
+//! of every public routine. Passing the address through `black_box` forces a
+//! standalone copy of each routine that knows nothing about its arguments, so
+//! the check covers every input rather than particular values.
+//!
+//! `tests/no_panic_coverage.rs` verifies that every public routine is listed.
 //!
 //! Run with:
 //!
 //! ```text
 //! cargo test --profile no-panic --features no-panic --test no_panic
 //! ```
 
-use scursor::{ReadCursor, WriteCursor};
+use scursor::{ReadCursor, ReadError, WriteCursor, WriteError};
 use std::hint::black_box;
 
-#[inline(never)]
-fn exercise_read_cursor(input: &[u8], count: usize) {
-    let mut cursor = ReadCursor::new(input);
-
-    black_box(cursor.read_u8().ok());
-    black_box(cursor.read_bytes(count).ok());
-    black_box(cursor.read_array::<0>().ok());
-    black_box(cursor.read_array::<3>().ok());
-    black_box(cursor.read_array::<32>().ok());
+/// Callback for the `transaction` and `at_pos` instances. Those routines are
+/// panic-free only if the caller's callback is, so they are checked with a
+/// callback that is.
+fn read_callback(cursor: &mut ReadCursor<'_>) -> Result<u8, ReadError> {
+    cursor.read_u8()
+}
 
-    black_box(cursor.read_u16_le().ok());
-    black_box(cursor.read_i16_le().ok());
-    black_box(cursor.read_u32_le().ok());
-    black_box(cursor.read_i32_le().ok());
-    black_box(cursor.read_u48_le().ok());
-    black_box(cursor.read_u64_le().ok());
-    black_box(cursor.read_i64_le().ok());
-    black_box(cursor.read_u128_le().ok());
-    black_box(cursor.read_i128_le().ok());
-    black_box(cursor.read_f32_le().ok());
-    black_box(cursor.read_f64_le().ok());
+fn write_callback(cursor: &mut WriteCursor<'_>) -> Result<(), WriteError> {
+    cursor.write_u8(0)
+}
 
-    black_box(cursor.read_u16_be().ok());
-    black_box(cursor.read_i16_be().ok());
-    black_box(cursor.read_u32_be().ok());
-    black_box(cursor.read_i32_be().ok());
-    black_box(cursor.read_u48_be().ok());
-    black_box(cursor.read_u64_be().ok());
-    black_box(cursor.read_i64_be().ok());
-    black_box(cursor.read_u128_be().ok());
-    black_box(cursor.read_i128_be().ok());
-    black_box(cursor.read_f32_be().ok());
-    black_box(cursor.read_f64_be().ok());
+fn read_transaction<'a, T>(_: T) -> fn(&mut ReadCursor<'a>, T) -> Result<u8, ReadError>
+where
+    T: FnOnce(&mut ReadCursor<'a>) -> Result<u8, ReadError>,
+{
+    ReadCursor::transaction
+}
 
-    black_box(cursor.transaction(|cur| cur.read_bytes(count)).ok());
-    black_box(cursor.expect_empty().ok());
-    black_box(cursor.remaining());
-    black_box(cursor.position());
-    black_box(cursor.is_empty());
-    black_box(cursor.read_all());
+fn write_transaction<'a, T>(_: T) -> fn(&mut WriteCursor<'a>, T) -> Result<(), WriteError>
+where
+    T: FnOnce(&mut WriteCursor<'_>) -> Result<(), WriteError>,
+{
+    WriteCursor::transaction
 }
 
-#[inline(never)]
-fn exercise_write_cursor(dest: &mut [u8], value: u128, pos: usize) {
-    let mut cursor = WriteCursor::new(dest);
+fn write_at_pos<'a, T>(_: T) -> fn(&mut WriteCursor<'a>, usize, T) -> Result<(), WriteError>
+where
+    T: FnOnce(&mut WriteCursor<'_>) -> Result<(), WriteError>,
+{
+    WriteCursor::at_pos
+}
 
-    black_box(cursor.write_u8(value as u8).ok());
-    black_box(cursor.write_bytes(black_box(&[0xCA, 0xFE])).ok());
-    black_box(cursor.skip(pos).ok());
-    black_box(cursor.seek_to(pos).ok());
+#[test]
+fn public_api_links_without_panic_paths() {
+    black_box::<fn(_) -> _>(ReadCursor::new);
+    black_box::<fn(_) -> _>(ReadCursor::read_u8);
+    black_box::<fn(_) -> _>(ReadCursor::expect_empty);
+    black_box::<fn(_) -> _>(ReadCursor::remaining);
+    black_box::<fn(_) -> _>(ReadCursor::position);
+    black_box(read_transaction(read_callback));
+    black_box::<fn(_) -> _>(ReadCursor::is_empty);
+    black_box::<fn(_) -> _>(ReadCursor::read_all);
+    black_box::<fn(_, _) -> _>(ReadCursor::read_bytes);
+    black_box::<fn(_) -> _>(ReadCursor::read_array::<0>);
+    black_box::<fn(_) -> _>(ReadCursor::read_array::<3>);
+    black_box::<fn(_) -> _>(ReadCursor::read_array::<32>);
 
-    black_box(cursor.write_u16_le(value as u16).ok());
-    black_box(cursor.write_i16_le(value as i16).ok());
-    black_box(cursor.write_u32_le(value as u32).ok());
-    black_box(cursor.write_i32_le(value as i32).ok());
-    black_box(cursor.write_u48_le(value as u64).ok());
-    black_box(cursor.write_u64_le(value as u64).ok());
-    black_box(cursor.write_i64_le(value as i64).ok());
-    black_box(cursor.write_u128_le(value).ok());
-    black_box(cursor.write_i128_le(value as i128).ok());
-    black_box(cursor.write_f32_le(value as f32).ok());
-    black_box(cursor.write_f64_le(value as f64).ok());
+    black_box::<fn(_) -> _>(ReadCursor::read_u16_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_i16_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_u32_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_i32_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_u48_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_u64_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_i64_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_u128_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_i128_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_f32_le);
+    black_box::<fn(_) -> _>(ReadCursor::read_f64_le);
 
-    black_box(cursor.write_u16_be(value as u16).ok());
-    black_box(cursor.write_i16_be(value as i16).ok());
-    black_box(cursor.write_u32_be(value as u32).ok());
-    black_box(cursor.write_i32_be(value as i32).ok());
-    black_box(cursor.write_u48_be(value as u64).ok());
-    black_box(cursor.write_u64_be(value as u64).ok());
-    black_box(cursor.write_i64_be(value as i64).ok());
-    black_box(cursor.write_u128_be(value).ok());
-    black_box(cursor.write_i128_be(value as i128).ok());
-    black_box(cursor.write_f32_be(value as f32).ok());
-    black_box(cursor.write_f64_be(value as f64).ok());
+    black_box::<fn(_) -> _>(ReadCursor::read_u16_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_i16_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_u32_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_i32_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_u48_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_u64_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_i64_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_u128_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_i128_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_f32_be);
+    black_box::<fn(_) -> _>(ReadCursor::read_f64_be);
 
-    black_box(
-        cursor
-            .transaction(|cur| cur.write_u16_le(value as u16))
-            .ok(),
-    );
-    black_box(
-        cursor
-            .at_pos(pos, |cur| cur.write_u32_be(value as u32))
-            .ok(),
-    );
-    black_box(cursor.written_since(pos).map(<[u8]>::len).ok());
-    black_box(cursor.get(pos..pos.wrapping_add(4)).map(<[u8]>::len));
-    black_box(cursor.written().len());
-    black_box(cursor.remaining());
-    black_box(cursor.position());
-}
+    black_box::<fn(_) -> _>(WriteCursor::new);
+    black_box::<fn(_) -> _>(WriteCursor::position);
+    black_box::<fn(_, _) -> _>(WriteCursor::get);
+    black_box::<fn(_, _) -> _>(WriteCursor::skip);
+    black_box::<fn(_, _) -> _>(WriteCursor::seek_to);
+    black_box(write_transaction(write_callback));
+    black_box(write_at_pos(write_callback));
+    black_box::<fn(_) -> _>(WriteCursor::written);
+    black_box::<fn(_, _) -> _>(WriteCursor::written_since);
+    black_box::<fn(_) -> _>(WriteCursor::remaining);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_bytes);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u8);
 
-#[test]
-fn public_api_links_without_panic_paths() {
-    let input: Vec<u8> = (0..=255).collect();
-    exercise_read_cursor(black_box(&input), black_box(7));
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u16_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i16_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u32_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i32_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u48_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u64_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i64_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_f32_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_f64_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u128_le);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i128_le);
 
-    let mut dest = [0u8; 256];
-    exercise_write_cursor(black_box(&mut dest), black_box(u128::MAX), black_box(5));
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u16_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i16_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u32_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i32_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u48_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u64_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i64_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_u128_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_i128_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_f32_be);
+    black_box::<fn(_, _) -> _>(WriteCursor::write_f64_be);
 }

tests/no_panic_canary.rs

@@ -0,0 +1,23 @@
+//! Canary for the link-time panic check: this test must fail to link.
+//!
+//! `canary` panics when its argument is zero. If the `no_panic` check is
+//! working, linking this test fails with "detected panic in function `canary`",
+//! and CI requires that failure. If this test ever links, the check has stopped
+//! detecting panics, and a passing `tests/no_panic.rs` would mean nothing.
+//!
+//! ```text
+//! cargo test --profile no-panic --features no-panic --test no_panic_canary
+//! ```
+
+use no_panic::no_panic;
+use std::hint::black_box;
+
+#[no_panic]
+fn canary(divisor: u8) -> u8 {
+    100 / divisor
+}
+
+#[test]
+fn canary_must_not_link() {
+    black_box::<fn(_) -> _>(canary);
+}