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

API doesn't work well with distinct owned / ref types #20

Closed
epage opened this issue Apr 1, 2018 · 8 comments
Closed

API doesn't work well with distinct owned / ref types #20

epage opened this issue Apr 1, 2018 · 8 comments
Labels
enhancement Improve the expected

Comments

@epage
Copy link
Contributor

epage commented Apr 1, 2018

EqPredicate stores T and its eval takes a &T. But what about Vec/slice, String/str, and PathBuf/path?

Exploration Summary

After making Predicate use generics rather than associated types (see #29)

Dead ends

  • Predicate { eval<I: AsRef<Item>>(&self, item: I) }: not object-safe which means we can't Box<Predicate> or &Predicate.
  • Predicate { eval(&self, item: Item) } (ie Item=&str rather than Item=str): violates borrow checker in boolean predicates
  • Above with Item: Copy: Lifetime issues (branch)
  • impl Predicate<AsRef<T>> for EqPredicate<T>: No AsRef implemented for numeric types
  • impl Predicate<Borrow<T>> for EqPredicate<T>: Sized issues (branch)

Options:

  • Create an AsRef adapter
@epage epage added the enhancement Improve the expected label Apr 1, 2018
@nastevens
Copy link
Collaborator

nastevens commented Apr 2, 2018

Commented in #18 but makes sense to add here. The Predicate trait could be changed to accept Borrow<T> instead of &T. Since Borrow<T> is implemented for &T, this probably won't even end up being a breaking change, and would allow things using String and Vec to be way more ergonomic.

@epage
Copy link
Contributor Author

epage commented Apr 17, 2018

In looking at this, I also might need to change the Predicate trait. In researching, it seems the recommendation is to use associative types for return types (don't need to mention type on every call) and generics for parameters (allows multiple trait implementations per struct).

@epage
Copy link
Contributor Author

epage commented Apr 19, 2018

Ok, got a branch with Predicate using generics.

  • Can't make eval accept AsRef because that will make the function not object-safe which means we can't Box<Predicate> or &Predicate.
  • Tried changing eval's signature to Item from &Item, putting the burden of & on the client. This causes borrow check violations in the boolean predicates. The only way to make this route work is if we required Item to be copyable.

So remaining areas to explore

  • Item: Copy
  • Just modifying EqPredicates impl to more easily allow the Predicate to borrow EqPredicates owned type

epage added a commit to epage/predicates-rs that referenced this issue Apr 20, 2018
The hope is that generics will also make implementing support for assert-rs#20
easier.

So when to use each according to [The
Book](https://doc.rust-lang.org/book/second-edition/ch19-03-advanced-traits.html#associated-types-versus-generics):

Generics
- Used when type is going to be function input
- Can have multiple implementations of trait

Associated types
- Used when type is going to be a function output, like iterator
  - Avoids need to annotate type
- Can only have one implementation of trait

BREAKING CHANGE: The `Predicate` trait's API has changed.
@epage
Copy link
Contributor Author

epage commented Apr 20, 2018

You can see my Item: Copy work here

Ran into life time issues of all things.

@epage
Copy link
Contributor Author

epage commented Apr 20, 2018

I tried just limited myself to modifying EqPredicate to have a Predicate<Borrow<T>>.

Example of issues I ran into

---- src/ord.rs - ord::eq (line 50) stdout ----
        error[E0277]: the trait bound `str: std::marker::Sized` is not satisfied
  --> src/ord.rs:58:31
   |
11 | assert_eq!(true, predicate_fn.eval("Hello World"));
   |                               ^^^^ `str` does not have a constant size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `str`
   = note: required because of the requirements on the impl of `predicates::Predicate<str>` for `predicates::ord::EqPredicate<&str>`

https://github.com/epage/predicates-rs/tree/borrow_borrow

I also tried AsRef since that would offer nicer conversions but I couldn't pass numbers in.

@epage
Copy link
Contributor Author

epage commented May 29, 2018

Had some time to evaluate this. The work for #29 is probably the best we'll get.

@epage epage closed this as completed May 29, 2018
@epage epage reopened this May 31, 2018
@epage
Copy link
Contributor Author

epage commented May 31, 2018

So while we need to address Predicate<str> for EqPredicate<String>, another angle on this is passing a &'static str to EqPredicate and being able to get a Predicate<str>.

@epage
Copy link
Contributor Author

epage commented Jun 6, 2018

#49 is another step towards addressing this

@epage epage closed this as completed Aug 2, 2018
rshearman added a commit to rshearman/predicates-rs that referenced this issue Dec 27, 2022
It is very useful to be able to dynamically construct an object and
have that object owned by the predicate, yet evaluate against an
unowned type related to the owned one. An obvious example is a String
being owned by the predicate but being compared against &strs.

Therefore, implement Predicate for Eq/OrdPredicate that store an
object that implements Borrow for the predicate type, replacing
existing impls of Predicate<T> for Eq/OrdPredicate<T> and
Eq/OrdPredicate<&T>. This is backwards compatible as there are blanket
implementations of Borrow<T> for T and Borrow<T> for &T.

Note that Borrow imposes more requirements than are actually required
and AsRef would be sufficient. However, AsRef doesn't have a blanket
implementation for T and thus the existing impl of Predicate<T> for
EqPredicate<T> is still required, but results in a conflict since T
may also implement AsRef<T>. Requiring Borrow instead of AsRef is
sufficient for common use cases though.

This addresses assert-rs#20 more completely.
rshearman added a commit to rshearman/predicates-rs that referenced this issue Dec 28, 2022
It is very useful to be able to dynamically construct an object and
have that object owned by the predicate, yet evaluate against an
unowned type related to the owned one. An obvious example is a String
being owned by the predicate but being compared against &strs.

Therefore, implement Predicate for Eq/OrdPredicate that store an
object that implements Borrow for the predicate type, replacing
existing impls of Predicate<T> for Eq/OrdPredicate<T> and
Eq/OrdPredicate<&T>. This is backwards compatible as there are blanket
implementations of Borrow<T> for T and Borrow<T> for &T.

Note that Borrow imposes more requirements than are actually required
and AsRef would be sufficient. However, AsRef doesn't have a blanket
implementation for T and thus the existing impl of Predicate<T> for
EqPredicate<T> is still required, but results in a conflict since T
may also implement AsRef<T>. Requiring Borrow instead of AsRef is
sufficient for common use cases though.

This addresses assert-rs#20 more completely.
rshearman added a commit to rshearman/predicates-rs that referenced this issue Dec 29, 2022
It is very useful to be able to dynamically construct an object and
have that object owned by the predicate, yet evaluate against an
unowned type related to the owned one. An obvious example is a String
being owned by the predicate but being compared against &strs.

Therefore, implement Predicate for Eq/OrdPredicate that store an
object that implements Borrow for the predicate type, replacing
existing impls of Predicate<T> for Eq/OrdPredicate<T> and
Eq/OrdPredicate<&T>. This is backwards compatible as there are blanket
implementations of Borrow<T> for T and Borrow<T> for &T.

Note that Borrow imposes more requirements than are actually required
and AsRef would be sufficient. However, AsRef doesn't have a blanket
implementation for T and thus the existing impl of Predicate<T> for
EqPredicate<T> is still required, but results in a conflict since T
may also implement AsRef<T>. Requiring Borrow instead of AsRef is
sufficient for common use cases though.

This addresses assert-rs#20 more completely.
rshearman added a commit to rshearman/predicates-rs that referenced this issue Dec 29, 2022
…ypes

It is very useful to be able to dynamically construct an object and
have that object owned by the predicate, yet evaluate against an
unowned type related to the owned one. An obvious example is a String
being owned by the predicate but being compared against &strs.

Therefore, implement Predicate for In/OrdIn/HashInPredicate that store
an object that implements Borrow for the predicate type, replacing
existing impls of Predicate<T> for In/OrdIn/HashInPredicate<T> and
In/OrdIn/HashInPredicate<&T>. This is backwards compatible as there
are blanket implementations of Borrow<T> for T and Borrow<T> for &T.

Note that Borrow imposes more requirements than are actually required
and AsRef would be sufficient. However, AsRef doesn't have a blanket
implementation for T and thus the existing impl of Predicate<T> for
InPredicate<T> is still required, but results in a conflict since T
may also implement AsRef<T>. Requiring Borrow instead of AsRef is
sufficient for common use cases though.

This addresses assert-rs#20 more completely.
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

2 participants