Add Iterator::flatten

[Centril]

Tracking issue: #48213

This adds the trait method .flatten() on Iterator which flattens one level of nesting from an iterator or (into)iterators. The method .flat_fmap(f) is then redefined as .map(f).flatten(). The implementation of Flatten is essentially that of what it was for FlatMap but removing the call to f at various places.

Hopefully the type alias approach should be OK as was indicated / alluded to by @bluss and @eddyb in rust-lang/rfcs#2306 (comment).

cc @scottmcm

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nagisa]

You should use a new-type because it affords us more wiggle-room /wrt what we need to keep stable.

With a regular type we wouldn't be able to change what FlatMap<T> is ever again, because code like this becomes valid:

let banana: Flatten<Map<_, _>, _> = peach.flat_map(pineapple);

and would prohibit us from changing what FlatMap<_> is.

[Centril]

@nagisa

You should use a new-type because it affords us more wiggle-room /wrt what we need to keep stable.

One of my original ideas was to use a newtype, exactly with your rationale, but on the other hand, users may now have to impl MyTrait for 3 types (Flatten, FlatMap, Map) instead of just Flatten and Map... at least that is what @scottmcm convinced me with (and we don't have to delegate in the impl of FlatMap as well..).

Is there a particular reason you think we'd change the data definition of FlatMap (we can still specialize Flatten<Map<_, _>, _>..) to something else? should we expect the unexpected?

[Centril]

One reason to use a newtype instead could be to get rid of the type parameter U in Flatten<I, U> so we could just have Flatten<I> which is nicer.

With a type alias, we can't do anything with the U in FlatMap<I, U, F> since we have no were to put it (and the compiler ignores bounds put on type parameters of type aliases) and so we get an "unused type parameter" error. But we can both use PhantomData or constrain F or I with a newtype.

On the other hand... having one parameter means that we can't #[derive(Clone, Debug)] and must write out the impls ourselves (at least that is what happened when I tried it).

There seems to be good reasons to go with either a type alias or newtype - so for me it is a toss up / draw right now...

[bluss]

No new struct would be awesome for the code reuse benefits you have mentioned, but still, we'd prefer to have each of .flat_map() and .flatten() use their own iterator struct for encapsulation purposes. So, what @nagisa said.

I would like to boldly skip the much-unused double ended case in the flatten iterator (like Itertools does), but that creates an inconsistency that I guess is not so popular(?)

[Centril]

@bluss One newtype coming Right Up™ =)

On the DoubleEndedIterator, I think that may be too bold? Doing .flatten().rev() may be entirely within the realm of possibilities (cc @scottmcm)...(?) And if we skip .rev(), we don't get code reuse also. Too bad we can't specialize based on the need for doing .rev() so we can have our cake and eat it too..

[theotherphil]

It's worth mentioning that this is equivalent to flat_map. (And that flat_map is preferable in this case?)

[Centril]

Sure, why not =) Can't hurt.

[Centril]

So... I ran into a problem...

If we want to have one type parameter on Flatten, then it has to be:

pub struct Flatten<I: Iterator>
where I::Item: IntoIterator {
    iter: I,
    frontiter: Option<<I::Item as IntoIterator>::IntoIter>,
    backiter: Option<<I::Item as IntoIterator>::IntoIter>,
}

instead of:

pub struct Flatten<I, U> {
    iter: I,
    frontiter: Option<U>,
    backiter: Option<U>,
}

unfortunately, this implies that the struct definition of FlatMap (assuming we want code reuse) must be:

pub struct FlatMap<I, U: IntoIterator, F>
where I: Iterator, F: FnMut(I::Item) -> U {
    iter: Flatten<Map<I, F>>,
}

instead of:

pub struct FlatMap<I, U: IntoIterator, F> {
    iter: Flatten<Map<I, F>, U>,
}

but now we have imposed two additional conditions upon FlatMap<I, U, F>, namely I: Iterator and F: FnMut(I::Item) -> U. That is a requirement in any case if FlatMap is to be an Iterator, but doing this change will likely break people's code and so it is a non-viable path forward. We could also wait for the 2018 epoch, but I think that the two options below are better.

This I think leaves us with two viable paths to take:

1. FlatMap is not implemented in terms of Flatten<Map<I, F>> in which case we get zero code reuse. The consequence of Flatten having one type parameter <I> is that Clone and Debug can't be derived and we must write impls manually.
2. Flatten has two type parameters <I, U> instead of just <I> and this lets us define FlatMap<I, U, F> as a newtype over Flatten<Map<I, F>, U>. This gives us some code reuse (we still have to delegate all traits due to the newtype, but no real logic is repeated). Having the extra type parameter U is a bit annoying, but it seems like we can live with that given that itertools also has a U parameter for Flatten.

Here's my experiment... Dumping it for future reference: https://play.rust-lang.org/?gist=3cfcc5742a7fbdcaea163381f7c72951&version=nightly

My preference would be to go with option 2. but I am very much open to suggestions.

[Centril]

I discussed this further with @eddyb, and I think the best way forward is to have FlattenCompat<I, U> as an internal type that provides all the logic, and then just let Flatten<I> and FlatMap<I, U, F> delegate to those... I think this is the best we can get and gets us closest to having our cake and eat it too.

I'll fix this up in a bit =)

[Centril]

Refactored according to recent discussion and added requested docs by @theotherphil.

r? @alexcrichton

[alexcrichton]

Looks good to me, thanks @Centril! Want to open an issue for tracking the instability here and we can r+?

[Centril]

@alexcrichton Done =) Tracking issue is #48213.

[alexcrichton]

@bors: r+

Thanks!

[bors]

📌 Commit 1291432 has been approved by alexcrichton

[Centril]

r? @alexcrichton

Should be up to snuff now.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 819d57a has been approved by alexcrichton

[bors]

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

[yrashk]

@CryZe Christopher, I just ran into this issue with itertools, did indeed break my builds on nightly. Annoying, but I am not sure what would have been the best direction here.

[CryZe]

This should‘ve at least been a crater run and required some more discussion imo. Even without stabilization, this will break stable in 2 cycles.

[yrashk]

I agree, it would have been nice to at least have better data and maybe a discussion on this.

…

On Feb 26, 2018 3:12 PM, "Christopher Serr" ***@***.***> wrote: This should‘ve at least been a crater run and required some more discussion imo. Even without stabilization, this will break stable in 2 cycles. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#48115 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABxABUkgPl_Mt1iRb7xK8uj1YSLZ8Wks5tYmdbgaJpZM4SA1Bh> .

[kennytm]

@CryZe the insta-breakage should be addressed in #48552.

[bluss]

I'm sorry that I didn't investigate the impact of the itertools compatibility issue before this feature was merged. The explanation for that is simply that this is a day that I have been expecting, when std starts to incorporate some features of itertools.

There are two ways to work around compatibility problems, mentioned in this itertools PR: rust-itertools/itertools#266, including a free function called flatten that is available from itertools 0.7.7. An automatic solution is not yet available.