Limitations of the Matcher Trait with Proxy reference type #323

bjacotg · 2023-11-17T16:29:51Z

See this playground for a minimal example.

While implementing matchers for the Rust protobuf library, I stumbled on an issue with the lifetime requirement of the Matcher trait. The lifetime issue still did not click right in my head, so forgive me if what I say is not exactly what is happening. But, as far as I currently understand it, the Matcher trait accepts &ActualT with any lifetime even though ActualT may be bounded, which makes the impl Matcher not able to access most functions of ActualT.

I see a few options to solve this issue:

Add a lifetime requirement on &ActualT to have a lifetime smaller than ActualT.
- Not sure if this will work. In my current set up with the protobuf, I managed to get it work, but of course all the other matchers do not work and will need to be updated.
Accept ActualT and not &ActualT in matcher, and constraint ActualT with Copy
- Not sure if this will work either. Same as above, it fixes the current issue, but it may break other use cases.
Do not support this pattern. Or the library is buggy
- Potentially, but I couldn't pinpoint where, so I still currently believe that the Matcher trait is just not right.

This reminds me of the issue we currently have with property!() and string slices as it was also a reference lifetime issue.

I will add more details while debugging further.

Current leads:

Adding the lifetime bound on the Matcher trait and see where it takes us
Constraining on Copy and see where it takes us
Changing the return lifetime of get_a_field() in the example from '_ to 'a seems to work, so maybe there is an issue in the library

The text was updated successfully, but these errors were encountered:

hovinen · 2023-11-20T21:28:11Z

I fiddled with the playground and got things to compile: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c9f86ed89ac8cf349a590f089b99b632

I needed to add a lifetime parameter to the Matcher trait among other changes. This feels pretty awkward. I'll play around a bit more to see what alternatives there are.

bjacotg · 2023-11-24T11:04:06Z

Nice find. I had a solution but with more lifetime parameters and clunkier.

The real world example was in:

Attempt at implementing directly: https://github.com/bjacotg/protobuf/pull/4/files
My first attempt with lifetime: https://github.com/bjacotg/protobuf/pull/3/files
Your proposal: https://github.com/bjacotg/protobuf/pull/1/files
The copy proposal: https://github.com/bjacotg/protobuf/pull/2/files

hovinen · 2023-11-24T12:26:34Z

At the same time, the ArenaHolder code looks odd to me:

impl<'a> ArenaHolder<'a, Strukt> {
    fn get_a_field(&self) -> ArenaHolder<'_, i32> {
        ArenaHolder {
            value: &self.value.a_field,
        }
    }
}

This is equivalent to

impl<'a> ArenaHolder<'a, Strukt> {
    fn get_a_field<'b>(&'b self) -> ArenaHolder<'b, i32> {
        ArenaHolder {
            value: &self.value.a_field,
        }
    }
}

The signature claims that the returned ArenaHolder will hold a reference whose lifetime is that of &self. But it's actually just taking a reference to a field in self.value, which has the lifetime 'a given by the lifetime parameter. And since it constructs a new ArenaHolder, there's no reason for its lifetime to be restricted to that of the self reference. So why not do this?

impl<'a> ArenaHolder<'a, Strukt> {
    fn get_a_field(&self) -> ArenaHolder<'a, i32> {
        ArenaHolder {
            value: &self.value.a_field,
        }
    }
}

bjacotg · 2023-11-24T14:04:00Z

To be completely honest, I may have butchered the actual meaning of the protobuffer library in my minimal example. The ArenaHolder is a simplified protobuf::FieldEntry and get_a_field is inspired from as_view.

hovinen · 2023-11-24T14:21:29Z

Okay, I understand now. The as_view method is explicitly present to reborrow the value with the lifetime of the ViewProxy which contains it.

Is there perhaps a way to avoid the reborrow? Can you show how you are building a matcher against the protobuf library?

bjacotg · 2023-11-24T16:37:40Z

Yes, see https://github.com/bjacotg/protobuf/pull/4/files

hovinen · 2023-11-26T17:25:44Z

I just ran into the same problem when trying to add a matcher for byte sequences. See #327.

The good news is that the example is much simpler. Perhaps this justifies adding a lifetime parameter to Matcher.

hovinen · 2023-11-27T10:09:01Z

I've tried adding a lifetime parameter to Matcher. Unfortunately this leads to more trouble with existing nested matchers. This fixes a specific lifetime as part of the matcher type. Whenever the outer matcher constructs a variable temporarily with which to use the inner matcher, that creates a lifetime which conflicts with the one embedded in the inner matcher's type. So the result does not compile.

See the result on this playground. Here I try the approach out with the displays_as and is_utf8_string matchers.

I have also tried switching the Matcher methods to be pass-by-value rather than pass-by-reference. The problem here is that implementations of Matcher must then set ActualT to be a reference in most cases. That reference must then have a named lifetime, which must come from somewhere. The only place from which it can come is again a lifetime parameter on the impl block, so one is back to the problem described above. What's more, the lifetime parameter is then no longer constrained by anything (associated types don't count), so the compiler complains.

bjacotg · 2023-11-27T15:01:51Z

I managed to get some success on the displays_as with an existential lifetime (which should work here, since String is concrete). See this playground.

However, this is probably tricky to generalize to fully generic matchers.

hovinen · 2023-11-27T15:53:13Z

Yeah, I tried using the where for<'b> method, but then couldn't use the matcher:

error: implementation of `matcher::Matcher` is not general enough
   --> googletest/src/matchers/char_count_matcher.rs:163:17
    |
163 |               err(displays_as(contains_substring(indoc!(
    |  _________________^
164 | |                 r#"
165 | |                 Value of: "äöüß"
166 | |                 Expected: has character count, which is equal to 3
167 | |                 Actual: "äöüß",
168 | |                   which has character count 4, which isn't equal to 3"#
169 | |             ))))
    | |_______________^ implementation of `matcher::Matcher` is not general enough
    |
    = note: `matcher::Matcher<'0>` would have to be implemented for the type `StrMatcher<'_, String, &str>`, for any lifetime `'0`...
    = note: ...but `matcher::Matcher<'1>` is actually implemented for the type `StrMatcher<'1, String, &str>`, for some specific lifetime `'1`

(This requires a lot of the library, so it's hard to put in a playground unfortunately.)

bjacotg · 2023-11-27T16:18:41Z

Hmmm, yeah, that would need a bit of extra work.

I will try to create a branch with only a few matcher implementations so it is easy to iterate over different trait definitions. I will add also the playground example of the ArenaHolder.

This will make it easier to share attempts of new trait definition and avoid the limitation of the playground.

bjacotg · 2023-12-01T14:08:35Z

I played around with

trait Matcher<'a> {
  type ActualT: Debug + 'a;
  fn matches(&self, actual: &'a Self::ActualT) -> MatcherResult;
}

in this branch

I believe the problem is that the trait is too flexible. You could implement a matcher that would store actual, but matchers should only use actual for the duration of matches.

I think:

trait Matcher<'a> {
  type ActualT: Debug +'a;
  fn matches<'b>(&self, actual: &'b Self::ActualT) -> MatcherResult where 'a: 'b;
}

(working on it in this branch)

or

trait Matcher {
  type ActualT: Debug;
  fn matches<'b>(&self, actual: &'b Self::ActualT) -> MatcherResult where Self::ActualT: 'b;
}

(but that probably does not compile)

hovinen · 2023-12-01T15:36:10Z

I had tried your first proposal but didn't get far.

I think your second proposal has the right idea. It almost compiles; see playground. The error I get is that putting the reference on an associated type is not considered to constrain the lifetime parameter. This is a known issue in Rust.

If I replace the associated type with a type parameter, it does compile. See playground. Of course, we had a good reason to switch to using an associated type, so I don't think that's the answer. Rather it would be great to fix the underlying issue in rustc.

hovinen · 2023-12-01T16:23:10Z

After lots of playing around with lifetimes, I did find a set of incantations which works in the case I linked above. See playground.

This is based on your second proposal, but has the twist that the ActualT used in the inner matcher is changed from &str to str. That requires adding ?Sized bounds everywhere, but that's actually fine, since ActualT is never passed by value.

Let me see whether this can be generalised.

bjacotg · 2023-12-01T16:24:27Z

The first proposal seems to avoid the issue of associated type constrains. Playground and branch. However, I am still blocked on it when calling the matcher.

This is my error:

error: implementation of `googletest::matcher::Matcher` is not general enough
  --> googletest/tests/arena_test.rs:50:5
   |
50 |     verify_that!(holder, GetAFieldMatcher { inner: anything() })
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `googletest::matcher::Matcher` is not general enough
   |
   = note: `impl googletest::matcher::Matcher<'2, ActualT = ArenaHolder<'_, i32>>` must implement `googletest::matcher::Matcher<'1>`, for any lifetime `'1`...
   = note: ...but it actually implements `googletest::matcher::Matcher<'2>`, for some specific lifetime `'2`
   = note: this error originates in the macro `verify_that` (in Nightly builds, run with -Z macro-backtrace for more info)

bjacotg · 2023-12-08T13:42:07Z

My current last attempt is to move back to generic parameters with this new matcher interface:

trait Matcher<ActualT> {
  fn matches<'a>(&self, actual: &'a ActualT) -> MatcherResult where ActualT: 'a;
}

I was expecting that this will cost us the support for Matcher::and(...) but in this PR, Matcher::and(...) still seems to work.

I will work more on it, after a few PR reviews.

bjacotg · 2024-01-12T12:16:49Z

(referencing conversation in #336 ) Matcher::and(...) is working with generic parameters since I kept the PhantomData field necessary for the associated type. This looked quite promising at first since we could keep both the support for Matcher::and(...) and the proto matchers by sprinkling seemingly unnecessary PhantomData fields.

However, you can't have your cake and eat it. Consider this test.

The compiler rejects this code with:

error: implementation of `googletest::matcher::Matcher` is not general enough
  --> googletest/tests/arena_test.rs:68:26
   |
68 |     verify_that!(holder, has_a_field(eq(33)))?;
   |                          ^^^^^^^^^^^^^^^^^^^ implementation of `googletest::matcher::Matcher` is not general enough
   |
   = note: `EqMatcher<ArenaHolder<'2, i32>, i32>` must implement `googletest::matcher::Matcher<ArenaHolder<'1, i32>>`, for any lifetime `'1`...
   = note: ...but it actually implements `googletest::matcher::Matcher<ArenaHolder<'2, i32>>`, for some specific lifetime `'2`

With the PhanthomData field, EqMatcher is forced to a fixed lifetime and cannot meet the bound: for<'b> Matcher<ArenaHolder<'b, i32>>.

Moreover, matchers cannot be returned by impl Matcher<T>, as it would run in the same issue.

Hence, continuing with this approach will require to:

Remove the PhantomData fields in matchers
Remove the support for Matcher::and(...)
Make all matcher struct public and return by their structure

I am still not convince this approach is the best (or whether it would work).

bjacotg · 2024-01-12T19:48:55Z

The approach described above does work, but sacrifices Matcher::and(...). However, I investigated this a little further and

I tried to look back at the Matcher::and(...) issues in the playground.

The simplest reproducible examples are:

Of course, removing the explicit ActualT in Without PhantomData is rejected by the compiler. But the log outputs with Matcher::<str> is interesting:

Calling Matcher::and with type *str*
Calling Conjunction::matches with type *alloc::string::String*
Calling MyMatcher::matcher with type *String*
Calling MyMatcher::matcher with type *String*

Somehow, the type checker is only confused on picking which Matcher::<_>::and(..., ...) but can decide on the rest (and go with Strings). In the With PhantomData, this problem does not come up. Since the type checker decides that MyMatcher should be a Matcher<String> then it is forced into Matcher::<String>::and(...).

But, the type checker shouldn't need to decide since Matcher::<str>::and(...) and Matcher::<String>::and(...) are the same functions, and does not depend on str or String. This can be achieved with a non-generic trait extensions like in this playground. This solution has still some issues, but could be refined further.

Another approach is to rely on impl trait in return to tip the type checker in the right direction. But I did not find a working solution.

bjacotg · 2024-01-20T11:08:56Z

I have finished one last experimentation and it seems that we have the following options:

#336 which migrate back to generic parameter, but trade automatically injecting Matcher::and and Matcher::or on all Matcher for this.

The trade can be handled by:

Removing Matcher::and and Matcher::or as this feature is supported by all! and any!
Injecting a non-generic trait extension on all types (like in this playground)
Requiring a non-generic trait extension be added to each Matcher implementation (like in this playground)

#353 which keeps associated types but let constrain the lifetime of &Self::ActualT

Both solutions make heavy use of HRTB which require to make the return types explicit (not through impl Matcher). This may be tricky for Matcher which type erase their inner matchers (like all!, any!, unordered_elements_are!, etc..)

Previously, `IsEncodedStringMatcher` converted its actual value input to a `Vec` and constructed a `String` out of that in order to match it. This is because using a string slice caused problems with the borrow checker as described in #323. The fundamental problem is that the type against which the inner matcher is matching is a reference whose lifetime must be named in the trait bound: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = &'what str>, ^^^^ What goes here??? ``` One option is just to remove the reference: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = str>, ``` This doesn't work when the inner matcher is `eq`, since one would then be comparing an `str` and a string slice: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str ``` However, this problem does not occur with `StrMatcher`: ``` verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes ``` So this also introduces an easy way to obtain a `StrMatcher` to check string equality: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes ``` This is slightly inconvenient, since one must use a different matcher to compare string equality in this case. But it is well-documented and not too inconvenient to use. So it should be okay.

hovinen · 2024-03-23T11:28:44Z

I've given this problem a bit more thought and looked more closely at the code in the protobuf library.

The basic problem occurs when one needs an inner matcher whose ActualT includes a lifetime. One must name the lifetime somehow. Normally one does this by:

adding a lifetime parameter to the struct, or
using the for <'a> construct to require that the inner matcher apply to all lifetimes.

But that lifetime is defined by the function Matcher::matches. So the first option of binding it to the struct is going to cause problems: at the very least, one must bring it into a relation with lifetime of the actual value passed to matches. This requires adding a bound to the trait itself, which means going down a rabbit hole.

The second option doesn't work because the type of the inner matcher is already fixed. So the lifetime of ActualT is fixed (even if unknown) and one can't declare that the matcher work with any lifetime.

In the case of ViewProxy from the protobuf library, that's basically just a kind of smart reference. It also needs a lifetime, and if one is going to write a matcher which matches against it, one needs to name that lifetime.

The idea of removing the reference to the actual value (and therefore requiring that that actual value be Copy) is an attempt to address the problem by removing one of the two lifetimes which need to be brought into relation, namely, that of the parameter actual. I have concerns about the effect of this solution on the rest of the library, as I wrote in my comment on PR #367. Namely, I still see a lot of places in that PR which involve adding reference operators in what look like completely arbitrary places. For example:

- verify_that!(vec![1, 2, 3], elements_are![eq(1), anything(), gt(0).and(lt(123))])
+ verify_that!(vec![1, 2, 3], elements_are![eq(&1), anything(), gt(&0).and(lt(&123))])

This will make the library much harder to understand and reason about.

I wonder whether there is another solution which is somewhere between the current state and #367. Namely, perhaps actual could accept general references which include ViewProxy, leaving the rest of the use of the library unchanged. I'll try some things out and see whether something can be done.

I've found a solution for the problem with is_utf8_string which, I think, should be okay for users. It relies on ensuring that ActualT does not include any references. I've sent PR #368 with this change.

This allows the use of smart pointers for the actual value when invoking `Matcher::matches` and `Matcher::explain_match`. The Protobuf implementation namely uses a [kind of smart pointer](https://github.com/protocolbuffers/protobuf/blob/62d5d9bf67d27d3b0084dabcf67bff2f8238162b/rust/proxied.rs#L113). The lifetime on that smart pointer has no a priori relation to that of the parameter `actual`, causing the borrow checker to complain whenever invoking an inner matcher on the output of `as_view`. This commit addresses the problem by allowing the `ViewProxy` -- rather than a reference to it -- to be passed directly to `Matcher::matches` and `Matcher::explain_match`. It does this by making those two methods generic with respect to the input type. They must only implement `Deref<Target = Self::ActualT>`. The new test `googletest/tests/arena_test.rs` illustrates this with a simplified example. Introducing these generics unfortunately makes `Matcher` no longer object-safe. Since several matchers construct trait objts on `Matcher`, this also introduces an internal-only wrapper trait `ObjectSafeMatcher` which every `Matcher` auto-implements. While this makes some changes to how `Matcher` is defined and implemented, it should have no effect on how it is _used_, other than to fix the aforementioned problem. So it should not affect compatibility for most users of the crate. Fixes #323.

hovinen · 2024-03-24T19:37:25Z

I've done some more experimenting and may have found a solution. See this commit. It works on the example of the playground to which you linked, with the addition that the ViewProxy struct (what you called ArenaHolder) implements Deref to the type it is proxying.

That said, I'm wondering what kind of API you envision for matching against protos. Do you envision something like 'matches_pattern!`, based on a kind of property matcher?

I'm not sure that the any of the approaches discussed here can work for that. Namely, the property matcher is based on an extractor closure which would in this case return a ViewProxy whose lifetime is limited to that of the parameter passed to it. But that parameter does not live longer than the call to the closure, so it cannot return such a ViewProxy. In the above commit, I added a test googletest/tests/arena_test.rs which illustrates the problem.

At the same time, it seems that an approach requiring the creation of a matcher for each field in the proto, as with has_a_field in your playground, will not really scale.

I'd suggest first clarifying exactly what kind of API you want to see for matching protos, then solving directly for that desired API.

Previously, `IsEncodedStringMatcher` converted its actual value input to a `Vec` and constructed a `String` out of that in order to match it. This is because using a string slice caused problems with the borrow checker as described in #323. The fundamental problem is that the type against which the inner matcher is matching is a reference whose lifetime must be named in the trait bound: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = &'what str>, ^^^^ What goes here??? ``` One option is just to remove the reference: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = str>, ``` This doesn't work when the inner matcher is `eq`, since one would then be comparing an `str` and a string slice: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str ``` However, this problem does not occur with `StrMatcher`: ``` verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes ``` So this also introduces an easy way to obtain a `StrMatcher` to check string equality: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes ``` This is slightly inconvenient, since one must use a different matcher to compare string equality in this case. But it is well-documented and not too inconvenient to use. So it should be okay.

This allows the use of smart pointers for the actual value when invoking `Matcher::matches` and `Matcher::explain_match`. The Protobuf implementation namely uses a [kind of smart pointer](https://github.com/protocolbuffers/protobuf/blob/62d5d9bf67d27d3b0084dabcf67bff2f8238162b/rust/proxied.rs#L113). The lifetime on that smart pointer has no a priori relation to that of the parameter `actual`, causing the borrow checker to complain whenever invoking an inner matcher on the output of `as_view`. This commit addresses the problem by allowing the `ViewProxy` -- rather than a reference to it -- to be passed directly to `Matcher::matches` and `Matcher::explain_match`. It does this by making those two methods generic with respect to the input type. They must only implement `Deref<Target = Self::ActualT>`. The new test `googletest/tests/arena_test.rs` illustrates this with a simplified example. Introducing these generics unfortunately makes `Matcher` no longer object-safe. Since several matchers construct trait objts on `Matcher`, this also introduces an internal-only wrapper trait `ObjectSafeMatcher` which every `Matcher` auto-implements. While this makes some changes to how `Matcher` is defined and implemented, it should have no effect on how it is _used_, other than to fix the aforementioned problem. So it should not affect compatibility for most users of the crate. Fixes #323.

Previously, `IsEncodedStringMatcher` converted its actual value input to a `Vec` and constructed a `String` out of that in order to match it. This is because using a string slice caused problems with the borrow checker as described in #323. The fundamental problem is that the type against which the inner matcher is matching is a reference whose lifetime must be named in the trait bound: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = &'what str>, ^^^^ What goes here??? ``` One option is just to remove the reference: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = str>, ``` This doesn't work when the inner matcher is `eq`, since one would then be comparing an `str` and a string slice: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str ``` However, this problem does not occur with `StrMatcher`: ``` verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes ``` So this also introduces an easy way to obtain a `StrMatcher` to check string equality: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes ``` This is slightly inconvenient, since one must use a different matcher to compare string equality in this case. But it is well-documented and not too inconvenient to use. So it should be okay. The introduction of `eq_str` also allows fixing a longstanding limitation of the matcher `property!` -- that it could not match on properties returning string slices. This PR therefore also updates the documentation accordingly and adds an appropriate test.

This allows the use of smart pointers for the actual value when invoking `Matcher::matches` and `Matcher::explain_match`. The Protobuf implementation namely uses a [kind of smart pointer](https://github.com/protocolbuffers/protobuf/blob/62d5d9bf67d27d3b0084dabcf67bff2f8238162b/rust/proxied.rs#L113). The lifetime on that smart pointer has no a priori relation to that of the parameter `actual`, causing the borrow checker to complain whenever invoking an inner matcher on the output of `as_view`. This commit addresses the problem by allowing the `ViewProxy` -- rather than a reference to it -- to be passed directly to `Matcher::matches` and `Matcher::explain_match`. It does this by making those two methods generic with respect to the input type. They must only implement `Deref<Target = Self::ActualT>`. The new test `googletest/tests/arena_test.rs` illustrates this with a simplified example. Introducing these generics unfortunately makes `Matcher` no longer object-safe. Since several matchers construct trait objts on `Matcher`, this also introduces an internal-only wrapper trait `ObjectSafeMatcher` which every `Matcher` auto-implements. While this makes some changes to how `Matcher` is defined and implemented, it should have no effect on how it is _used_, other than to fix the aforementioned problem. So it should not affect compatibility for most users of the crate. Fixes #323.

Previously, `IsEncodedStringMatcher` converted its actual value input to a `Vec` and constructed a `String` out of that in order to match it. This is because using a string slice caused problems with the borrow checker as described in #323. The fundamental problem is that the type against which the inner matcher is matching is a reference whose lifetime must be named in the trait bound: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = &'what str>, ^^^^ What goes here??? ``` One option is just to remove the reference: ``` impl<ActualT: AsRef<[u8]> + Debug, InnerMatcherT> Matcher for IsEncodedStringMatcher<ActualT, InnerMatcherT> where InnerMatcherT: Matcher<ActualT = str>, ``` This doesn't work when the inner matcher is `eq`, since one would then be comparing an `str` and a string slice: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq("A string"))) // Compile error: cannot compare str and &str ``` However, this problem does not occur with `StrMatcher`: ``` verify_that!("A string".as_bytes(), is_utf8_string(constains_substring("A string"))) // Passes ``` So this also introduces an easy way to obtain a `StrMatcher` to check string equality: ``` verify_that!("A string".as_bytes(), is_utf8_string(eq_str("A string"))) // Passes ``` This is slightly inconvenient, since one must use a different matcher to compare string equality in this case. But it is well-documented and not too inconvenient to use. So it should be okay. The introduction of `eq_str` also allows fixing a longstanding limitation of the matcher `property!` -- that it could not match on properties returning string slices. This PR therefore also updates the documentation accordingly and adds an appropriate test.

This allows the use of smart pointers for the actual value when invoking `Matcher::matches` and `Matcher::explain_match`. The Protobuf implementation namely uses a [kind of smart pointer](https://github.com/protocolbuffers/protobuf/blob/62d5d9bf67d27d3b0084dabcf67bff2f8238162b/rust/proxied.rs#L113). The lifetime on that smart pointer has no a priori relation to that of the parameter `actual`, causing the borrow checker to complain whenever invoking an inner matcher on the output of `as_view`. This commit addresses the problem by allowing the `ViewProxy` -- rather than a reference to it -- to be passed directly to `Matcher::matches` and `Matcher::explain_match`. It does this by making those two methods generic with respect to the input type. They must only implement `Deref<Target = Self::ActualT>`. The new test `googletest/tests/arena_test.rs` illustrates this with a simplified example. Introducing these generics unfortunately makes `Matcher` no longer object-safe. Since several matchers construct trait objts on `Matcher`, this also introduces an internal-only wrapper trait `ObjectSafeMatcher` which every `Matcher` auto-implements. While this makes some changes to how `Matcher` is defined and implemented, it should have no effect on how it is _used_, other than to fix the aforementioned problem. So it should not affect compatibility for most users of the crate. Fixes #323.

hovinen mentioned this issue Nov 26, 2023

Introduce a matcher for byte sequences which represent UTF-8 encoded strings #327

Merged

bjacotg mentioned this issue Dec 8, 2023

Migrate back to generic type parameter #336

Closed

bjacotg mentioned this issue Jan 20, 2024

Adding lifetime parameter to Matcher #353

Closed

This was referenced Feb 1, 2024

Implement Clone for possible matchers #355

Closed

Use ConjunctionMatcher in all! matcher. #358

Merged

Use DisjunctionMatcher for any![] macro. #359

Merged

bjacotg mentioned this issue Feb 16, 2024

Make Matcher::ActualT a generic parameter instead of an associated type #349

Merged

bjacotg mentioned this issue Mar 14, 2024

Copy instead of ref for ActualT #367

Merged

hovinen mentioned this issue Mar 23, 2024

Remove the need for allocation from is_utf8_string. #368

Open

hovinen mentioned this issue Mar 25, 2024

Parameterize Matcher::matches and Matcher::explain_match. #369

Open

copybara-service bot closed this as completed in #349 Apr 12, 2024

This was referenced May 3, 2024

Fixing type annotation with PhantomData #337

Closed

Moving the reference lifetime at the trait level #375

Closed

chmnchiang mentioned this issue Dec 14, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limitations of the Matcher Trait with Proxy reference type #323

Limitations of the Matcher Trait with Proxy reference type #323

bjacotg commented Nov 17, 2023 •

edited

Loading

hovinen commented Nov 20, 2023

bjacotg commented Nov 24, 2023

hovinen commented Nov 24, 2023

bjacotg commented Nov 24, 2023

hovinen commented Nov 24, 2023 •

edited

Loading

bjacotg commented Nov 24, 2023

hovinen commented Nov 26, 2023

hovinen commented Nov 27, 2023 •

edited

Loading

bjacotg commented Nov 27, 2023 •

edited

Loading

hovinen commented Nov 27, 2023

bjacotg commented Nov 27, 2023

bjacotg commented Dec 1, 2023 •

edited

Loading

hovinen commented Dec 1, 2023

hovinen commented Dec 1, 2023

bjacotg commented Dec 1, 2023

bjacotg commented Dec 8, 2023

bjacotg commented Jan 12, 2024 •

edited

Loading

bjacotg commented Jan 12, 2024

bjacotg commented Jan 20, 2024

hovinen commented Mar 23, 2024

hovinen commented Mar 24, 2024 •

edited

Loading

Limitations of the Matcher Trait with Proxy reference type #323

Limitations of the Matcher Trait with Proxy reference type #323

Comments

bjacotg commented Nov 17, 2023 • edited Loading

hovinen commented Nov 20, 2023

bjacotg commented Nov 24, 2023

hovinen commented Nov 24, 2023

bjacotg commented Nov 24, 2023

hovinen commented Nov 24, 2023 • edited Loading

bjacotg commented Nov 24, 2023

hovinen commented Nov 26, 2023

hovinen commented Nov 27, 2023 • edited Loading

bjacotg commented Nov 27, 2023 • edited Loading

hovinen commented Nov 27, 2023

bjacotg commented Nov 27, 2023

bjacotg commented Dec 1, 2023 • edited Loading

hovinen commented Dec 1, 2023

hovinen commented Dec 1, 2023

bjacotg commented Dec 1, 2023

bjacotg commented Dec 8, 2023

bjacotg commented Jan 12, 2024 • edited Loading

bjacotg commented Jan 12, 2024

bjacotg commented Jan 20, 2024

hovinen commented Mar 23, 2024

hovinen commented Mar 24, 2024 • edited Loading

bjacotg commented Nov 17, 2023 •

edited

Loading

hovinen commented Nov 24, 2023 •

edited

Loading

hovinen commented Nov 27, 2023 •

edited

Loading

bjacotg commented Nov 27, 2023 •

edited

Loading

bjacotg commented Dec 1, 2023 •

edited

Loading

bjacotg commented Jan 12, 2024 •

edited

Loading

hovinen commented Mar 24, 2024 •

edited

Loading