Do not implement Package trait automatically

[mpizenberg]

I've just received this by email and thought I'd put it here to have more feedback. What do you think?

---

Implementing a trait for all other types that implement something is a
really bad design decision because it makes all types available to an
interface, but most types are not relevant for that interface.

That's especially bad in a case where the trait (here Package) has
functions associated with it. This is not the case here, as Package
is a simple marker trait, so not too bad.

Still, making users of the library implement this is much better.

[mpizenberg]

One thing annoying in my opinion without default implementation is that users can only implement a trait on a type they own. So they could not use &str, String and plenty of other valid types for package identifiers. Maybe the alternative is to implement manually for those types in std that make sense?

[Eh2406]

I did think it was an odd design, but we do need to cover the cases from std, and maybe some common crates. CouldPackage be a type alias?

[mpizenberg]

Could Package be a type alias?

How to do that?

[mpizenberg]

In my mind, Package was mostly a type alias because I didn't want to have this cognitive overload everywhere

impl<P: Clone + Eq + Hash + Debug + Display, V: Version> Assignment<P, V> {

But if there is a better way to make a "type alias" for traits that's a good idea. We could also just do the Clone + ... everywhere and replace P by Package to reduce a bit the cognitive overload of wondering what P is. But that adds a lot of noise throughout the whole code base

[Eh2406]

Yes, you are correct. I got type alias witch is a thing and stable and grate confused with trait alias witch is what we need here and is unstable and not a thing. Looks like the recommended workaround for this case is what you have implemented.

[Eh2406]

This does bring up the related issue that Solver::run is a trait implementation, so theoretically someone could

impl<P: Package, V: Version + Hash> Solver<P, V> for BadSolver<P, V> {
    fn list_available_versions(&mut self, package: &P) -> Result<Vec<V>, Box<dyn Error>> {
        ...
    }

    fn get_dependencies(
        &mut self,
        package: &P,
        version: &V,
    ) -> Result<Option<Map<P, Range<V>>>, Box<dyn Error>> {
        ...
    }

    fn run(
        &mut self,
        package: P,
        version: impl Into<V>
    ) -> Result<Map<P, V>, PubGrubError<P, V>> {
        Ok(Map::new())
    }
}

I don't know why someone would, but our api lets them. Mabe run could be a free function that takes a impl Solver as an argument, or a method on anything that impls Solver.

[mpizenberg]

interesting, I didn't think of that, do you mind if I split that comment into another issue?

[matthiasbeyer]

Maybe the alternative is to implement manually for those types in std that make sense?

Yes, that exactly.

[aleksator]

Yes, that exactly.

I fail to see how that's better than the current solution.

[matthiasbeyer]

It's not implemented for every type automatically. It's like all fruits are oranges with the current solution. But not every fruit is an orange!

[aleksator]

I see the difference, but it's not necessarily better / solves any real problem is what I meant.

[harrysarson]

@matthiasbeyer I understand this (https://stackoverflow.com/questions/26070559/is-there-any-way-to-create-a-type-alias-for-multiple-traits) to be a recommendation for the approach that this library with a blanket implementation.

[mpizenberg]

The discussion was quite interesting but it seems @matthiasbeyer that you are seeing this from the wrong perspective. We do not see Package as a trait that the solver depends upon. Package ids of the solver depend upon Clone + Eq + Hash + Debug + Display. As such, we are making all types that support these constraints available for package identifiers. Now adding the Package trait as defined in here simply acts as an alias for those constraints. So you argument here in this perspective boils down to "do not use Clone + Eq + Hash + Debug + Display as a bound for package ids but use a trait instead". I think for the time being, for practical usage of this with std types we are keeping this design choice.