Implement Extend<(A, B)> for (Extend<A>, Extend<B>)

[Seppel3210]

I oriented myself at the implementation of Iterator::unzip and also rewrote the impl in terms of (A, B)::extend after that.

Since (A, B) now also implements Extend we could also mention in the documentation of unzip that it can do "nested unzipping" (you could unzip Iterator<Item=(A, (B, C))> into (Vec<A>, (Vec<B>, Vec<C>)) for example) but I'm not sure of that so I'm asking here 🙂

(P.S. I saw a couple of people asking if there is an unzip3 but there isn't. So this could be a way to get equivalent functionality)

[rust-highfive]

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

[cuviper]

FWIW, rayon has this for its ParallelExtend.

I also mentioned the idea for Extend in #78204 (IntoIterator for (A, B)), but I lost interest after that one fell through. I suppose the arguments are not directly parallel though, so maybe there's hope that this kind of "magic" is more acceptable on the unzipping side.

I also like it for FromIterator, enabling complex nested collect, like short-circuiting unzip as Result<(A, B), E>. We just added that to rayon's FromParallelIterator too. 🙂

[Seppel3210]

@cuviper I would also be a fan of an analogous FromIterator impl but I don't see a way to do that without some sort of temporary buffer or am I missing something?

[cuviper]

Here's a previous discussion where @yaahc used inner Default + Extend to implement FromIterator for tuples. I don't think there's a way to do it with inner FromIterator because that's a "pull" model, whereas Extend lets us "push" items. (Rayon's model is a mix of both pull and push.)

[dtolnay]

This is a neat idea though borderline for me on whether Extend should support this behavior, but I could get on board with it. Curious what the rest of the team thinks.

impl<A, B, ExtendA, ExtendB> Extend<(A, B)> for (ExtendA, ExtendB)
where
    ExtendA: Extend<A>,
    ExtendB: Extend<B>;

@rust-lang/libs
@rfcbot fcp merge

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @sfackler
• @yaahc

Concerns:

• future FromIterator compatibility and comparison resolved by Implement Extend<(A, B)> for (Extend<A>, Extend<B>) #85835 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[BurntSushi]

I think I just have my routine questions:

What is the use case for an impl like this? A code snippet might help.

Do we expect to impl Extend for (A, B, C), (A, B, C, D, ...) and so on? If not, why not?

I see there are no tests of this API? Could some be added?

[Seppel3210]

I have a doctest that I didn't push that would provide a test for the example I mentioned (unzipping Iterator<Item=(A, (B, C))> into (Vec<A>, (Vec<B>, Vec<C>))) and I wouldn't add an impl for (A, B, C), (A, B, C, D, ...) because it would be equivalent to (A, (B, C)), (A, (B, (C, (D, …)))) wich is already covered by this impl.
so should I push that doctest?

[BurntSushi]

Can you say more words about the use case here? I don't understand it. Is there a small code snippet (perhaps it's the doc test you've written) that shows what this impl unlocks that wasn't previous possible (or wasn't previously convenient)?

To be clear here, I don't have any specific problems with this API. I just generally want to understand use cases for APIs before signing off on them.

[BurntSushi]

Pushing a doc test SGTM.

[Seppel3210]

@BurntSushi I'm not on my main machine right now (where I have the code) but I can push in about 2 hours or so :)

[Seppel3210]

@BurntSushi does this clear it up a bit?

[yaahc]

Curious what the rest of the team thinks.

Personally I'm a fan. I'm not sure if I prefer this vs the version I suggested in the thread @cuviper linked that relies on FromIterator and collect. I'd like to get a detailed comparison of the pros and cons for the two approaches if possible. Also, I want to make sure that adding this impl won't prevent us from later adding the FromIterator impls. If both of these can exist together than that would be ideal imo and I think I'd want both. If not I want to get a clear idea of what issues we run into and if there are any other existing trait impls this could interact with.

[yaahc]

Let me make this official rq

@rfcbot concern future FromIterator compatibility and comparison

[BurntSushi]

@Seppel3210 Looking at the code, if I were to guess at the answers to my questions, it would be that this impl makes it easier to implement the unzip method, is that right?

Backing up, a use case should answer these questions:

1. What code in wild or "real world" will benefit from this?
2. If we didn't merge this, what would you need to do instead?

Maybe something that would help here is a doc comment on the impl, along with an example of using it directly.

Looking at the code more carefully now, I see that perhaps this is actually indirectly expanding the utility of the unzip method itself. And that your doc comment shows that. That took me a bit of time to figure out. (It generally takes me a long time to work out things like this, so when generics are involved, I definitely appreciate a prose description of what's going on. And I generally think that should be passed along in the docs too.)

[Seppel3210]

Yeah I maybe should have made that more clear (It was pretty clear in my head because I came up with it, but I didn't think about that other people didn't), this is primarily to extend the functionality of unzip.
Currently there is no good way to unzip an Iterator<Item=(A, B, C)> into three collections. If you want to do it right now the most ergonomic way would be

let (as, bcs): (Vec<_>, Vec<_>)= iter.map(|(a, b, c)| (a, (b, c))).unzip();
let (bs, cs): (Vec<_>, Vec<_>) = bcs.into_iter().unzip();

which is pretty awkward (extra step and extra allocation).
If this impl was added it would be much less awkward IMO

let (as, (bs, cs)): (Vec<_>, (Vec<_>, Vec<_>)) = iter.map(|(a, b, c)| (a, (b, c))).unzip();

(One less step, no unnecessary allocation, yay!)
@BurntSushi

[bors]

⌛ Testing commit 3d0c5d0 with merge 4e90017...

[dragostis]

As soon as I saw this, I thought it would have the same limitation as the impl on ParallelExtend in Rayon, namely the extremely slow compile times. But, oddly, it seems to work just fine. Does anyone know why the Rayon equivalent generates such long generic names while the one in std doesn't?

Also, this approach currently lacks a means to extend Vecs from &mut references. This is what I've been using as an alternative:

use std::marker::PhantomData;

struct ExtendRef<'a, T, E: Extend<T>> {
    extend: &'a mut E,
    _phantom: PhantomData<T>,
}

impl<'a, T, E: Extend<T>> ExtendRef<'a, T, E> {
    pub fn new(extend: &'a mut E) -> Self {
        Self {
            extend,
            _phantom: PhantomData,
        }
    }
}

impl<'a, T, E: Extend<T>> Extend<T> for ExtendRef<'a, T, E> {
    fn extend<I>(&mut self, iter: I)
    where
        I: IntoIterator<Item = T>
    {
        self.extend.extend(iter);
    }
}

fn main() {
    let mut v0 = Vec::new();
    let mut v1 = Vec::new();
    
    (ExtendRef::new(&mut v0), ExtendRef::new(&mut v1)).extend(
        (0..10).into_iter()
            .zip((0..10).into_iter())
    );
}

[cuviper]

@dragostis

As soon as I saw this, I thought it would have the same limitation as the impl on ParallelExtend in Rayon, namely the extremely slow compile times. But, oddly, it seems to work just fine. Does anyone know why the Rayon equivalent generates such long generic names while the one in std doesn't?

It's a much easier task to extend serially. The implementation here is pulled from unzip, and it's a pretty simple loop to unpack the tuple from the incoming iterator, then push to each collection. The nested case is still just one outer loop with nested extend_one calls, so it should scale pretty easily.

Rayon's design is much more complicated on the producer side, essentially a "push" model of iteration compared to the "pull" model of Iterator::next. Then ParallelExtend is also more complicated to be able to consume items in parallel, especially for collections that want to keep the original order. Hooking all of that up generically was quite a challenge, and I'm proud of how that turned out, but it does layer a lot of complexity for the compiler to chew through.
https://github.com/rayon-rs/rayon/blob/master/src/iter/unzip.rs

[dragostis]

@cuviper

The design here is definitely nice. I'm more curious if there's any way to make this work in LLVM. If you opt not to generate any code, rustc can check the example just fine which begs the questions whether there's any way to send less data to LLVM for codegen.

[cuviper]

Oh, I didn't realize that cargo check was still fast! Let's go back to #68926 on that point though...

[phimuemue]

Sorry to rake this up again, but what's the reason that this is implemented for 2-tuples instead for up-to-12-tuples (such as the std usually does)?

(It came up in a discussion in rust-itertools/itertools#996 (comment))

[Amanieu]

I think it just didn't occur to us at the time. I don't see any reason but to extend this to arbitrary tuples.

[shahn]

What would be the process to get this implemented? Could I just open a new PR with an implementation, or would something need to be done first to decide to actually do it?

[Amanieu]

Just open a new PR with the implementation, then a libs-api member will start an FCP to get team approval for it.