Better support for mocking methods like std::io::Read::read (#651)

std::io::Read::read is often called with an uninitialized buffer as its
argument.  So Mockall shouldn't try to read from that buffer, even
though the language rules allow it to.  Previously, Mockall would try to
read from it, in order to format a panic message in case no matching
Expectation was found.

Now, Mockall will not try to format such a panic message preemptively.
It will only do it if there actually isn't any matching Expectation.
That also provides a small performance boost to all users.

Also, do a better job of clearing poison from static methods' Mutexes
following a panic.  Previously we would only do that if the user had
created a Context object.  Now we will do it regardless.

Fixes #647

Author: asomers

CHANGELOG.md

@@ -12,8 +12,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
-- No longer poison a Context object's internal `Mutex` when panicing, even when
-  using a stable Rust compiler.
+- No longer poison a static mock method's internal `Mutex` when panicing, even when
+  using a stable Rust compiler.  Also, no longer poison it even if there is no
+  `Context` object.  For example, if the user never set an Expectation at all.
   ([#650](https://github.com/asomers/mockall/pull/650))
 
 - Suppress `single-use-lifetimes` lints in the generated code, for cases where

mockall/src/lib.rs

@@ -1691,7 +1691,7 @@ impl SeqHandle {
     }
 
     /// Verify that this handle was called in the correct order
-    pub fn verify(&self, desc: &str) {
+    pub fn verify<F: Fn() -> String>(&self, desc: F) {
         self.inner.verify(self.seq, desc);
     }
 }
@@ -1709,9 +1709,9 @@ impl SeqInner {
     }
 
     /// Verify that the call identified by `seq` was called in the correct order
-    fn verify(&self, seq: usize, desc: &str) {
+    fn verify<F: Fn() -> String>(&self, seq: usize, desc: F) {
         assert_eq!(seq, self.satisfaction_level.load(Ordering::Relaxed),
-            "{desc}: Method sequence violation")
+            "{}: Method sequence violation", &desc())
     }
 }
 

mockall/tests/clear_expectations_on_panic.rs

@@ -4,7 +4,7 @@
 //! https://github.com/asomers/mockall/issues/442
 #![deny(warnings)]
 
-use std::panic;
+use std::{panic, sync::Mutex};
 
 use mockall::*;
 
@@ -14,8 +14,12 @@ pub trait Foo {
     fn bar() -> i32;
 }
 
+static FOO_MTX: Mutex<()> = Mutex::new(());
+
 #[test]
 fn too_few_calls() {
+    let _m = FOO_MTX.lock().unwrap();
+
     panic::catch_unwind(|| {
         let ctx = MockFoo::foo_context();
         ctx.expect()
@@ -34,17 +38,29 @@ fn too_few_calls() {
 
 // We shouldn't panic during drop in this case.  Regression test for
 // https://github.com/asomers/mockall/issues/491
-#[cfg_attr(not(feature = "nightly"), ignore)]
 #[test]
 fn too_many_calls() {
+    let _m = FOO_MTX.lock().unwrap();
+
     panic::catch_unwind(|| {
         let ctx = MockFoo::bar_context();
         ctx.expect()
             .times(0);
         MockFoo::bar();
     }).unwrap_err();
 
-    // This line will panic with a PoisonError, at least until issue #515 is
-    // complete.  
+    let _ctx = MockFoo::bar_context();
+}
+
+// If we panic due to not setting any expectations at all, the mock method's
+// mutex shouldn't be poisoned.
+#[test]
+fn no_expectations_at_all() {
+    let _m = FOO_MTX.lock().unwrap();
+
+    panic::catch_unwind(|| {
+        MockFoo::bar();
+    }).unwrap_err();
+
     let _ctx = MockFoo::bar_context();
 }

mockall/tests/mock_read.rs

@@ -0,0 +1,106 @@
+// vim: tw=80
+//! Mock a trait like std::io::Read.  Mockall should not try to read the read
+//! method's arguments if the test succeeds.
+//! https://github.com/asomers/mockall/issues/647
+#![deny(warnings)]
+
+use std::{
+    fmt::{self, Debug, Formatter},
+    io
+};
+
+use mockall::*;
+
+pub struct MyBuf<'a>(&'a mut [u8]);
+impl<'a> Debug for MyBuf<'a> {
+    fn fmt(&self, _f: &mut Formatter<'_>) -> Result<(), fmt::Error> {
+        // An implementor of std::io::Read::read should not attempt to read from
+        // the provided buffer, because it might be uninitialized.  Here, we
+        // implement a panic in Debug::fmt, just to ensure that Mockall won't
+        // try to format the mock method's arguments.
+        panic!("Shouldn't attempt to read from me!");
+    }
+}
+
+// Similarly, this wrapper type ensures that Mockall won't try to format such an
+// object.
+#[derive(Clone, Copy, Eq, PartialEq)]
+pub struct NonDebuggable<T: 'static>(T);
+impl<T: 'static> Debug for NonDebuggable<T> {
+    fn fmt(&self, _f: &mut Formatter<'_>) -> Result<(), fmt::Error> {
+        panic!("Shouldn't attempt to debug me!");
+    }
+}
+
+trait Readlike {
+    fn read<'a>(&mut self, buf: MyBuf<'a>) -> io::Result<usize>;
+}
+
+mock! {
+    pub Foo {
+        /* Also test some methods with different Expectation types */
+        fn foo(&mut self, x: NonDebuggable<i32>) -> &usize;
+        fn bar(&mut self, x: NonDebuggable<i32>) -> &mut usize;
+        fn baz<T: 'static>(&mut self, x: NonDebuggable<T>) -> usize;
+    }
+    /* Just like std::io::Read, but with the MyBuf wrapper */
+    impl Readlike for Foo {
+        fn read<'a>(&mut self, buf: MyBuf<'a>) -> io::Result<usize>;
+    }
+}
+
+const HELLO: &[u8] = b"Hello, World!\0";
+
+#[test]
+fn like_read() {
+    let mut mock = MockFoo::new();
+    let mut seq = Sequence::new();
+    mock.expect_read()
+        .times(1)
+        .in_sequence(&mut seq)
+        .returning(|buf| {
+            buf.0[0..HELLO.len()].copy_from_slice(HELLO);
+            Ok(HELLO.len())
+        });
+
+    let mut inner_buf = [0; 80];
+    let my_buf = MyBuf(&mut inner_buf);
+    mock.read(my_buf).unwrap();
+    assert_eq!(&inner_buf[0..HELLO.len()], HELLO);
+}
+
+#[test]
+fn return_ref() {
+    let mut mock = MockFoo::new();
+    let mut seq = Sequence::new();
+    mock.expect_foo()
+        .times(1)
+        .in_sequence(&mut seq)
+        .return_const(42usize);
+
+    assert_eq!(*mock.foo(NonDebuggable(5i32)), 42);
+}
+
+#[test]
+fn return_refmut() {
+    let mut mock = MockFoo::new();
+    let mut seq = Sequence::new();
+    mock.expect_bar()
+        .times(1)
+        .in_sequence(&mut seq)
+        .return_var(42usize);
+
+    assert_eq!(*mock.bar(NonDebuggable(5i32)), 42);
+}
+
+#[test]
+fn generic() {
+    let mut mock = MockFoo::new();
+    let mut seq = Sequence::new();
+    mock.expect_baz::<u32>()
+        .times(1)
+        .in_sequence(&mut seq)
+        .return_const(42usize);
+
+    assert_eq!(mock.baz(NonDebuggable(5u32)), 42);
+}