Make Sink::SinkItem a generic parameter

[cramertj]

See tokio-rs/tokio-core#273.

This would allow Sinks to accept values of multiple different types. I'm sure there's a reason it's an associated type right now, but I can't quite figure out why that is.

Example:

pub trait Sink<Item> {
    type SinkError;
    fn start_send(
        &mut self, 
        item: Item
    ) -> StartSend<Item, Self::SinkError>;
    fn poll_complete(&mut self) -> Poll<(), Self::SinkError>;
    ...
}

The same goes for the tokio-io Encoder trait:

pub trait Encoder<Item> {
    type Error: From<Error>;
    fn encode(
        &mut self, 
        item: Item, 
        dst: &mut BytesMut
    ) -> Result<(), Self::Error>;
}

This would solve this tokio-io issue, because users could write an impl like:

struct Bytes;

impl<'a> Encoder<&'a [u8]> for Bytes {
    type Error = io::Error;
    fn encode(&mut self, data: &'a [u8]>, buf: &mut BytesMut) -> io::Result<()> {
        buf.put(data);
        Ok(())
    }
}

impl Encoder<Vec<u8>> for Bytes {
    type Error = io::Error;
    fn encode(&mut self, data: Vec<u8>, buf: &mut BytesMut) -> io::Result<()> {
        buf.put(&data[..]);
        Ok(())
    }
}

Conceptually, Item is an input to a Sync or Encoder, rather than an output like it is in Stream, Future, and Iterator. As an input, it makes sense for it to be variant. For other examples of this design, see the Fn<...> traits, which use Args as a generic parameter, and Output as an associated type.

[alexcrichton]

Thanks for the report! It's interesting how this is pretty different from Future and Stream where it doesn't make sense to produce more than one value, but it may make sense to consume more than one value.

I wonder, though, if we could perhaps make this some sort of combinator? In any case this'll be a long-term issue as we can't change Sink for now.

[AljoschaMeyer]

This issue is currently blocking me.

I'm writing a typesafe interface for a multiplexing rpc protocol. Basically I want to open up multiple "virtual" sinks multiplexed over one "real" sink:

// This is just a sketch, not actual code. In particular, I don't need to consume
// the `T` to get a value accepted by the underlying sink. So I can actually
// return a `T` if the underlying sink doesn't accept the item.
pub struct MultiplexedSink<T>(UnderlyingSinkOfVecU8s);

impl<T: Into<Vec<u8>>> Sink<T> for MultiplexedSink<T> {
    fn start_send(&mut self, item: T) -> StartSend<T, Self::SinkError> {
        self.0.start_send(item.into())
    }
}

But currently, the best I can do is the following, which loses all the type-safety.

pub struct MultiplexedSink(UnderlyingSinkOfVecU8s);

impl Sink for MultiplexedSink {
    fn start_send(&mut self, item: Vec<u8>) -> StartSend<Vec<u8>, Self::SinkError> {
        self.0.start_send(item)
    }
}

The actual code uses serde traits and is fully independent of the actual serialization format. So while I could do better than just taking raw Vec<u8>s by taking a representation of the data format (i.e. serde_json::Value in my case), that would lose the genericity over the serialization format, and still wouldn't give me static typing. Just a stringly-typed (or rather jsonly-typed) interface.

The alternative would be to just define my own GenericSink trait, but that would force a trait upon the api consumers that is completely unsupported in the rust ecosystem.

As for "some sort of combinator", I'd be glad for any suggestions on how to do that, I currently don't see a way of implementing this.

So I really hope this change will land in this crate at some point, and I'm happy to contribute the necessary code myself. But even then, it looks like this would still be blocked until all other changes for 0.2.0 are done, right?

[AljoschaMeyer]

@alexcrichton are there any plans for making this change, or will this be dropped?

[cramertj]

@AljoschaMeyer We've been talking about a number of different possibilities for refactoring the Sink trait, and this is one that comes up often. The initial 0.2 release won't include any changes here, but we should be able to continue to experiment since the Sink trait is defined in the futures-sink crate, separate from futures-core.

[shepmaster]

Ping @cramertj, @alexcrichton — should a concrete decision be made on this for futures-preview 0.3?

[cramertj]

@shepmaster I made a prototype a while ago, but had to decide what to do about unifying the error type in the Sink trait. I wound up making a trait SinkBase { type SinkError; } and trait Sink<Item>: SinkBase { ... }, which worked, but made the impls a lot noisier and less ergonomic. Overall I still think that's the right direction to go as it's the only way that would allow us to have borrowed arguments while keeping a unified error type,, but I'd love to play around with other ideas if you have them!

[shepmaster]

other ideas if you have them!

Sadly, I have nothing useful to offer. I just followed links here from a similar problem and figured I could (a) poke you to see if there were any problems with the proposed implementation and (b) help make sure it didn't get lost among the bigger pile of work 😉

[seanmonstar]

I made a prototype a while ago, but had to decide what to do about unifying the error type in the Sink trait. I wound up making a trait SinkBase { type SinkError; } and trait Sink<Item>: SinkBase { ... }

I was thinking about the need for this change, since we've finished up a similar change in tower, but I don't follow what you mean with this comment. What problem arose that needed "unifying the error type"?

[cramertj]

@seanmonstar I wanted to accept a type that was a Sink over several different types, but for which the errors were all the same. This is, I think, what you'd want-- for the error type to always be the same, no matter what type was being written. However, it's possible to add this as a bound if necessary (e.g. T: Sink<Input1, Error = <T as Sink<Input2>>::Error>, or introduce a type param and do T: Sink<Input1, Error = E> + Sink<Input2, Error = E>.

[seanmonstar]

Oh I see what you mean! Hm, could there be cases where the error type were actually different? If a Sink wanted to be able to return the item in an error, it would need to be...

[cramertj]

@seanmonstar yeah, I could imagine trying to do something like that. My gut feeling is that being able to return the item is an uncommon case, and not terribly important to support, but I don't have strong feelings either way.