Don't drain-on-drop in DrainFilter impls of various collections.

[the8472]

This removes drain-on-drop behavior from various unstable DrainFilter impls (not yet for HashSet/Map) because that behavior is problematic (because it can lead to panic-in-drop when user closures panic) and may become forbidden if this draft RFC passes.

closes #101122

ACP

affected tracking issues

• Tracking issue for Vec::extract_if and LinkedList::extract_if #43244
• Tracking Issue for {BTreeMap,BTreeSet}::extract_if #70530
• Tracking issue for HashMap::extract_if and HashSet::extract_if #59618

Related hashbrown update: rust-lang/hashbrown#374

[rustbot]

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[the8472]

For discussion: @m-ou-se proposed to additionally renaming drain_filter to avoid confusion with the behavior of drain which does eagerly drain on drop which is less of an issue because it doesn't evaluate closures while doing so.

One proposed name would be extract_filter().

[the8472]

LinkedList doesn't have a retain method, so unlike the other collections I can't recommend that as alternative.

[bors]

☔ The latest upstream changes (presumably #106209) made this pull request unmergeable. Please resolve the merge conflicts.

[apiraino]

Removing T-compiler (changes under ./compiler dont need a review)

@rustbot label -T-compiler

[pitaj]

@rustbot label -S-waiting-on-review +S-waiting-on-ACP

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rust-timer]

Finished benchmarking commit (6ee4265): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 1.2%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-1.8%, -0.1%]	35
Improvements ✅ (secondary)	-0.5%	[-1.1%, -0.2%]	14
All ❌✅ (primary)	-0.4%	[-1.8%, 1.2%]	38

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.5%	[1.1%, 10.0%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-5.1%, -1.4%]	5
Improvements ✅ (secondary)	-2.1%	[-3.4%, -1.0%]	10
All ❌✅ (primary)	2.0%	[-5.1%, 10.0%]	12

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-1.3%, -1.3%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.1%, 2.0%]	48
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.1%	[-0.3%, -0.0%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[-0.3%, 2.0%]	56

Bootstrap: 649.711s -> 647.883s (-0.28%)

[WaffleLapkin]

This is not actually true though, is it? I don't see any mentions of panic_flag except = true and = false in next

[the8472]

Good catch. With the Drop impl gone this is no longer needed. I'll do a followup.

[MarekSosnicki-deliveroo]

Hi @the8472,

I was upgrading Rust version in our project and I found that upgrade from 1.71.0 to 1.72.0 causes differences in our determinism checks. In short, we have a complicated heuristic algorithm with a lot of inputs and we have a way to check if it gives exactly the same results (if we do not expect changes).

In 1.72.0 Changelog I could not see anything that would affect our code, but for some reason our determinism checks failed.
I've used cargo-bisect-rustc to find the commit that started the issue and it pointed me to this PR.

I've analysed our code and I found where the behaviour has changed.
We have a DeterministicHashMap defined as follows:

pub type DeterministicHashMap<K, V> = std::collections::HashMap<K, V, BuildHasherDefault<FxHasher64>>;

In Rust 1.72.0 this DeterministicHashMap is sorted differently then in 1.71.0, I've checked the hashes generated by FxHasher64 and they remained identical, but something, probably this PR, caused different ordering inside the std::collections::HashMap.

It is not really much of a problem for us, as I now know understand the difference, I can just upgrade Rust version in our project.
However, I would like to understand why did this PR cause the change in the ordering?

[the8472]

That's probably due to the hashbrown update. You could try comparing the behavior by dropping hashbrown 0.13.1 and 0.14's types into your type alias instead of the std type.

[MarekSosnicki-deliveroo]

Ok, I've used HashMap from hashbrown 0.13.1 for the DeterministicHashMap and it passes.