`SignedShrinker` calls `abs()` on `i*::MIN_VALUE`, causing panic

[jhpratt]

Consider the following snippet, using quickcheck 1.0.1.

quickcheck::quickcheck! {
    fn foo_can_shrink(v: i8) -> quickcheck::TestResult {
        let _ = quickcheck::Arbitrary::shrink(&dbg!(v));
        quickcheck::TestResult::passed()
    }
}

The test is trivial: it just ensures that some future value is smaller than the current.

When running cargo test on the snippet, I get (among other stuff)

[demo.rs:3] v = -128
thread 'foo_can_shrink' panicked at 'attempt to negate with overflow', /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/mod.rs:98:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:50:5
   3: core::num::<impl i8>::abs
             at /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/int_macros.rs:1905:21
   4: <i8 as quickcheck::arbitrary::Arbitrary>::shrink::shrinker::SignedShrinker::new
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/arbitrary.rs:817:40
   5: <i8 as quickcheck::arbitrary::Arbitrary>::shrink
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/arbitrary.rs:860:21
   6: demo::foo_can_shrink::prop
             at ./demo.rs:3:17
   7: <fn(A) .> T as quickcheck::tester::Testable>::result::{{closure}}
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/tester.rs:371:35
   8: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:322:9
   9: std::panicking::try::do_call
             at /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:381:40
  10: __rust_try
  11: std::panicking::try
             at /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:345:19
  12: std::panic::catch_unwind
             at /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:396:14
  13: quickcheck::tester::safe
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/tester.rs:402:5
  14: <fn(A) .> T as quickcheck::tester::Testable>::result
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/tester.rs:371:21
  15: quickcheck::tester::QuickCheck::quicktest
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/tester.rs:121:19
  16: quickcheck::tester::QuickCheck::quickcheck
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/tester.rs:163:36
  17: quickcheck::tester::quickcheck
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/tester.rs:183:5
  18: demo::foo_can_shrink
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/lib.rs:62:21
  19: demo::foo_can_shrink::{{closure}}
             at /home/jhpratt/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-1.0.1/src/lib.rs:58:17
  20: core::ops::function::FnOnce::call_once
             at /home/jhpratt/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
  21: core::ops::function::FnOnce::call_once
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The panic ultimately traces back to this line, where (-128).abs() is the root cause. I'm not sure what the intended behavior is here, but a panic is obviously wrong.

I'm also not sure why the code is run twice, but I don't think that should be the case either.

[BurntSushi]

Nice find! Thanks for the report. This should be fixed in quickcheck 1.0.2 on crates.io.

[jhpratt]

Awesome, I'll give it a try. Out of curiosity, did the strategy for choosing the numbers change? I did a git blame and saw that that specific line has existed since 2015.

[BurntSushi]

Yeah, it did. See: #240

The full range of integers is now generated.

[jhpratt]

Looks like line 827 of the same file has the same issue. Performing a similar patch locally avoided this bad behavior.

diff --git a/src/arbitrary.rs b/src/arbitrary.rs
index 13b5b65..bdd0721 100644
--- a/src/arbitrary.rs
+++ b/src/arbitrary.rs
@@ -824,7 +824,7 @@ macro_rules! signed_shrinker {
             impl Iterator for SignedShrinker {
                 type Item = $ty;
                 fn next(&mut self) -> Option<$ty> {
-                    if (self.x - self.i).abs() < self.x.abs() {
+                    if self.x == <$ty>::MIN || (self.x - self.i).abs() < self.x.abs() {
                         let result = Some(self.x - self.i);
                         self.i = self.i / 2;
                         result

So I guess a test that actually uses the value is probably a good idea too 😆