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

Tracking Issue for vec::Drain{,Filter}::keep_rest #101122

Open
1 of 3 tasks
WaffleLapkin opened this issue Aug 28, 2022 · 2 comments · Fixed by #104455
Open
1 of 3 tasks

Tracking Issue for vec::Drain{,Filter}::keep_rest #101122

WaffleLapkin opened this issue Aug 28, 2022 · 2 comments · Fixed by #104455
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@WaffleLapkin
Copy link
Member

Feature gate: #![feature(drain_keep_rest)]

This is a tracking issue for vec::Drain{,Filter}::keep_rest, methods allowing to keep elements in a Vec after draining some of them.

Public API

// mod alloc::vec

impl<T, A: Allocator> Drain<'_, T, A> {
    pub fn keep_rest(self);
}

impl<T, F, A: Allocator> DrainFilter<'_, T, F, A>
where
    F: FnMut(&mut T) -> bool,
{
    pub fn keep_rest(self);
}

Steps / History

Unresolved Questions

  • Just change the not-yet-stable Drop for DrainFilter to keep rest?
    • Advantage: usually what you want (??)
      • e.g. .drain_filter(f).take(N) works as expected
    • Disadvantage: inconsistent with stable Drop for Drain
    • If you want to remove rest (matching the current unstable behavior of drain_filter) then you'd need to write .foreach(drop) to explicitly drop all the rest of the range that matches the filter.
  • &mut self instead of self?
    • If you're holding a Drain inside a struct and are operating on it from a &mut self method of the struct, keep_rest(self) is impossible to use. :(
      • You'd want something like mem::replace(&mut self.drain_filter, Vec::new().drain(..)).keep_rest() but the borrow checker won't like that.
      • Failing that, you'd need to change your Drain field to Option<Drain> and use self.drain_filter.take().unwrap().keep_rest() along with unwrap() everywhere else that the drain is used. Not good.
    • We'd need to define behavior of calling .next() after .keep_rest(). Presumably one of:
      • .next() returns None
      • this is considered incorrect usage and so .next() panic!s
      • .keep_rest() sets a flag inside the Drain which Drop will use to decide its behavior, and you're free to continue accessing iterator elements in between.
    • 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);
        Instead of
        let mut iter = vec.drain_filter(f);
        iter.by_ref().take(N).for_each(g);
        iter.keep_rest()

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@WaffleLapkin WaffleLapkin added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 28, 2022
@bors bors closed this as completed in 6ee4265 Jun 15, 2023
@RustyYato
Copy link
Contributor

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?

@dtolnay dtolnay reopened this May 12, 2024
@RustyYato
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants