Limitations of the Matcher Trait with Proxy reference type

[bjacotg]

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

[hovinen]

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]

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]

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]

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]

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]

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

[hovinen]

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]

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]

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]

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]

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]

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]

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]

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.