Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Iterator::join to combine Iterator and Join #75738

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions library/alloc/src/iter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
//! Allocation extensions for [`Iterator`].
//!
//! *[See also the Iterator trait][Iterator].*
#![unstable(feature = "iterator_join", issue = "75638")]

use crate::slice::Join;
use crate::vec::Vec;

/// Iterator extension traits that requires allocation.
#[unstable(feature = "iterator_join", issue = "75638")]
pub trait IteratorExt: Iterator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impl seems to be missing

Copy link
Contributor Author

@pickfire pickfire Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to impl IteratorExt for all items that impl Iterator? I thought this already does that? Do we need to do like impl IteratorExt for dyn Iterator or impl<T: Iterator> IteratorExt for T?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without any impl, it's just specifying the supertrait relationship as a requirement to implementors. I would recommend a blanket impl<T: Iterator + ?Sized>, which will include the dyn case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added impl<T: ExactSizeIterator + ?Sized> IteratorExt for T {}

Copy link
Member

@jswrenn jswrenn Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub trait IteratorExt: Iterator {
pub trait IteratorAlloc: Iterator {

I'd caution against calling this trait IteratorExt. The name isn't wrong—this is an extension trait of Iterator for contexts where alloc is available—but it's not the only reasonable extension trait of Iterator that could exist: what about an extension trait for Iterator that's usable in contexts where std is also available?

I'm really worried about the floodgates of future itertools breakage that this trait opens up. It's bad enough that the Itertools readme has to beg people not to contribute methods that could reside on Iterator, but at least we can welcome methods that involve allocation:

However, if your feature involves heap allocation, such as storing elements in a Vec, then it can't be accepted into libcore, and you should propose it for itertools directly instead.

An IteratorExt trait in alloc blows this advice up. If such a trait is introduced, I'm not sure there's any situation in which we can accept useful PRs without risking future conflict with the standard libraries. Itertools is less and less an ergonomic multitool than it is a ticking timebomb that will break your builds if you have the audacity to rustup. Can we please fix this problem?

/// Flattens an iterator into a single value with the given separator in
/// between.
///
/// Combines `collect` with `join` to convert a sequence into a value
/// separated with the specified separator.
///
/// Allows `.join(sep)` instead of `.collect::<Vec<_>>().join(sep)`.
///
/// ```
/// #![feature(iterator_join)]
/// use alloc::iter::IteratorExt;
///
/// assert_eq!(["hello", "world"].iter().copied().join(" "), "hello world");
/// assert_eq!([[1, 2], [3, 4]].iter().copied().join(&0), [1, 2, 0, 3, 4]);
/// assert_eq!([[1, 2], [3, 4]].iter().copied().join(&[0, 0][..]), [1, 2, 0, 0, 3, 4]);
/// ```
#[inline]
#[unstable(feature = "iterator_join", issue = "75638")]
#[must_use = "if you really need to exhaust the iterator, consider `.for_each(drop)` instead"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must_use is not appropriate here and probably a copy&paste bug, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

fn join<Separator>(self, sep: Separator) -> <[Self::Item] as Join<Separator>>::Output
Copy link
Member

@jswrenn jswrenn Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. Here we go again... :-(

Alternatively:

Suggested change
fn join<Separator>(self, sep: Separator) -> <[Self::Item] as Join<Separator>>::Output
fn smoosh<Separator>(self, sep: Separator) -> <[Self::Item] as Join<Separator>>::Output

😉

bunnies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall that Itertools::intersperse also exists, how was this issue solved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. It's not solved. Stabilizing Iterator::intersperse will cause breakages for Itertools users. In that case, the fix will be as simple as upgrading one's itertools dependency to a new version without Itertools::intersperse, since the signatures and behavior of Itertools::intersperse and Iterator::intersperse is virtually identical. That's not the case with join, unfortunately.

Copy link
Contributor Author

@pickfire pickfire Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we do a breaking change in itertools to test out this join? Previously the team did suggest to make a change in itertools but the join is already there and is quite limited, this is similar but there is more choice on the type of things joinable.

where
[Self::Item]: Join<Separator>,
Self: Sized,
{
Join::join(self.collect::<Vec<Self::Item>>().as_slice(), sep)
}
}
Comment on lines +31 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we're going to get Iterator::intersperse, so why not just:

Suggested change
fn join<Separator>(self, sep: Separator) -> <[Self::Item] as Join<Separator>>::Output
where
[Self::Item]: Join<Separator>,
Self: Sized,
{
Join::join(self.collect::<Vec<Self::Item>>().as_slice(), sep)
}
}
fn join<B>(self, sep: Self::Item) -> B
where
Self: Sized,
Self::Item: Clone,
B: FromIterator<Self::Item>,
{
self.intersperse(sep).collect()
}

This method can be defined on Iterator directly!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the reason why I don't want intersperse is that intersperse does not support joining with different types, it's the reason for this why I opened up this pull request, otherwise I will use intersperse instead.


impl<T: Iterator + ?Sized> IteratorExt for T {}
1 change: 1 addition & 0 deletions library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ mod boxed {
pub mod borrow;
pub mod collections;
pub mod fmt;
pub mod iter;
pub mod prelude;
pub mod raw_vec;
pub mod rc;
Expand Down