IntoStream for vec

[yoshuawuyts]

This WIP PR implements IntoStream for Vec, adding three new stream types for vec: vec::Stream, vec::IntoStream, vec::StreamMut. We may want to extend this patch to also cover slice, because if we'd follow std, vec should use Stream and StreamMut types from slice::*.

Ref #129.

cc/ @stjepang @sunjay

Error

I'm currently blocked on a problem with our bounds. Namely we do:

impl<I: Stream + Send> IntoStream for I {
    type Item = I::Item;
    type IntoStream = I;

    #[inline]
    fn into_stream(self) -> I {
        self
    }
}

Which is causing the following error to show up:

I'm confused why this is happening, as std::iter has a similar impl. I'm worried this is some kind of orphan rule thing, but I don't quite get it. Getting help with this would be greatly appreciated!

[jamesmunns]

@yoshuawuyts is this a specialization problem? e.g. rust-lang/rust#31844 ?

Is this because Vec<T> could be Stream (EDIT: now, or at any point in the future), so then there would be two implementations, e.g. both of these impls would match:

  --> src/vec/into_stream.rs:10:1
   |
10 | impl<T: Send> crate::stream::IntoStream for Vec<T> {

  ::: src/stream/into_stream.rs:28:1
   |
28 | impl<I: Stream + Send> IntoStream for I {

[yoshuawuyts]

okay, I'm very confused by this. Rayon has a similar impl as we do, and they're not having the same problem. It seems like this may be something rather nuanced.

edit: for reference, they have impl IntoParallelIterator for Vec too.

[yoshuawuyts]

From conversations over chat, the next thing we should test is if it matters that we're using a foreign trait as part of the trait bound. Rayon doesn't do this, and we should check if only to cross it off.

[sunjay]

The thing that confuses me about this is that the following code compiles:

trait IntoStream {}
trait Stream {}

impl<T: Send> IntoStream for Vec<T> {}
impl<T: Stream + Send> IntoStream for T {}

These are exactly the same impls as what you seem to have. This is also exactly the same as what rayon has and it works for them.

This is probably not the issue at all, but have you tried removing the crate::stream prefixes from IntoStream? I'm getting into the "it's a compiler bug" mindset and wondering if that will fix it for some odd reason. If that doesn't work (and it shouldn't!) I can probably look into this more in a day or so and see if I can get it to work.

[taiki-e]

This behavior is not a bug. For example, if Vec comes to implement futures_core::stream::Stream, there is a conflict, so it is correct that this is a compilation error.

Refs: rust-lang/rust#43426

[sunjay]

Oh I didn't realize that Stream isn't defined in this crate. That's my bad then. Yes this isn't a bug.

[yoshuawuyts]

Even when working around this by making the bound our own Stream type in #214, this rule still seems to be triggered (which is good!) but bad news for us. @taiki-e is entirely right, and if we want to keep compat out of the box between async-std and futures-rs then the only way to add this feature is by moving the implementation to futures-rs; possibly even futures-core because that is where the Stream trait is defined.

Conversation about this has been happening in rust-lang/futures-rs#1879, but perhaps we should have a dedicated issue for this?

[yoshuawuyts]

Closing this PR because it isn't possible for all the reasons @taiki-e has listed. I've posted a summary of this PR in the streams tracking issue: #129 (comment).

My hope is that we can add IntoStream to futures-core so this can be possible. Thanks all!