Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference return value with lifetime of self plus second parameter of same lieftime #134

Closed
e00E opened this issue May 25, 2020 · 6 comments

Comments

@e00E
Copy link

e00E commented May 25, 2020

Thank you for making mockall.

As the docs say https://docs.rs/mockall/0.7.1/mockall/#reference-return-values

Mockall can also use reference return values. There is one restriction: the lifetime of the returned reference must be either the same as the lifetime of the mock object, or 'static.

mockall supports this

struct StructWithReference<'a> {
    reference: &'a (),
}

#[mockall::automock]
trait Trait {
    fn foo<'a>(&'a self) -> StructWithReference<'a>;
}

If we add another parameter with the same lifetime the program no longer compiles

#[mockall::automock]
trait Trait {
    fn foo<'a>(&'a self, b: &'a ()) -> StructWithReference<'a>;
}
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the method body at 7:12...
 --> src/main.rs:7:12
  |
7 |     fn foo<'a>(&'a self, b: &'a ()) -> StructWithReference<'a>;
  |            ^^
note: ...so that reference does not outlive borrowed content
 --> src/main.rs:7:26
  |
7 |     fn foo<'a>(&'a self, b: &'a ()) -> StructWithReference<'a>;
  |                          ^
  = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the types are compatible
 --> src/main.rs:5:1
  |
5 | #[mockall::automock]
  | ^^^^^^^^^^^^^^^^^^^^
  = note: expected  `&__mock_Trait_Trait::foo::Expectations<'_>`
             found  `&__mock_Trait_Trait::foo::Expectations<'static>`

I understand that this might be expected. I am reporting this in case there is an easy fix inside of mockall and this case was merely overlooked.

@asomers
Copy link
Owner

asomers commented May 25, 2020

The first thing to notice is that while Mockall can handle your first example, it doesn't handle it very well. It will only be possible to set expectations where the reference is 'static. Secondly, notice why your particular example is particularly hard to mock: the caller of foo gets to choose the lifetime of the returned reference. And yet, the mock object needs to store its expectations somewhere. So the expectations must have lifetimes at least as great as the mock object, but foo's caller gets to choose the lifetime. Those requirements are contradictory. The fact that you added a 'a lifetime bound to self might make things easier. But it still isn't easy. I'm going to close this issue as a duplicate of #85 , even though it's a little more specific. And I can't promise that I'll get to it on any particular timeframe. Unlike callers of foo, posters of issues do not get to choose the return value's lifetime ;).

@asomers asomers closed this as completed May 25, 2020
@e00E
Copy link
Author

e00E commented May 25, 2020

Thank you for the explanation.

It will only be possible to set expectations where the reference is 'static

So the expectations must have lifetimes at least as great as the mock object, but foo's caller gets to choose the lifetime. Those requirements are contradictory

If the mocked returned reference is 'static then it is guaranteed to live at least as long as the caller's chosen lifetime so it seems this case can still work. The issue you linked wants to lift the 'static requirement but in this case we do not even get to the requirement because the code does not compile.

@asomers
Copy link
Owner

asomers commented May 25, 2020

It's not that the returned reference is magically 'static; it's only static if the closure you supply to returning is 'static. And no other kind of closure will compile. That's why the limitation is so significant.

@e00E
Copy link
Author

e00E commented May 25, 2020

Sorry if I'm being dense. My point is, at least I could be allowed to provide static closures if other kind of closures don't work. But currently I cannot even provide static closures even though they should work because the mock code generation doesn't compile. This would not be a perfect solution because as you say some closures won't work but it is better than not being able to mock at all.

@asomers
Copy link
Owner

asomers commented May 25, 2020

Yes, that's correct. But what I haven't been sufficiently clear about is that, from the perspective of Mockall's internals, these two issues are actually very similar. Fixing #85 is at the very least a precondition to this issue. More likely, I think that #85 would probably go at least 90% of the way to getting what you want. That's my judgement based on how the implementation works, not based on what the user-visible capabilities are.

@e00E
Copy link
Author

e00E commented May 29, 2020

In case someone finds this in the future. Here is a workaround that doesn't require changing your trait or mockall:

pub struct StructWithReference<'a> {
    reference: &'a (),
}

pub trait Trait {
    fn foo<'a>(&'a self, b: &'a ()) -> StructWithReference<'a>;
}

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

    #[mockall::automock]
    pub trait Trait_ {
        fn foo<'a>(&'a self, b: &()) -> StructWithReference<'a>;
    }

    impl Trait for MockTrait_ {
        fn foo(&self, b: &()) -> StructWithReference {
            Trait_::foo(self, b)
        }
    }
}
#[cfg(test)]
pub use mock::MockTrait_ as MockTrait;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants