Mockall will format function arguments too greedily

[asomers]

If a mock method's expectations aren't satisfied, Mockall will print an error message that, if possible, includes every method argument formatted with Debug::fmt. That's been the case with stable rust ever since #425 . But with nightly rust, when using Mockall's nightly feature, it's been the case for much longer.
The problem is that Mockall actually formats the error message before checking if the mock method call would fail, in MockFunction::desc¹. That has two problems:

1. It wastes CPU cycles, potentially lots of them if Debug::fmt is expensive on that object.
2. It calls Debug::fmt on objects where that shouldn't be allowed. That's a problem for implementers of std::io::Read::read, which by contractgentlemen's agreement aren't supposed to read data from their arguments. If the argument is an uninitialized buffer (a common case), then formatting that argument with Debug::fmt could even technically be UB. And Miri will complain about it.

We should fix this problem by changing MockFunction::desc to return a closure which will produce a format string, rather than the format string itself. And ripple that through the stack accordingly.

Footnotes

1. https://github.com/asomers/mockall/blob/aeb460c5aca3f8f38b855cd459d718af265b32fb/mockall_derive/src/mock_function.rs#L632 ↩

[bjorn3]

It calls Debug::fmt on objects where that shouldn't be allowed. That's a problem for implementers of std::io::Read::read, which by contract aren't supposed to read data from their arguments. If the argument is an uninitialized buffer (a common case), then formatting that argument with Debug::fmt could technically be UB. And Miri will complain about it.

It is not unsound for implementers of Read::read to read data from the output buffer.

Implementations of this method can make no assumptions about the contents of buf when this function is called. It is recommended that implementations only write data to buf instead of reading its contents.

Is only a recommendation, not a safety invariant. The unstable read_buf api exists to allow uninitialized buffers precisely because implementors of Read::read can safely read from the output buffer.

[asomers]

Hearing that gives me a good feeling, @bjorn3 . It allows me to mentally blame somebody else. And yet, I think I still need to fix the problem anyway. It would be a performance improvement, if nothing else.

[bjorn3]

Yeah, it may be a correctness and/or performance issue to read from the output buffer of Read::read, but is definitively not a soundness issue.