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

"All" Predicate for tuples #75

Open
asomers opened this issue Feb 5, 2019 · 9 comments
Open

"All" Predicate for tuples #75

asomers opened this issue Feb 5, 2019 · 9 comments
Labels
enhancement Improve the expected

Comments

@asomers
Copy link
Contributor

asomers commented Feb 5, 2019

For my application I have need of a predicate that works on tuples. The function predicate is more cumbersome than I would like. Ideally, the predicate would apply a different predicate to every member of the tuple, and return the AND of all its children. It would be a macro, and usage would be something like this:

tuple!(eq(4), gt(10)).eval(&(4u32, 20i16)

Do you think that such a macro would be of general use? If not I'll add it to my crate alone. If so, I'll make a PR for predicates-rs.

@epage
Copy link
Contributor

epage commented Feb 5, 2019

I'll have to think about this. Maybe start with it in your crate, get runtime with it, and we can see about moving it to predicates-rs repo.

@epage epage added the enhancement Improve the expected label Feb 5, 2019
@asomers
Copy link
Contributor Author

asomers commented Feb 7, 2019

Here's an initial implementation of the macro.
https://github.com/asomers/mockall/blob/9174346d1a66db2e6067251e3f25b2c138ee2ce2/mockall/src/lib.rs#L979
However, I realized that there's a better way to do this than by using macros. If there were an additional trait that were implemented for tuples of predicates of all sizes, then a generic method could accept any argument implementing that trait, like this:

pub fn doit<I, P: IntoBoxPredicate<I>> (p: P) -> Box<dyn Predicate<I> + Send> {
    p.into_box()
}
doit(eq(5));
doit((eq(5), eq(6)));
doit((eq(5), eq(6), eq(-1)));

Here's one implementation of such a trait. Stick it in predicates-core/src/core.rs to make the above snippet work:

struct TuplePredicate2<T0, T1>(T0, T1);
impl<T0, T1> std::fmt::Display for TuplePredicate2<T0, T1> {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        unimplemented!()
    }
}
impl<T0, T1> reflection::PredicateReflection for TuplePredicate2<T0, T1> {}
impl<I0, I1, T0, T1> Predicate<(I0, I1)> for TuplePredicate2<T0, T1>
    where T0: Predicate<I0> + Send + Sync + 'static,
          T1: Predicate<I1> + Send + Sync + 'static,
{
    fn eval(&self, variable: &(I0, I1)) -> bool {
        self.0.eval(&variable.0) && self.1.eval(&variable.1)
    }
}

struct TuplePredicate3<T0, T1, T2>(T0, T1, T2);
impl<T0, T1, T2> std::fmt::Display for TuplePredicate3<T0, T1, T2> {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        unimplemented!()
    }
}
impl<T0, T1, T2> reflection::PredicateReflection for TuplePredicate3<T0, T1, T2> {}
impl<I0, I1, I2, T0, T1, T2> Predicate<(I0, I1, I2)> for TuplePredicate3<T0, T1, T2>
    where T0: Predicate<I0> + Send + Sync + 'static,
          T1: Predicate<I1> + Send + Sync + 'static,
          T2: Predicate<I2> + Send + Sync + 'static,
{
    fn eval(&self, variable: &(I0, I1, I2)) -> bool {
        self.0.eval(&variable.0) && self.1.eval(&variable.1) && self.2.eval(&variable.2)
    }
}

pub trait IntoBoxPredicate<I> {
    fn into_box(self) -> Box<dyn Predicate<I> + Send>;
}

impl<I, P> IntoBoxPredicate<I> for P
    where I: Send + 'static,
          P: Predicate<I> + Send + Sync + 'static,
{
    fn into_box(self) -> Box<dyn Predicate<I> + Send> {
        Box::new(self)
    }
}

impl<I0, I1, T0, T1> IntoBoxPredicate<(I0, I1)> for (T0, T1)
    where I0: Send + 'static,
          I1: Send + 'static,
          T0: Predicate<I0> + Send + Sync + 'static,
          T1: Predicate<I1> + Send + Sync + 'static,
{
    fn into_box(self) -> Box<dyn Predicate<(I0, I1)> + Send> {
        Box::new(TuplePredicate2(self.0, self.1))
    }
}

impl<I0, I1, I2, T0, T1, T2> IntoBoxPredicate<(I0, I1, I2)> for (T0, T1, T2)
    where I0: Send + 'static,
          I1: Send + 'static,
          I2: Send + 'static,
          T0: Predicate<I0> + Send + Sync + 'static,
          T1: Predicate<I1> + Send + Sync + 'static,
          T2: Predicate<I2> + Send + Sync + 'static,
{
    fn into_box(self) -> Box<dyn Predicate<(I0, I1, I2)> + Send> {
        Box::new(TuplePredicate3(self.0, self.1, self.2))
    }
}

But there's a catch: such a trait may only be implemented in the same trait that defines Predicate. To do otherwise would potentially create conflicting trait implementations, and rustc won't allow it. Even the unstable function specialization doesn't help. So this technique isn't possible unless you want to add the functionality directly to predicates-core.

@epage
Copy link
Contributor

epage commented Feb 10, 2019

Neat and interesting to see where you are using predicates.

This doesn't have to live in predicates-core. For example, assert_cmd and assert_fs define custom Intos that also allow for domain-specific implicit conversions.

I think I'd still prefer to see this grow close to its use and see how it matures and find out how generally applicable this will be.

@asomers
Copy link
Contributor Author

asomers commented Feb 10, 2019

Yes, I can define IntoBoxPredicate in my own crate, and at first I tried to. The problem is that I can't impl IntoBoxPredicate for all p: Predicate and impl it for any other type unless that's done in the same crate that defines Predicate. For example:

impl<I, P> IntoBoxPredicate<I> for P where P: Predicate<I> {...}
impl<I> IntoBoxPredicate<I> for AnythingElse<I> {...}

That will fail to compile, because it's a potential conflicting trait implementation violation. The problem is that the compiler thinks that a future version of predicates-rs might impl Predicate for AnythingElse. If that happens, then my crate will fail to compile. So the compiler preemptively disallows it today.

The only potential solution I see that lies within my crate would be to separately impl IntoBoxPredicate for each and every Predicate struct, like this:

impl<I> IntoBoxPredicate<I> for EqPredicate<I> {...}
impl<I> IntoBoxPredicate<I> for BooleanPredicate<I> {...}
...

That's obviously a PITA. It's also functionally inferior, because it won't cover any Predicate struct that a Mockall user might create. Worse, it wouldn't allow me to impl IntoBoxPredicate<I> for Fn<I> -> O, which is something I would like to do, because the predicates-rs crate might some day impl Fn<I> -> O for EqPredicate or something. It's another potential conflicting trait implementation violation.

If I get ambitious, I might fork predicates-rs to demonstrate how this would work. But for now, it's easier just to use my params macro.

@taminomara
Copy link
Contributor

I'm building on top of mockall and ran into the very same issue. Definitely would like to see a Predicate for tuples of predicates.

@asomers
Copy link
Contributor Author

asomers commented Jul 26, 2021

I totally forgot about this issue until taminomara reminded me. This feature isn't actually nearly as important now as it was prior to Mockall 0.1.0. Just a minor convenience now.

@epage
Copy link
Contributor

epage commented Jul 26, 2021

Sorry, I lost track of this.

Why is IntoBoxPredicate needed?

I would expect an upstream implementation for tuples to implement find_case and all of the reflection features so we can tell users which element in the tuple failed.

@taminomara
Copy link
Contributor

If I understand this correctly, IntoBoxPredicate is just a workaround for orphan rule. Upstream crate can definitely implement Predicate for tuples of predicates directly. Alternatively, we can have a function that takes a tuple of predicates and converts it into a single and predicate. I happen to have this function implemented here, albeit without find_case. It's essentially the same as above, but with nice documentation and it doesn't use Box =)

@epage
Copy link
Contributor

epage commented Jul 27, 2021

Ok, the way the comment was written, it sounded like it was suggesting to upstream IntoBoxPredicate .

I can see going either route and would be game for adding it if someone wants to.

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

No branches or pull requests

3 participants