Stabilize partition_in_place

[slo1]

Closes #62543

[rust-highfive]

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

[scottmcm]

To copy an open question from the tracking issue:

• Is this best on Iterator -- where it is now -- or on DoubleEndedIterator like methods like rfold?

[bors]

☔ The latest upstream changes (presumably #77381) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[dtolnay]

@rust-lang/libs: This PR stabilizes https://doc.rust-lang.org/1.47.0/std/iter/trait.Iterator.html#method.partition_in_place.

pub trait Iterator {
    ...

    fn partition_in_place<'a, T, P>(self, predicate: P) -> usize
    where
        Self: Sized + DoubleEndedIterator<Item = &'a mut T>,
        P: FnMut(&T) -> bool,
        T: 'a {/*...*/}
}

Some design questions from the tracking issue:

• Is it maybe necessary to name it partition_unstable_in_place rather than partition_in_place, for symmetry with sort/sort_unstable and because the existing stable Iterator::partition is stable? Answer: not unless we'd want a stable in-place partition, but that would involve an allocated side buffer of displaced items and not something we would do. Stable partition in place isn't the common need that stable sort is.

• Put the method on Iterator or DoubleEndedIterator? Answer: the behavior of the method involves enough forward work that I like it on Iterator. The DoubleEndedIterator methods are exclusively backward iterating.

@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
• @KodrAus
• @dtolnay
• @sfackler
• @withoutboats

Concerns:

• should-this-be-an-iter-method (Stabilize partition_in_place #76505 (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.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[withoutboats]

@rfcbot concern should-this-be-an-iter-method

I'm vaguely concerned about the precedent set by this API addition, and I'd like to see more justification for it. It's a very strange and novel addition actually because of how it uses iterator, and I don't really see any big discussion of this.

1. What is the motivation for this addition? What are the use cases that require it?
2. The Iterator API is usually constructed as a by-value API: all of the adapters change the set of yielded members, they aren't designed to operate on an underlying collection in place. Is there any precedent in std for an in iterator that operates in place?
3. Is there any precedent in std for the extremely unusual where Self: DoubleEndedIterator<Item = &'mut T> bound? I've never seen std restrict self type to a subtrait like this, or restrict the associated item via this trick. Is it normal?
4. What types even meet this strange bound actually? Would it make more sense to just add this functionality to slice, instead of Iterator? I know there are some types that wouldn't be supported by that, but again this refers to the motivation question.

[timvermeulen]

Alexander Stepanov, author of the partition functions in the C++ STL, considers it to have been a mistake to place the elements that satisfy the predicate at the start, rather than at the end. See page 167 of his Notes on Programming at http://stepanovpapers.com/notes.pdf. Should we consider "fixing" this?

[cuviper]

@withoutboats

1. What is the motivation for this addition? What are the use cases that require it?

I think it falls in the category of "simple, but hard" that's good for std -- something that's a fairly straightforward concept, but not so easy to implement in Rust, especially for folks that aren't used to juggling multiple &mut references yet.

My motivation was to match the C++ std::partition, as mentioned in #62278 (comment). I don't have a specific use case, but maybe those from the tracking issue can chime in -- @PurpleMyst?

2. The Iterator API is usually constructed as a by-value API: all of the adapters change the set of yielded members, they aren't designed to operate on an underlying collection in place. Is there any precedent in std for an in iterator that operates in place?

Any iterator with &mut items is probably doing something in-place, like collection.iter_mut().for_each(update_it). I guess it would be less common for those items to interact though, like the swapping done here.

3. Is there any precedent in std for the extremely unusual where Self: DoubleEndedIterator<Item = &'mut T> bound? I've never seen std restrict self type to a subtrait like this, or restrict the associated item via this trick. Is it normal?

Iterator::rev is restricted to Self: DoubleEndedIterator, and rposition to Self: ExactSizeIterator + DoubleEndedIterator.

Iterator::cloned and copied are examples of restricting the associated type, where Self: Iterator<Item = &T>. Another is unzip for Self: Iterator<Item = (A, B)>.

4. What types even meet this strange bound actually? Would it make more sense to just add this functionality to slice, instead of Iterator? I know there are some types that wouldn't be supported by that, but again this refers to the motivation question.

Any ordered collection with mutable iterators would apply. It's more questionable whether it makes sense to be swapping items around. Yes for slices, VecDeque, and (maligned) LinkedList, but probably not BTreeMap::values_mut().

It could be a method for slices instead, but I think that restriction is only useful if it helps the implementation. Maybe that would make a stable variant feasible, but I haven't given that algorithm much thought.

@timvermeulen

Alexander Stepanov, author of the partition functions in the C++ STL, considers it to have been a mistake to place the elements that satisfy the predicate at the start, rather than at the end. See page 167 of his Notes on Programming at http://stepanovpapers.com/notes.pdf. Should we consider "fixing" this?

I think there's value in keeping that precedent as a point of consistency for C++ defectors. We also have some precedent of our own in that Iterator::partition returns true items in the first part of the tuple. I think his reasoning about scaling to 3-way or N-way comes in part from C++'s fuzzy bool/int promotions, whereas Rust's bool is not used in an ordered way so often. I'd probably use an enum "key" in Rust for 3-way, and an integer for N-way, but these are all distinct flavors.

[m-ou-se]

This PR was discussed in the libs team meeting of 2020-11-18. The consensus was that functions for algorithms like this should probably not be part of Iterator, especially since this method would not make sense for all (double ended) iterators. It would probably make more sense to have algorithms like this as free functions.

[cuviper]

It would probably make more sense to have algorithms like this as free functions.

That does exist as itertools::partition. Do we want to start adding some of these to std::iter? The same question arises for #78204 (comment) suggesting a free function zip vs. itertools::zip.

[bors]

☔ The latest upstream changes (presumably #80530) made this pull request unmergeable. Please resolve the merge conflicts.

[m-ou-se]

@rfcbot cancel

[rfcbot]

@m-ou-se proposal cancelled.

[crlf0710]

@slo1 Ping from triage, according to previous discussions. Would you like to help change the forementioned method into free functions? Thanks!

[slo1]

@slo1 Ping from triage, according to previous discussions. Would you like to help change the forementioned method into free functions? Thanks!

Oh whoops, I'm a beginner so a lot of the discussion went over my head. I didn't realize there was something actionable to do. When you say free functions, do you mean point free functions?

[m-ou-se]

@slo1 The conclusion was that we'd probably not want functions like this directly on the Iterator trait. Having them as a function not attached under a type or trait (often called a free function, e.g. std::iter::partition_in_place or std::algorithms::partition_in_place) might make more sense, but that would still need a separate discussion.

If you want to work towards that or discuss the stabilization of this feature more, you're very welcome to do so. But for now, I'm closing this PR and directing the discussion back to the tracking issue: #62543

Thanks!