Best practices around verification

[tkaitchuck]

In the readme it notes:

Panic detection happens at link time across the entire dependency graph, so any Cargo commands that do not invoke a linker will not trigger panic detection. This includes cargo build of library crates and cargo check of binary and library crates.

This can be frustrating when developing a library as running the linker is not part of normal work flow.

I've found a good way to ensure this does not get overlooked. A good way to do this is rather than annotating the actual code, creating a wrapper function that is annotated with:

#[inline(never)]
#[no_panic]
#[cfg(test)]

And then have a test that calls it passing whatever parameters are needed. The inline(never) should prevent any constants from the test from getting inlined, which might not cover the panicking code path. The build will then fail any time cargo test is run.

It might be good to show an example working this way using a test rather than a main() because it is easy to not end up running the linker when developing a library.

[dtolnay]

Thanks! I would accept a PR to add a best practices section that shows how to test libraries.

The inline(never) should prevent any constants from the test from getting inlined, which might not cover the panicking code path.

Hmm, I am not sure that's true. For example in this test, the compiler inlines the knowledge that index 1 is a char boundary from the string literal in the test, even though the function can panic on other inputs. This compiles if I run cargo test --release.

#[cfg(test)]
use no_panic::no_panic;

pub fn demo(s: &str) -> &str {
    &s[1..]
}

#[inline(never)]
#[no_panic]
#[cfg(test)]
fn entrypoint(s: &str) -> &str {
    demo(s)
}

#[test]
fn no_panic() {
    let _ = entrypoint("aaa");
}

We may need a black_box sort of thing to use as the arguments.

[tkaitchuck]

@dtolnay Good point. In testing I found that if I modified your example to have two layers of wrapping methods each annotated with #[inline(never)] that problem goes away. (Perhaps the issue was not the method being inlined but rather the demo method being inlined into entrypoint.

This is of course adding a lot of extra typing for the user. I played around with making this into a macro:

use no_panic::no_panic;

fn main() {
    println!("Hello, world!");
}

pub fn demo(s: &str) -> &str {
    &s[1..]
}

macro_rules! test_no_panic {
    ( $function:ident ($type:ty = $example:expr ) ) => { 
        #[cfg(test)]
        #[test]
        #[inline(never)]
        pub fn test_panic() {

            #[inline(never)]
            #[no_panic]
            fn inner_wrapper(a: $type) {
                $function(a);
            }

            #[inline(never)]
            #[no_panic]
            fn outer_wrapper(a: $type) {
                inner_wrapper(a)
            }

            outer_wrapper($example);
        }
    };
}

test_no_panic!(demo(&str = "123"));

That could be generalized to have multiple parameters fairly easily. The unfortunate thing is having to type the types. That could be fixed if it were made into a proc-macro that annotated the actual implementation method instead of a function style macro.

[dragazo]

I've been testing this crate in some toy examples, and I think I have a decent solution to the problem. We would use the following no-panic unit test and run with cargo test.

#[no_panic]
pub fn foo(a: &[u8], b: usize) -> u8 {
    a[b]
}

#[test]
fn test_foo() {
    use core::hint::black_box;
    let _ = black_box(foo(black_box(&[1, 2, 3]), black_box(1)));
}

AFAIK cargo test must always invoke the linker to build the test binary, so this should work in both binaries and libraries. I noticed that if a function is not invoked, it gets totally optimized out before linking, so we need at least one invocation of every #[no_panic] function, which is most convenient to do in a dedicated unit test. On top of this, I saw that #[inline(never)] still has local information leaking (as mentioned in this thread). The best I've found to circumvent that is to wrap all parameters in core::hint::black_box, and for good measure also wrap the return value and bind it to _ for any #[must_use] functions/types. I think with that, it should work in pretty much every use case.