asomers / mockall

A powerful mock object library for Rust
Apache License 2.0
1.5k stars 62 forks source link

can we use &dyn to circumvent: `error[E0562]: impl Trait only allowed in function and inherent method return types, not in Fn trait param` #498

Open corecode opened 1 year ago

corecode commented 1 year ago

My code uses &impl Trait function arguments, but automock struggles with it (see example code below). If I use generics, mockall forces me to add + 'static, and I don't know how that interacts with my other code.

However, I can use custom &dyn Trait mocks that seem to work. Is this a reasonable approach?

Example

use mockall::{automock, mock};

#[automock]
trait Foo {}

struct Bar;

/*
cannot use #[automock]:

error[E0562]: `impl Trait` only allowed in function and inherent method return types, not in `Fn` trait param
  --> src/lib.rs:10:24
   |
10 |     fn baz(&self, _f: &impl Foo) {}
   |                        ^^^^^^^^
*/

impl Bar {
    fn baz(&self, _f: &impl Foo) {}
}

mock! {
    Bar {
        // Replace &impl with &dyn, happy compiler.
        fn baz(&self, _f: &dyn Foo);
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        let f = MockFoo::new();
        let mut b = MockBar::new();

        b.expect_baz().return_const(());

        b.baz(&f);
    }
}
corecode commented 1 year ago

sorry about the title spam.

asomers commented 1 year ago

fn baz(&self, _f: &impl Foo) and fn baz(&self, _f: &dyn Foo) aren't the same thing. The former is an alternate syntax for fn baz<F: Foo>(&self, _f: &F). A generic function, in other words. The latter, OTOH, takes f as a trait object. So it's not a generic function. Because of various deep-seated language issues, Mockall can't really work with generic arguments that aren't 'static. Switching to &dyn Foo for the mock object happens to work for all of the callers' syntax in your program, but there's no guarantee that it will continue to do so. So my advice to you is:

  1. Just add the + 'static to your original function signature. That's the easiest thing to do. It cannot possibly cause any failures at runtime, and any compile-time failures will be pretty obvious.
  2. If you really do need some non-'static arguments in your production code, then my second suggestion is to use mock!, but not the trait object. Instead, add the + 'static bound for the mock object in mock! but not in the original code.
  3. If that still doesn't work because you need non-'static arguments in your unit tests, then what you came up with is a clever if imperfect workaround.
asomers commented 1 year ago

I forgot to mention one other option: #[concretize]. This is a new feature currently available only in Mockall's master branch from git. It's not in any release yet. But it is intended to solve this exact problem. What it does it change a generic argument into a trait object for purposes of the expectation methods. But it doesn't change the signature of the method at all. You would use it like this:

#[automock]
impl Bar {
    #[concretize]
    fn baz<T: Foo>(&self, _f: &T) {}
}
corecode commented 1 year ago

perfect, that's exactly what I was looking for. Will it also work for &impl Foo?

asomers commented 1 year ago

perfect, that's exactly what I was looking for. Will it also work for &impl Foo?

I'm not sure. The &impl Foo syntax is uncommon, and offers no advantages over the regular syntax, so I haven't prioritized it with Mockall. But if it doesn't work, then you can always change the original trait's signature to use the regular generic syntax. It's equivalent.

glueball commented 10 months ago

I forgot to mention one other option: #[concretize]. This is a new feature currently available only in Mockall's master branch from git. It's not in any release yet.

Do you have any ETA for releasing #[concretize]? With "Return position impl trait in traits" (RPITIT) becoming stable later this month (together with async fn in traits), I expect that the need for #[concretize] will be much higher (for instance, I'd move some associated types to RPITIT).

At the very least, there could be a 0.12.0-beta.1 release (or 0.11.5-beta.1) which won't be pickup automatically by cargo, so only those that opt-in will be getting it. It'd also allow us to provide feedback on the new features!

asomers commented 10 months ago

I'm going to release it later today. BTW, RPITIT looks very useful, and Mockall might have some additional work to do to support that.

Mrodent commented 7 months ago

Ran into this issue. My understanding of the problem here is not that great.

But I also don't understand the recommended solution: you say "Just add the + 'static to your original function signature." In the OP's code his original function was

fn baz(&self, _f: &impl Foo) {}

... so how could we "add the + 'static" to that to solve the problem? I mean, could you possibly show the new function signature?

I tried to do this with my signature:

Failing due to error in question: pub fn new(document_root_path: &str, index_name: String, configuror: &impl config_logging::LoggingConfiguror, gather_closure: &dyn Fn(WalkDir, &Weak<HandlingFramework>) -> Result<HashMap<String, Box<dyn TextDocument>>>)
Following compiler recommendation: pub fn new<F: config_logging::LoggingConfiguror + 'static>(document_root_path: &str, index_name: String, configuror: &F, gather_closure: &dyn Fn(WalkDir, &Weak<HandlingFramework>) -> Result<HashMap<String, Box<dyn TextDocument>>>)

I get a different failure after this:

59 |     #[automock]
   |     ^^^^^^^^^^^- lifetime `'__mockall_gather_closure` is missing in item created through this procedural macro
   |     |
   |     undeclared lifetime
...
63 |         pub fn new<F:  config_logging::LoggingConfiguror + 'static>(document_root_path: &str, index_name: String, configuror: &F,
   |                    - help: consider introducing lifetime `'__mockall_gather_closure` here: `'__mockall_gather_closure,`
   |
   = note: this error originates in the attribute macro `automock` (in Nightly builds, run with -Z macro-backtrace for more info)

I'm assuming this is just too complicated (with the closure-as-parameter) ... but maybe you can suggest something?

asomers commented 7 months ago

You already answered your first question: convert the &impl T syntax into the generic function syntax. The compiler failure is different. It's probably a Mockall bug. I would appreciate it if you could open a new issue with a minimal reproducer.