-
Notifications
You must be signed in to change notification settings - Fork 13k
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::collect_into #48597
Add Iterator::collect_into #48597
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aidanhs (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. |
See also #45840, where I suggested |
|
I'm obviously in favor of I'm inclined to change my mind given technical arguments for why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with including these methods... here are some improvement ideas.
src/libcore/iter/iterator.rs
Outdated
fn extend<E>(self, mut extended: E) -> E | ||
where | ||
E: Extend<Self::Item>, | ||
Self: Sized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's quite a lot of indentation here - run rustfmt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran rustfmt 0.3.8-nightly (346238f 2018-02-04)
on libcore, but the results were rather catastrophic. It threw few dozens of errors, even more warnings and modified 950 files. I think I'll just manually format it to fit the convention of other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodeSandwich You could copy only this function into the playground and format it, so that you don't need to care about the rest of libcore.
src/libcore/iter/iterator.rs
Outdated
@@ -2463,6 +2463,52 @@ pub trait Iterator { | |||
} | |||
} | |||
} | |||
|
|||
/// Extends passed collection with the contents of an iterator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the
between passed
and collection
here and on extend_mut
as well.
src/libcore/iter/iterator.rs
Outdated
|
||
/// Extends passed collection with the contents of an iterator. | ||
/// | ||
/// This method consumes the iterator and adds all its items to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This phrasing seems to indicate that extended
will gain self.count()
number of new elements while it may gain zero new elements in the case of sets. I'd rephrase add
to include
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might also be prudent to mention that .by_ref()
exists if people are using .take(..)
and such things. This also applies to extend_mut
.
src/libcore/iter/iterator.rs
Outdated
/// passed collection. The collection is then returned, so the call chain | ||
/// can be continued. | ||
/// | ||
/// This method is a counter-part of `Extend::extend`, but instead of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should mention this in the documentation of Extend
with a note that the methods Iterator::extend(_mut)
are preferred.
src/libcore/iter/iterator.rs
Outdated
/// ``` | ||
#[unstable(feature = "iterator_extend", issue = "0")] | ||
fn extend_mut<E>(self, extended: &mut E) -> &mut E | ||
where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird indentation... rustfmt =)
src/libcore/iter/iterator.rs
Outdated
/// ``` | ||
/// let result = (3..5).extend(vec![1, 2]); | ||
/// assert_eq!(vec![1, 2, 3, 4], result); | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good example.. perhaps include a bit longer example as well which is more real-worldy?
src/libcore/iter/iterator.rs
Outdated
/// Extends referenced collection with the contents of an iterator. | ||
/// | ||
/// This method consumes the iterator and adds all its items to the | ||
/// referenced collection. The collection reference is then returned, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reword as: The mutable reference to the collection is then returned, [..]
I don't like the name |
@kennytm Oh no... non-commutativity strikes back! Still, I think the ordering is legible from the fact that one is an iterator and one is a collection.. But - do you have any suggestion perhaps.. we might be able to find a name better than both |
That's not true, |
@rkruppe 😢 But once you start applying methods on the iterators you get from one of the vecs, then I think it becomes legible. I admit |
Good point, @rkruppe! I wonder if this helper should thus be on For |
Interesting idea, hadn't thought of that =) On naming.. how about |
@scottmcm Awesome idea, I've added this impl and it works like a charm! |
Arbitrary libs team reassignment - r? @Kimundi |
0c566d9
to
f1eef5c
Compare
@Centril I've renamed The only thing I didn't do is mentioning |
iter.by_ref().take(5).collect_into(first_5);
iter.collect_into(the_rest); It is important because it allows the user to consume the elements of an iterator partially while still retaining ownership of the iterator so that it can be reused. I think the idiom can be useful together with |
ba384f2
to
e0342c3
Compare
I'm not trying to be pushy, we have plenty of time and I can't demand anything from volunteers in a FOSS project, but what are next steps of a PR? I think, we've considered all the comments, improved the design as much as possible and the code is finished. |
There's a reviewer assigned; they'll look at it at some point (or triage will bug them about it). Note that it was a weekend since things stabilized, and many team members don't do rust every day. Also, the nit: Consider updating the title of the PR, since the approach has changed from |
Ok thanks @aidanhs and for the analysis @scottmcm! I think that's a good sign in that we may be able to move forward with this but crater is by no means exhaustive Due to the breaking nature of this PR I'm going to tag this as |
For other like me just joining this thread now, the possible breakage being discussed is the new Do I read correctly that crater found no such impl collision? |
@SimonSapin correct! |
One one hand it seems unlikely that That’s one of the principles making breakage acceptable when it’s in type inference: it can be fixed by adding type annotation that are also valid (if unnecessary) before the change. |
I'd like to voice my support for |
I agree with @SimonSapin that we should be careful here, because its definitely a breaking change in the actual sense. But provided we don't get any actual issues with it, it seems a clear improvement. But one thing got me thinking. The pattern here is one used in other places in std, most notably the If we provided such a adapter for |
This was discussed during libs triage today and the conclusion was that we're not willing to add the There are alternate possible strategies though with different traits, multiple methods, newtype wrappers, etc. @CodeSandwich would you like to pursue any of those instead? |
We also noted that we probably don't want to go too hog wild with new abstractions as they often incur significant cost and this is a relatively small convenience method which may not justify adding, for example, new traits to the standard library |
Thank you for looking into this proposition so deeply 👍 The resolution is reasonable, I'll be happy to explore less intrusive solutions |
As best I understand, we do not wish to follow this path. I'm going to close the PR. Please reopen if I've misunderstood. |
(summary at the end) Hi everyone! I dug through several issues and landed here. I'd really like to see As far as I understand it, this PR has been closed because the blanket As far as see it, the following method should be suitable for pretty much all use cases: fn collect_into<E: Extend>(self, collection: &mut E) In the original RFC is a hint why the complexity with
I don't find this very convincing. In all cases I wanted something like In summary:
@CodeSandwich Would you be willing to work on a second attempt? If you don't want to or don't have time, I can create another PR (only if people agree with me, of course). In that case: may I reuse parts of your code in this PR? |
It's nice to see, that you support the idea behind this RFC :) The separation into If you want to reuse my code, go ahead :) I'm not sure, but the license of the whole Rust forces me to make it fully reusable by anyone anyway. |
@LukasKalbertodt I think the non- |
Mhhh I see. So the "ideal" case would be this: let vec = (1..100)
.filter(|i| i % 2 == 0)
.more_stuff(...)
.collect_into(Vec::with_capacity(50)); With only the let mut vec = Vec::with_capacity(50);
(1..100)
.filter(|i| i % 2 == 0)
.more_stuff(...)
.collect_into(&mut vec); Without let mut vec = Vec::with_capacity(50);
vec.extend(
(1..100)
.filter(|i| i % 2 == 0)
.more_stuff(...)
); I agree that the first version looks better than the second one. But I still think the second looks better than the last (so I wouldn't say "doesn't benefit at all"). As @scottmcm already said, this might be rare. I think this really only applies to when we want to "configure" an empty collection. Putting a pre-filled collection by value into Sadly, I really can't tell how many use cases for
Is there a clear path how to wait for these blanket impls? I don't think Rust will relax its coherence rules in the near future. And even when specialization finally lands, I don't think the blanket impl that was proposed here would be a lot better. It still breaks code that So I don't think our little problem is solved by any new language feature that will land in the near future... (that's why I started the discussion here :P)
I'm just not sure if that license is already valid when your code is not merged yet. 🤷♂️ |
I would rather have separate methods for accepting a reference or a value. Having one method for both seems over-generalized and ambiguous. Suggestion: fn add_to<E: Extend>(self, collection: &mut E);
fn collect_into<E: Extend>(self, collection: E) -> E; Even though these methods basically do the same thing, the semantics are quite distinct IMO.
Notice that the meaning of "collect" stays consistent with the existing Notice the reference variant does not need a return type but the value variant could have I think it is hard to overstate the readability gains afforded by this improvement. It allows more cases where you can create and consume an Iterator in the same call chain. It doesn't feel right to have to save an Iterator to a variable just to use it with |
I agree with @camsteffen that multiple methods is the best solution. I would bikeshed the names to While I have much love for the turbofish, Aforementioned bikeshedding:I propose, noting the existing collect API: // fn collect<B>(self) -> B
// where
// B: FromIterator<Self::Item>;
fn collect_into<E>(self, collection: &mut E)
where
E: Extend<Self::Item>;
fn collect_with<E>(self, collection: E) -> E
where
E: Extend<Self::Item>;
Alternative design for comparisonAs @alexcrichton mentioned, you could use a newtype wrapper. There is a kind of precedent in the Unfortunately it is bad at serving the original purpose of being syntax sugar. Wrapping in a cursor is about as annoying as let-binding the iterator. Imagine a wrapper type // then (fine)
let mut vec = (0..100)
.map(|x| x + 5)
.collect_bikeshed(Vec::new());
// but then (ew)
use core::iter::Sink;
let sink = Sink::new(&mut vec);
other_numbers
.into_iter()
.collect_bikeshed(sink);
// uh oh (borrow checker complains)
vec.push(5);
more_numbers.collect_bikeshed(sink);
// slightly better but still annoying
more.into_iter()
.collect_bikeshed(Sink::new(&mut vec)); Basically you can see how there would be a need for a method that does |
I'm going to have a go of implementing |
I'd really like having |
I gave up on it a while ago, as you've probably guessed on how long it's been since my latest reply. If someone else wants to give it a go, they're welcome to! Someone should probably open an issue to track it though. |
I can try to implement it. Edit: I've just seen there's a issue template which gives enough information about what a tracking issue is and how it works. I think I'm just going to open a PR then. |
Another that I use often enough is "collect_into_slice" (modified from: https://github.com/kchmck/collect_into_slice ): trait CollectSlice<'a, A>: Iterator<Item=A> {
fn collect_into_slice(&mut self, slice: &'a mut [A]) -> &'a [A] {
fn collect_into_slice_mut(&mut self, slice: &'a mut [A]) -> &'a mut [A] {
} |
Add Iterator::collect_into This PR adds `Iterator::collect_into` as proposed by `@cormacrelf` in rust-lang#48597 (see rust-lang#48597 (comment)). Followup of rust-lang#92982. This adds the following method to the Iterator trait: ```rust fn collect_into<E: Extend<Self::Item>>(self, collection: &mut E) -> &mut E ```
Add Iterator::collect_into This PR adds `Iterator::collect_into` as proposed by ``@cormacrelf`` in rust-lang#48597 (see rust-lang#48597 (comment)). Followup of rust-lang#92982. This adds the following method to the Iterator trait: ```rust fn collect_into<E: Extend<Self::Item>>(self, collection: &mut E) -> &mut E ```
This is PR for adding
Iterator::extend
andIterator::extend_mut
as discussed in RFC issue 2339. It was advised to bypass RFC and just implement it.