Repository navigation
Tracking Issue for vec::Drain{,Filter}::keep_rest #101122
Description
Activity
Why was this closed? The feature is still in nightly according to the docs, https://doc.rust-lang.org/1.78.0/std/vec/struct.Drain.html#method.keep_rest
Is there a separate ticket for Drain specifically? If not, is there some way to stabilize
keep_rest?On one of the unresolved issues:
Just change the not-yet-stable Drop for DrainFilter to keep rest?
This is now no longer unresolved, as it was handled in #104455
Another alternative: add a const EXHAUST_ON_DROP: bool = true const generic parameter
It's still impossible to set the flag without ownership You can store &mut Drain<'a, T, A, false> from the beginning Works better with iterator APIs, i.e. vec.drain_filter(f).keeping_rest().take(N).for_each(g);I think this is the best option, since it will also work if iteration panics in some way. Setting the flag is equivalent to just having
Either< Drain<.., true>, Drain<.., false> >so, I think this is also the most general solution of the bunch that handles panics.&mut self instead of self?
This would require setting a flag, and also handles panics, but I don't see any reason to set the flag some time after construction. And it's quite easy to add a flag on top of a
Drain<..., false>(even if a bit verbose). And could be wrapped in a crate.struct Drain<'a, T> { should_drain_on_drop: bool, drain: std::vec::Drain<'a, T, false>, // << with const-generic param to control drop behavior } impl<'a, T> Drop for Drain<'a, T> { fn drop(&mut self) { if self.should_drain_on_drop { // optionally user a scope guard to guarantee that all items get dropped, but that's not to hard to add as well self.for_each(drop); } } } impl Iterator for Drain<'a, T> { /* forward to std::vec::Drain */ }
so this is also easy to do.
So all in all, I think the const-generics approach remains the most flexible, and correct even in the face of panics.
Reacted by waffleWe briefly discussed this in the libs-api meeting today. We would like to defer stabilizing this until
extract_ifis stabilized so that we can get a better picture of whether this is still needed.extract_ifalready has the behavior of preserving all remaining elements if the iterator is dropped.There is also
BinaryHeap::drain_sorted, with the same on drop behavior asdrain. Should we replace it by "extract_sorted"?IMO
drain_sortedshould be changed to just clear the heap directly to avoid calling user code (viaOrd) during drop. This avoids any issues with double drops.I’d like to make one observation here, so it isn’t possibly missed in the future: (I haven’t seen it mentioned anywhere around here yet)
For
vec::Drain, we cannot soundly have both akeep_restand anas_mut_slicemethod :-)The relevant context:
- there was a comment when
vec::Drain::as_slicewas introduced: Add as_slice() to slice::IterMut and vec::Drain #58924 (comment)
that statedFWIW, these could both support
as_mut_slice()too, but I have no immediate use for that. I'm willing to add that for completeness if desired. vec::Drain<'a, T[, A]>is covariant inT(compare Make vec::Drain and binary_heap::Drain covariant #34951)- and of course,
vec::Drain::keep_restis tracked in this issue
So stabilizing
keep_restprecludes any future addition ofas_mut_slice.(Either method on its own is fine though;
as_mut_slicecan work on covariant types, as long as the contained values are fully owned.)Reacted by waffle and Jan Riemer- there was a comment when
@Amanieu even if double drop is not an issue, it can still be desirable to only remove some elements.
I only just learned about
Drain::keep_rest, and I want to note a mild concern about it: it breaks longstanding expectations in a way that could allow existing libraries to be broken by unreasonable users. I would expect approximately every place where the concrete typeDrainhas been used in libraries in the past to have the understanding that the values ceded would not return; but this allows them to return in a way that, at first blush, feels wrong to me.As a specific example I’ve just been dealing with (but I’ve hit other situations before): in Wayland, graphics tablet tools send a burst of events for a hardware change, so in handling it you can wish to collect the pending events, then when the
frameevent comes in, you flush the batch.- If the library gives consumers
&[Event], they can’t move certain things out of the events, so that’s not ideal. - If it gives
Vec<Event>(e.g. viastd::mem::take(self.pending_frame_events)), allocation efficiency suffers. - The obvious choice is to use
Drain<Event>in some way. The easiest way is to use the concrete type: it tends to only feel like mildly exposing an implementation detail, especially if the alternative you considered was exposingVec<Event>. - If you want to encapsulate it, you can use
impl Iterator<Item = Event>, but then you’ll want to add something like+ DoubleEndedIterator + ExactSizeIterator + FusedIterator + TrustedLen + Send + Sync + AsRef<[Event]> + Debug(and sorry, you lost your nice.as_slice()method) and it gets kinda unwieldy, but if you omit any of it, you’re damaging capabilities. - Of course, the ultimate future-compatibility-safest way is to wrap
Drainand duplicate all its surface area. In the past, that meant literally all—there was nothing you wouldn’t want. And sorry, you’ll need to useunsafein order to getTrustedLenthere.
But all things considered, just exposingDrain<Event>was often the most practical thing to do.
But now, if you do that, consumers can put items back.
So now I feel that I want to newtype drain and repeat every single thing of its surface area… except
keep_rest. And the also-unstableallocator, I guess.It’s not a big deal, I think, and anyone that actually uses
keep_restwhen given aDrainof this sort is crazy and emphatically asking for trouble, but it does feel like an important semantic change. It has become possible to misuse such things, where it wasn’t before.It’s like the drain part of a kitchen sink, which you thought was just a drain that things could flow out of, can pull a plunger out of nowhere, suck stuff back up into the sink, and slide a hidden plug into place to keep it there against your will. (At least it’s not full-on backflow, a sewage nightmare!)
I just wanted to record this mild discomfort that I have about this method. It feels a little bit wrong to me.
- If the library gives consumers
Feature gate:
#![feature(drain_keep_rest)]This is a tracking issue for
vec::Drain::keep_rest, methods allowing to keep elements in aVecafter draining some of them.Public API
Steps / History
vec::Drain{,Filter}::keep_rest#95376Unresolved Questions
.drain_filter(f).take(N)works as expectedDropforDraindrain_filter) then you'd need to write.foreach(drop)to explicitly drop all the rest of the range that matches the filter.&mut selfinstead ofself?Draininside a struct and are operating on it from a&mut selfmethod of the struct,keep_rest(self)is impossible to use. :(mem::replace(&mut self.drain_filter, Vec::new().drain(..)).keep_rest()but the borrow checker won't like that.Drainfield toOption<Drain>and useself.drain_filter.take().unwrap().keep_rest()along withunwrap()everywhere else that the drain is used. Not good..next()returnsNone.next()panic!s.keep_rest()sets a flag inside theDrainwhichDropwill use to decide its behavior, and you're free to continue accessing iterator elements in between.const EXHAUST_ON_DROP: bool = trueconst generic parameter&mut Drain<'a, T, A, false>from the beginningFootnotes
https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩