Implement Vec::drain from RFC 574

[bluss]

Implement Vec::drain() from rust-lang/rfcs#574, tracking issue #23055.

This is a big step forward for vector usability. This is an introduction of an API for removing a range of m consecutive elements from a vector, as efficently as possible.

New features:

• Introduce trait std::collections::range::RangeArgument implemented by all four built-in range types.
• Change Vec::drain() to use Vec::drain<R: RangeArgument>(R)

Implementation notes:

• Use @Gankro's idea for memory safety: Use set_len on the source vector when creating the iterator, to make sure that the part of the vector that will be modified is unreachable. Fix up things in Drain's destructor — but even if it doesn't run, we don't expose any moved-out-from slots of the vector.
• This .drain<R>(R) very close to how it is specified in the RFC.
• Introduced as unstable
• Drain reuses the slice iterator — copying and pasting the same iterator pointer arithmetic again felt very bad
• The usize index as a range argument in the RFC is not included. The ranges trait would have to change to accomodate it.

Please help me with:

• Name and location of the new ranges trait.
• Design of the ranges trait
• Understanding Niko's comments about variance (Note: for a long time I was using a straight up &mut Vec in the iterator, but I changed this to permit reusing the slice iterator).

Previous PR and discussion: #23071

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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. 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 CONTRIBUTING.md for more information.

[brson]

r? @aturon

[alexcrichton]

Stylistically the compiler doesn't tend to put an import-per-line like this but instead use multiple use statements.

[bluss]

One line with {RangeFull, Range, ..} for all the ranges would be the usual style then?

[alexcrichton]

Indeed! The wrapping (to a new import) just happens if it runs over 80 chars

[alexcrichton]

Style-wise the compiler typically puts this brace on the prior line (for here and the functions and such below)

[bluss]

Thanks, I should have checked for this! I'm somewhat of a rebel and I know it

[alexcrichton]

Props for this, it's a clever solution!

[alexcrichton]

Perhaps this could eschew the PhantomData by storing slice::IterMut<'a, T>?

[bluss]

The old PR had a discussion about that. This would affect variance, but I'm not clear on what's right. We use the iterator during iteration but then deref the *mut Vec once in the destructor to use it as &mut Vec.

[alexcrichton]

Ah that's why I was thinking of using IterMut instead of Iter (to take out the mutable borrow as opposed to a shared one). Although I'm not sure what effect that would have on this in terms of variance.

[bluss]

I've done my variance homework and IterMut seems fine. Both &mut Vec<T> and &mut T (in IterMut) should have the same implications (invariant in T).

[bluss]

I see why covariant in T is really possible here too. We could do that too. Just removing values from the Vec would allow it, but it feels shaky (and I don't know any example where it mattes in practice).

[alexcrichton]

This may be a good spot for a map:

self.iter.next_back().map(|ptr| unsafe {
    ptr::read(ptr)
})

[alexcrichton]

In the interest of saving space, either length or start can be derived from the other, right?

[alexcrichton]

Oh wait nevermind, the vector's length is "inaccurate" so it can't be used!

[bluss]

The vector's temporary length stores another necessary field -- the original starting point of the range we are removing.

[bluss]

Space that can be saved: The drop flag.

[bluss]

I'm not going to attempt the drop flag without advice because I'm not up to date with how filling drop interacts with that.

[alexcrichton]

There's a few next-line { characters that should move up here and below (just skimming the diff)

[bluss]

I've addressed all review comments, thank you @alexcrichton!

Introducing this as unstable gives us leeway to fix it up later, what we could change:

• Range trait method fn into_endpoints(self) -> (Option<T>, Option<T>) would allow interpreting usize as a n..n+1 range, but it feels bad to affix that interpretation in a general trait
• Gankro's preferred Drain trait in the previous PR would allow to do what we do with the Index trait -- each collection determines itself which ranges and index types to handle for Drain.
• Personally I don't like .drain(usize) because I want the API to always be range based.
• We may consider calling the method .remove_range instead. Depends on if you want to emphasize the removal feature or the iterator feature.

[alexcrichton]

On second though, could you apply just one of these stability attributes to the module itself? The instability should be inherited. Additionally, could you give this a name like collections_range as well?

[alexcrichton]

While you're at it, could you change the name of this feature to something like collections_drain?

[alexcrichton]

Same thought about changing the feature name here as above.

[alexcrichton]

Looks great to me, thanks @bluss! After some minor refinements of the feature names here I think this is good to go!

[bluss]

I'm excited. I'm working on checking that it compiles and then squashing it together.

[alexcrichton]

Thanks! Lemme know when it's ready to go and I'll r+

[bluss]

There, it should be ready. I could have missed something, haven't added new feature flags before.

[alexcrichton]

@bors: r+ 447ef06

Thanks!

[bors]

⌛️ Testing commit 447ef06 with merge dc19940...

[bors]

💔 Test failed - auto-mac-64-opt

[bluss]

Fixed that test.

[alexcrichton]

@bors: r+ b475fc7

[bors]

⌛️ Testing commit b475fc7 with merge 8871c17...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[Stebalien]

So I know I'm starting to sound like a broken record but... RangeArgument isn't going to play well with inclusive ranges. Would you consider changing it to something like Bounds (sometime before stabilization)?

use std::collections::Bound;
trait Bounds {
    fn start(&self) -> Bound<&T>;
    fn end(&self) -> Bound<&T>;
}
impl<T> Bounds for Range<T> {
    fn start(&self) -> Bound<&T> { Bound::Included(&self.start) }
    fn start(&self) -> Bound<&T> { Bound::Excluded(&self.end) }
}
// ...

---

I suggest adding an into_bounds(self) -> (Bound<T>, Bound<T>) method for extracting the endpoints by value.

[bluss]

I agree it's not ready for stabilization. Specifically the design of the trait over ranges, and then we should impl drain for more collections to see how the API holds up.

[glaebhoerl]

RangeArgument trait brainstorming:

trait IsRange {
    type Idx;
    fn as_range(&self)         -> Range<Option<&Idx>>;
    fn as_mut_range(&mut self) -> Range<Option<&mut Idx>>;
    fn into_range(self)        -> Range<Option<Idx>>;
}

• I feel like an associated type is more appropriate here? The existing range types all have an index type that's uniquely determined by the type of the range.
• Instead of a tuple or separate getters, we can use Range itself. I find this appealing aesthetically, though I'm not sure about the ergonomics. This could potentially have positive interactions if we were to give the Range types fleshed-out APIs with inherent impls (e.g. range intersection, smallest-containing-range, ...), as befits their status as built-in types.
• I'm not sure if there's a way to abstract the three very similar methods into just one, or if not, whether the single trait should perhaps be decomposed into a hierarchy (a la the other Foo, FooMut traits). (If the only implementors are going to be the built-in range types, perhaps not.)
• I don't like the name RangeArgument. Passing any range type as an argument is a particular use case, but not what the trait is. I'd most like to call it simply Range, but the conflict with struct Range would probably be too annoying. (I don't really like IsRange either - it's just a bad compromise after I couldn't think of anything better.)
• Can we just use convert::{As, AsMut, Into} instead? (Maybe if we had trait aliases to make it ergonomic...?)