Remove drain-on-drop behavior from various DrainFilter iterators

[the8472]

Proposal

Problem statement

The current implementations can double-panic (via a panic-drop-panic cascade), may become aborts even on a single panic if the deny panic-in-drop RFC goes through and are not consistent with general Iterator laziness.

Motivation, use-cases

A panicking closure can lead to a double-panic while iterating. Playground link

#![feature(drain_filter)]
use std::collections::LinkedList;

fn main() {
    let mut c = LinkedList::from([1, 2, 3]);
    let _c = c.drain_filter(|_| panic!("panic")).collect::<Vec<_>>();
}

Solution sketches

Add #[must_use] and remove drain-on-drop behavior from the drain_filter impls for these collections:

• Vec
• LinkedList
• BTree{Map, Set}
• Hash{Map, Set}

Since it'll be redundant needed it will also remove vec::DrainFilter::keep_rest()

Additionally drain_filter can be renamed to extract_filter🚲🏚️ to avoid similarity to Drain which will retain its drain-on-drop behavior since it's less problematic as it doesn't evaluate a closure for each item. To be bikeshed.

Limitations

This may cause surprises with short-circuiting iterators. That can be worked around (unergonomically) by taking by_ref and then exhausting the rest once the main use is completed or by avoiding short-circuiting ops.

Links and related work

• Don't drain-on-drop in DrainFilter impls of various collections. rust#104455 (PR for the proposed change)
• Remove drain-on-drop behavior from DrainFilter hashbrown#374
• Don't allow unwinding from Drop impls rfcs#3288
• Tracking issue for Vec::extract_if and LinkedList::extract_if rust#43244 (comment) (summary comment highlighting the issue)
• Tracking Issue for {BTreeMap,BTreeSet}::extract_if rust#70530 (affected tracking issue)
• Tracking issue for HashMap::extract_if and HashSet::extract_if rust#59618 (affected tracking issue)

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[Amanieu]

Seconded. I think this is actually quite intuitive: only elements that are returned from the iterator are actually removed from the collection.

[Mark-Simulacrum]

I think renaming is a good idea, not least because the alternative is breaking people in a pretty subtle way as they upgrade their nightly version. (None of these are stable, right?)

Is the recommended pattern if you want the old behavior something like .for_each(|_| {}) to preserve performance? Or is there something better?

[the8472]

(None of these are stable, right?)

Correct.

Is the recommended pattern if you want the old behavior something like .for_each(|_| {}) to preserve performance? Or is there something better?

For most collections there's a retain method and my PR directs users to use it when they don't need the iterator.

Only LinkedList doesn't have that. But there you can't do much better than .for_each(drop) anyway.

[Mark-Simulacrum]

Ah, I see. I guess we stabilized retain_mut on Vec too now. Still, the rename seems like a good idea to avoid future confusion and subtle breakage now, even if most users not actively iterating have a better path in retain today.

[the8472]

Sure. Any preferences?

• extract_if
• extract_filter{ed}
• take_if
• ...

[Mark-Simulacrum]

No particular preferences from me.

[the8472]

This has been seconded and no objections have been registered, so I'm closing the issue as approved.