Tracking Issue for Vec::pop_if

[avandesa]

Feature gate: #![feature(vec_pop_if)]

This feature adds the Vec::pop_if method, which takes a predicate, evaluates it with the last element in the Vec if present, and returns the item if the predicate returns true. This makes it possible to conditionally remove the last element without the use of unwrap.

Public API

impl<T> Vec<T> {
    pub fn pop_if(&mut self, f: impl FnOnce(&mut T) -> bool) -> Option<T>;
}

Steps / History

• ACP: pop_if or Entry/Peek-like API for Vec and VecDeque libs-team#208
• Implementation: Implement Vec::pop_if #123107
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• Do we want a VecDeque::pop_front_if and VecDeque::pop_back_if API under this as well?
  • Separate tracking issue: Tracking Issue for VecDeque::pop_front_if & VecDeque::pop_back_if #135889

Footnotes

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

[GrigorenkoPV]

Found a piece of code in the wild that could benefit (get rid of an .unwrap()) with this API stabilized:
https://github.com/XAMPPRocky/tokei/blob/edbd5d5cbb0b7ea0081df0265a8ae6e7742a5051/src/language/syntax.rs#L663-L668

Also I myself have found this feature to be useful on couple occasions in personal projects.

This does not look too controversial, so maybe let's stabilize it?

The corresponding VecDeque methods can be added and stabilized later without blocking this.

I don't think I have right to start an FCP, so I will just open a PR instead.

[tgross35]

Docs should probably mention that the function does not get run if the vector is empty.

I don't think I have right to start an FCP, so I will just open a PR instead.

You can add the I-libs-api-nominated label with rustbot

[GrigorenkoPV]

You can add the I-libs-api-nominated label with rustbot

Thanks, that makes sense

@rustbot label +I-libs-api-nominated

Docs should probably mention that the function does not get run if the vector is empty.

True, it might have side-effects, so it's probably worth mentioning. I will adjust my PR.

[Amanieu]

@rfcbot merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

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.

[GrigorenkoPV]

@rfcbot concern impl-trait-syntax (well, I guess not enough rights)

Is there a reason why we use f: F ... where F: Fn... syntax instead of just f: impl Fn...? I recall having (or reading) a discussion about this with a reviewer like a year ago, and IIRC, they said that impl Fn is generally preferred, but since this syntax wasn't there since Rust 1.0, some APIs got stuck with an explicit generic argument.

Should I change this from

impl<T> Vec<T> {
    pub fn pop_if<F>(&mut self, f: F) -> Option<T>
        where F: FnOnce(&mut T) -> bool;
}

to

impl<T> Vec<T> {
    pub fn pop_if(&mut self, predicate: impl FnOnce(&mut T) -> bool) -> Option<T>;
}

in stabilization PR (#135488)?

[Amanieu]

Yes, please do. It's always backwards-compatible to change an impl Trait to a generic parameter, but not the other way around.

[GrigorenkoPV]

Done

[fmease]

It's always backwards-compatible to change an impl Trait to a generic parameter

This is not true: Such a change breaks call/usage sites where the generic argument list is specified (aka turbofished call/usage sites).

However, in the case of Vec::pop_if which doesn't have own generic parameters, such code is incredibly unlikely to be written in practice (only macro-generated code would realistically contain usages like v.pop_if::<>(f)).

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.