Vec::retain could (should?) give &mut T to the closure, not just &T

@aturon

Vec<T> has this method:

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

However, looking at its implementation, I don’t see a reason why the &T part couldn’t be &mut T, allowing in-place mutation of retained items. This would make the method more useful. The only downside that I can see is that the name does not reflect this new capability of the method. I believe this change would be backward-compatible.

This came up at today’s meetup where someone have a Vec<Item> and wanted (but didn’t know how) to iterate the vector, decreasing some quantity inside Item but also removing items from the vector when the quantity reaches zero.

CC @aturon

Speaking in context of rust-lang/rfcs#1105 this is a breaking change, but probably qualifies as a minor change. It might be interesting to pull it into the discussion as an example.

For the idea itself, it sounds sound and useful.

Oh, right. I was thinking of using this method with a closure with inferred parameter types, but it is indeed breaking if you have type annotations.

From the RFC:

it must always be possible to locally fix the problem through some kind of disambiguation that could have been done in advance (by using more explicit forms)

This case seems unusual since using the more implicit form is the change you could do in advance, but I suppose it qualifies.

@brson

I am +1 for this change in principle, but slightly worried about potential breakage. It's probably worth prototyping and testing out with @brson's crater tool.

If this causes breakage, an alternative would be to add a separate retain_mut method. The downside is that we then end up with two very similar methods, which might become confusing.

Changing the parameter to &mut breaks uses of retain with closures of the form |&x| {...}. There are a few uses of this in rustc, so this pattern might be used in user code as well.

One example in rustc:

../src/librustc/middle/infer/higher_ranked/mod.rs:445
region_vars.retain(|&region_vid| {

I'm not a huge fan of doing this. We made a bad call. That sucks, but this is also why we expose the guts of Vec.

It's interesting in general that we could choose to give out &mut instead of &, everywhere there is exclusive access even if you'd typically not mutate. Another example is Iterator::filter.

Sounds like the resolution shall be a separate API @gankro, retain_mut() which can be developed outside libstd.

Another case is BTree::iter_mut for the keys. I think in most of these cases, mutable access is a bit "strange" to have intuitively (and in the case of BTree, a footgun), but I think that's because we're not super used to thinking of &mut and & in terms of their shared-ness, but rather in terms of mutability (because that's often the most natural perspective IMO).

retain_mut is simple to lift out (retain uses only public api). I've put retain_mut into the crate odds.

I've suggested we have some kind of gathering of more-than-libstd collections API in a crate in contain-rs, no other interested people have said anything in that discussion yet.

Given that F: Fn(.., &T, ..) can be called anywhere that F: Fn(.., &mut T, ..) can, I wonder if it would be possible to make them coerce? Then the problems with this change would presumably be reduced to just type-inferal issues. (Obviously that would need an RFC.)

I wrote an RFC for retain_mut: rust-lang/rfcs/pull/1353

I'm ok with this change.

Triage: as time goes on here, I don't think that we can make this change, as the breakage is much more significant than in the few days post 1.0. I'm going to give it a close.

So what is planned to remove elements based on predicates and mutate the elements? #34265 has been closed in favor of this issue, but this issue has been closed too.

Drain filter is going to cover that.

If I do understand, retain will be useless? Each call to retain can be replaced with a call to drain_filter.

Well, that's up to your judgement. Existing retain usages will continue to work, which has a value.

retain will be useless?

Also, drain_filter is an iterator, so you have to drive it to completion for it to have any effect. retain is more eager, so it's more ergonomic for that case:

    let mut v = vec![1, 2, 3, 4, 5, 6];
    v.retain(|x| x % 2 == 0);
    println!("{:?}", v);

    let mut v = vec![1, 2, 3, 4, 5, 6];
    v.drain_filter(|x| *x % 2 != 0).count();
    println!("{:?}", v);

Existing drain_filter impl does not need to be driven explicitly -- it is driven in drop, too. It makes panic behaviour interesting but it is similar to .drain().

it is driven in drop, too

Ah, I guess that makes sense.

FYI, I just published a crate retain_mut which implements the idea here.

I've read all the comments, but still confused about what to do, that's not good! Ideally there would be a pinned comment or something with current best solution. I see that there are two crates odds and retain_mut that implement this... Are they best solution?

@cheako

@cheako You can also use drain_filter but you have to negate the closure's return value.

@Boscop

@Boscop drain_filter does something different... or it's documentation isn't as clear as retain's. drain_filter creates an iterator that would then need to be consumed ??for anything to happen??
Looking into it, that just leads to another question... Not a solution that's easily grasped. If drain_filter was supposed to be retain_mut it's not clear how.

Merge #85

For reference, retain_mut is currently being added to the standard library: #90829

Merge RoaringBitmap#85

Navigation Menu

Repository navigation

Vec::retain could (should?) give `&mut T` to the closure, not just `&T` #25477

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Vec::retain could (should?) give &mut T to the closure, not just &T #25477

Description

Activity

bluss commented on May 16, 2015

SimonSapin commented on May 17, 2015

aturon commented on May 28, 2015

Amanieu commented on Nov 6, 2015

Amanieu commented on Nov 6, 2015

Gankra commented on Nov 6, 2015

bluss commented on Nov 7, 2015

Gankra commented on Nov 7, 2015

bluss commented on Nov 7, 2015

wthrowe commented on Nov 7, 2015

Amanieu commented on Nov 7, 2015

arthurprs commented on Nov 10, 2015

steveklabnik commented on Mar 8, 2017

real-felix commented on Sep 21, 2017

bluss commented on Sep 21, 2017

real-felix commented on Sep 22, 2017

bluss commented on Sep 22, 2017

shepmaster commented on Sep 22, 2017

bluss commented on Sep 22, 2017

shepmaster commented on Sep 22, 2017

upsuper commented on Nov 28, 2018

cheako commented on Jun 12, 2020

Boscop commented on Jun 13, 2020

cheako commented on Jun 13, 2020

Ten0 commented on Jan 28, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Vec::retain could (should?) give `&mut T` to the closure, not just `&T` #25477