Could we remove the Copy requirement of ActualT type parameter in Matcher trait?

[chmnchiang]

In 471d4a2, the ActualT changed from an associated type to a type parameter. I think it is a good change, but is ActualT: Copy still required? In #323, there was some discussions about lifetime problems, but I don't think that is still valid now because the lifetime of the object being matched is not tied to the matcher anymore.

The comment in the code mentioned:

// `ActualT` requires `Copy` so that `actual` could be passed to `matches` and
// if it fails passed to `explain_match`. We can relax this constraint later by
// requiring only `Clone`.

But Copy in rust is generally considered cheap (if object is so large that memcpy is expensive, then the type probably shouldn't implement Copy in the first place), so there is no harm to take actual: &ActualT instead. If ActualT is Copy, the implementer can dereference it by themselves. This simplify a lot of lifetime problems when implementing custom matchers.

[bjacotg]

In #323, there was some discussions about lifetime problems, but I don't think that is still valid now because the lifetime of the object being matched is not tied to the matcher anymore.

Can you share more details why this is not valid anymore? There are a few playgrounds in the #323. I noticed that I did not add a regression test to demonstrate this. I will try to add them in the future.

This simplify a lot of lifetime problems when implementing custom matchers.

Custom matchers are something that is supported, but has not been the main focus of our devex effort. I would like to improve on that. Can you share the difficulty you are facing there?

[chmnchiang]

Sorry for the late reply

Custom matchers are something that is supported, but has not been the main focus of our devex effort. I would like to improve on that. Can you share the difficulty you are facing there?

For example, when I tried to implement a matcher that takes a Vec of sub-matchers and match each elements in a slice in order (similar to elements_are!, but does not rely on IntoIterator), I'll have to write

use googletest::{description::Description, matcher::MatcherResult, prelude::*};

#[derive(MatcherBase)]
struct MyMatcher {
    children: Vec<Box<dyn for<'a> Matcher<&'a i32>>>,
}

impl<'a> Matcher<&'a [i32]> for MyMatcher {
    fn matches(&self, actual: &'a [i32]) -> MatcherResult {
        if actual.len() != self.children.len() {
            return MatcherResult::NoMatch;
        }

        for (x, m) in actual.iter().zip(self.children.iter()) {
            if m.matches(x).is_no_match() {
                return MatcherResult::NoMatch;
            }
        }

        MatcherResult::Match
    }

    fn describe(
        &self,
        matcher_result: MatcherResult,
    ) -> Description {
        todo!()
    }
}

fn main() {
    let m = MyMatcher {
        children: vec![Box::new(points_to(eq(3)))],
    };

    let x = vec![1, 2, 3];
    let x = &*x;
    expect_that!(x, m);
}

If the matches trait function take &ActualT instead of Actual, then most of the lifetime will not be needed. For "but I don't think that is still valid now .." I was just trying to say that it doesn't seem that there are good reasons to take Actual instead of &ActualT from now on.

[chmnchiang]

Or put it another way, if I define the new trait that takes reference in matches and does not require Copy as:

trait MyMatcherTrait<T: std::fmt::Debug> {
    fn matches(&self, actual: &T) -> MatcherResult;
    fn describe(&self, matcher_result: MatcherResult) -> Description;
}

Then the following blanket implementation can be applied to any type that implements the current Matcher interface:

impl<T, ActualT> MyMatcherTrait<ActualT> for T
where
    T: Matcher<ActualT>,
    ActualT: std::fmt::Debug + Copy,
{
    fn matches(&self, actual: &ActualT) -> MatcherResult {
        Matcher::matches(self, *actual)
    }

    fn describe(&self, matcher_result: MatcherResult) -> Description {
        todo!()
    }
}

Which means that we don't really lose anything if we switched to the new trait.

[slinkydeveloper]

I've been trying to migrate from 0.11 to 0.13, and I stumbled on this new requirement pretty much throughout all my code, as my assertions were built all with owned values. For example, I have a looot of assertion methods that looks like these:

pub fn is_output_with_failure(
    code: u16,
    message: impl Into<String>,
) -> impl Matcher<ActualT = OutputEntryMessage> {
    pat!(OutputEntryMessage {
        result: some(pat!(output_entry_message::Result::Failure(eq(
            messages::Failure {
                code: code as u32,
                message: message.into(),
            }
        ))))
    })
}

And now I can't really implement them anymore:

pub fn is_output_with_failure(
    code: u16,
    message: impl Into<String>,
) ->impl for<'a> Matcher<&'a OutputCommandMessage> {
    pat!(OutputCommandMessage {
        result: some(pat!(output_command_message::Result::Failure(eq(
            &messages::Failure {
                code: code as u32,
                message: message.into(),
            }
        ))))
    })
}

as obviously:

error[E0716]: temporary value dropped while borrowed
   --> src/tests/mod.rs:159:14
    |
157 | /      pat!(OutputCommandMessage {
158 | |          result: some(pat!(output_command_message::Result::Failure(eq(
159 | |              &messages::Failure {
    | | ______________^
160 | ||                 code: code as u32,
161 | ||                 message: message.into(),
162 | ||             }
    | ||_____________^ creates a temporary value which is freed while still in use
163 | |          ))))
164 | |      })
    | |_______- opaque type requires that borrow lasts for `'static`
165 |    }
    |    - temporary value is freed at the end of this statement

Essentially taking ownership in the matchers of the "expected" values was super handy to create these methods, which now is not really possible anymore.

And yes I know this specific case I could nest a pat! matcher here, but i have more complex cases where the pat! matcher doesn't work because of private fields and i really wanna test eq.