Conventions for "blanket" implementations

[reem]

See the discussion here: reem/rust-modifier#7

Blanket implementations can enable some very convenient behavior, but they can also be problematic for some users. A set of solid conventions here would help.

[Manishearth]

Carrying over opinions from that thread, I would prefer that blanket impls be discouraged and be replaced with easily-impld traits with default methods.

The current system for normal trait impls is that if you want to apply a trait to a type where neither are from the current crate, you create a newtype.

I suggest that we continue along this vein and recommend that instead of a blanket impl, library authors should provide a trait with default methods (Which can be easily impld as impl TheTrait for MyStruct {}), and if a library user wishes to apply this trait to something defined in another crate, they must do:

struct Wrap(pub OtherStruct);
impl TheTrait for Wrap {}

basically, the same advice we give for cross-crate trait impls.

---

Aside from being in the same spirit as the usual recommendations for cross-trait impls, this has the advantage of not breaking things. A blanket impl has a good chance of breaking other crates. The newtype solution makes it a tad harder to use when you want to do it cross-crate, but it's no longer breaking anything.

[reem]

@Manishearth It's worth noting that the new type solution does not actually work in all cases, for instance in modifier the methods on the Set trait mention Self in their bounds, so you would not be able to apply modifiers of X to a Y(X) wrapper.

[Manishearth]

@reem The idea is that your impls for Modifier would take Y, not X as a parameter.

[Manishearth]

(impl Modifier<Y> {} instead of impl Modifier<X> {})

[reem]

I see. That would work. That would also mean a lot of unwrapping and re-wrapping if you wanted to mix modifiers from another crate and that crate was also not the definer of the wrapped type. (a contrived use case, I admit)

[mukilan]

How about bringing in the blanket trait into scope only when needed
i.e

fn main() {
   let x = {
      use krate::Set;
      A::new()
          .set()
          .set()
          .set()
   }
}

If it is too verbose, we can always wrap them in a macro

macro_rules! build(
   ($e:expr) => ({
        use krate::Set;
        $e
   });
)
fn main() {
   let x =  build!(A::new()
                      .set()
                      .set()
                      .set());

}

[Manishearth]

@mukilan I think that would still break; IIRC the blanket impl from rust-modifier clashed with set()s from Hyper without even being used.

Besides, requiring a couple of blank impls is less verbose than having little use statements everywhere :)

[mukilan]

@Manishearth : Could you give an example where it would break even without a use. Maybe I'm missing something, but doesn't Rust disallow calling methods of traits that are not in scope? And we can always isolate the Set trait and the blanket impl in their own module.

I do agree that the ad-hoc use statements are verbose, but as I said in my previous comment, we can wrap them in macros :).

And for the record, I don't like blanket impls as well. But in this case, they do seem pretty useful to have.

[Manishearth]

Oh, turns out I'm wrong, it won't clash. The error I was getting was due to a different sort of clash which the use can solve. However, you'll have a problem when any other trait implementing .set() is in scope, you'll need to selectively import that as well all over the place (which gets messy, and library users would have to rearrange their code or write their own macro for each clashing use statement). Also, if the type has a direct impl of .set() (impl Foo {pub fn set(&self) {...}} -- Hyper headers have something like this), that overrides this one, and I don't think there's a way around it without UFCS.

Wrapping every usage site in macros is still more verbose than a single impl per modify-able type :) (Also, if we bring macros into the picture, a lot more things are possible on both sides)

Blanket impls are a neat idea and convenient, but at least in the current state of Rust they seem to be a lot of trouble for relatively lesser convenience.

[mukilan]

@Manishearth : You're right. Having traits with same method names in the outer scope (that is also implemented for the same type or blanket) will cause name conflicts, and only UFCS an solve that.

[mukilan]

s/name conflicts/ambiguity

[Centril]

I've moved this over to rust-lang/api-guidelines#183.
Closing this in favor of that issue.