Tracking issue for Vec::retain_mut and VecDeque::retain_mut

[GuillaumeGomez]

Feature gate: #![feature(vec_retain_mut)]

This is a tracking issue for adding the retain_mut method on Vec.

It'll provide a retain_mut method on the Vec and on the VecDeque types. It is the equivalent of the Vec::retain and VecDeque methods except that it'll expect a closure with a mutable argument instead of a const one. It'll allow to modify the elements of an array while filtering which elements we want to keep.

Public API

pub fn retain_mut<F>(&mut self, mut f: F)
    where
        F: FnMut(&mut T) -> bool;

Steps / History

• Implementation: Add Vec::retain_mut #90772 and Implement VecDeque::retain_mut #91215
• Final comment period (FCP)
• Stabilization PR

Unresolved Questions

None

[crumblingstatue]

Thank you! It's nice not to have to use a crate for this simple functionality.

[upsuper]

As I mentioned in #90772 (comment) maybe we should add this for VecDeque as well.

[GuillaumeGomez]

I'll send a PR and we'll see if the libs team will agree or not. I'll update the issue afterward.

[elichai]

This is pretty neat. if anyone else is looking then it seems that the current best stable alternative is:
vec = vec.into_iter().filter_map(|mut elem| {...}).collect()

[GuillaumeGomez]

Should we start an FCP to stabilize it?

[m-ou-se]

@rfcbot merge

[rfcbot]

Team member @m-ou-se 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
• @yaahc

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.

[Mark-Simulacrum]

To be clear, I was not suggesting that retain_mut should be delayed in favor of drain_filter, but that it seems annoying that the current implementations result in a flipped conditional -- if you are refactoring and start to need the elements returned from retain_mut, and are now using drain_filter, then you need to be careful to flip the return value of your closure.

I suppose this is more of an argument to change drain_filter than retain_mut, since retain_mut should match retain for sure, though.

[the8472]

From this zulip thread:

There are two ways retain_mut could be generalized further

• it could take a range, this way one can operate on a small subset of a large vec, this is especially useful when another method appends something to a vec and then the caller wants to perform some cleanup of the appended elements. Of course it's less beneficial when doing it at the beginning of a vec since that'll incur additional moves, but it can still help when the closure is expensive
• it could move the item instead of providing a &mut and then the closure could return the item if it should be retained. This allows the closure to do things that require ownership. By splitting the iteration into two phases the compiler may be able to realize that those moves are a noop as long as no item has been dropped so far. And thus come at no additional cost compared to taking a &mut (might need some benchmarking).

The latter might also obviate some needs for drain_filter.

[crumblingstatue]

I don't think either of those should be the job of retain_mut. retain_mut should behave exactly as retain, with the only exception of taking &mut.
Other APIs can be added later (after possibly a lot of bikeshedding). retain_mut should just be about correcting the mistake that retain doesn't take &mut. (which can't be directly fixed due to backwards compatibility issues)

[the8472]

I don't think either of those should be the job of retain_mut.

Obviously the method could be renamed if it does more.

Other APIs can be added later (after possibly a lot of bikeshedding).

If the performance is the same I don't see much of a benefit of having both.

[crumblingstatue]

The benefit is to have it now. It's a simple fix to a mistake. It doesn't bloat the standard library in any significant way. This has been missing for years because other alternatives keep getting bikeshedded.

[the8472]

Considering that things that go into std have to be maintained forever rushing things doesn't seem like a good plan either.

[crumblingstatue]

There is nothing to maintain to speak of. retain is already well established. retain_mut is the exact same function with changed mutability

[the8472]

Tests, documentation and general clutter in the API surface have non-zero costs. Anyway, I don't see urgency here and I would prefer not to spend more time on meta-discussion and instead discuss the API itself on its own merits rather than shutting down discussion on the ground that discussion is harmful because it delays things.

So, back to the API itself. Taking &mut T is a generalization here. I don't think it makes sense to generalize incrementally and leave intermediate steps behind as long as broader generalizations don't come with a performance or significant complexity cost.

As far as complexity for users goes I think retain_mut_range(.., closure) is easy enough to spell out if one wants to apply it to the full vec.

[crumblingstatue]

Taking &mut T is a generalization here.

I argue that it qualifies as a fix rather than simply a generalization. There is no reason why it shouldn't take &mut T, so taking &T was a mistake. The merit of this change is to fix that mistake (without breaking backwards compatibility).

If a more general API is possible, it would deprecate retain as well by extension, since it's the exact same function.
So this discussion should also extend to whether retain should be deprecated for a more general API, but I think that should belong in a separate thread of discussion.

[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.

[faern]

I submitted a stabilization PR. #95491. Looks like that's the only remaining thing here. And I would appreciate if this made it into 1.61

[yaahc]

I'm concerned that there was a fair bit of discussion and new comments on this issue during the FCP and I want to be careful to review the comments so far to make sure that none of them should be promoted to a blocking concern before I go ahead and approve the stabilization PR. I'll do that as soon as I'm done with the libs teams meetings in a little over an hour.

Edit: So my summary of the recent discussion is that there was some more discussion related to drain filter, that bled over into retain_mut. The main concerns raised were around:

• whether we could provide a generally more powerful API
• API Bloat

I don't think the API bloat concern is particularly specific to this issue, and any more powerful API would not necessarily solve the issue in this case since we would still be left with retain. I'd like to see more progress on how we can deal with API bloat long term such as by finding ways to remove APIs in the library at an edition boundary or via better organization of rustdoc output, but I do not want to block this issue on it and so I'm going to r+ the stabilization PR.

[nashley]

Apologies if this is not the appropriate place to ask this.

I'd like to be able to call async code within F.
Would that (presumably in a new method based off of this one) be a good fit for inclusion into std?

edit: looks like I want Keyword Generics.