never patterns: Check bindings wrt never patterns #119610

Nadrieril · 2024-01-05T09:18:47Z

Never patterns:

Shouldn't contain bindings since they never match anything;
Don't count when checking that or-patterns have consistent bindings.

Nadrieril · 2024-01-05T09:33:08Z

I'm computing the binding map for all patterns now (instead of just those that have or-patterns), this could affect perf.

@bors try @rust-timer queue

bors · 2024-01-05T09:34:18Z

⌛ Trying commit 997103a with merge 23e3f32...

…<try> never patterns: Check bindings wrt never patterns Never patterns: - Shouldn't contain bindings since they never match anything; - Don't count when checking that or-patterns have consistent bindings. r? `@compiler-errors`

bors · 2024-01-05T11:00:57Z

☀️ Try build successful - checks-actions
Build commit: 23e3f32 (23e3f32508ac86df1f160dfd61db68c82954c84e)

Nadrieril · 2024-01-05T12:53:53Z

Alright, is this faster

@bors try @rust-timer queue

…<try> never patterns: Check bindings wrt never patterns Never patterns: - Shouldn't contain bindings since they never match anything; - Don't count when checking that or-patterns have consistent bindings. r? `@compiler-errors`

bors · 2024-01-05T12:55:03Z

⌛ Trying commit ad1bf5d with merge 1a3eeaf...

bors · 2024-01-05T14:21:35Z

☀️ Try build successful - checks-actions
Build commit: 1a3eeaf (1a3eeafd5326544c0e388a2dce8a4c122cf0d345)

rust-timer · 2024-01-05T15:48:00Z

Finished benchmarking commit (1a3eeaf): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.5%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.5%, -0.4%]	3

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

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)	9.3%	[1.3%, 21.3%]	15
Regressions ❌ (secondary)	1.5%	[1.2%, 1.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	9.3%	[1.3%, 21.3%]	15

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.803s -> 670.085s (0.04%)
Artifact size: 311.12 MiB -> 311.18 MiB (0.02%)

compiler/rustc_resolve/src/late.rs

compiler-errors · 2024-01-09T01:55:21Z

compiler/rustc_resolve/src/late.rs

-        // 1) Compute the binding maps of all arms.
-        let maps = pats.iter().map(|pat| self.binding_mode_map(pat)).collect::<Vec<_>>();
+        // 1) Compute the binding maps of all arms; never patterns don't participate in this.
+        let not_never_pats = pats


I wonder if we should use partition here. It would be nice to collect never pats even if we don't use them... 🤷

What would we do with them? Just store them in a Vec and drop it? I'm not sure I find that clearer; I could add more comments if you think this is confusing

compiler-errors · 2024-01-09T01:56:14Z

compiler/rustc_resolve/src/late.rs

-        // 5) Finally bubble up all the binding maps.
-        maps
+        // 5) Bubble up the final binding map.
+        let is_never_pat = not_never_pats.is_empty();


This is a bit... confusing IMO

So an or-pat is "never" if all of its constituents are never? This feels easy to mess up.

Yeah, Ok(x) | Err(!) works anywhere a normal pattern would. Compare with Ok(!) | Err(!), which is unreachable and doesn't take a body.

Unless you meant that the implementation is confusing?

No, I just mean the invariants feel subtle because what makes the product of patterns a never pattern is if one is never, but what makes a sum of patterns a never pattern is if all are never.

I think this should be made more clear, preferably with documentation and examples, because we really shouldn't be making resolve more complicated without helping future readers out, lol.

tests/ui/rfcs/rfc-0000-never_patterns/bindings.rs

Nadrieril · 2024-01-10T21:14:25Z

@rustbot ready

compiler-errors · 2024-01-14T23:44:55Z

@bors r+

bors · 2024-01-14T23:44:59Z

📌 Commit 5ccd29d has been approved by compiler-errors

It is now in the queue for this repository.

…compiler-errors never patterns: Check bindings wrt never patterns Never patterns: - Shouldn't contain bindings since they never match anything; - Don't count when checking that or-patterns have consistent bindings. r? `@compiler-errors`

bors · 2024-01-15T02:10:03Z

⌛ Testing commit 5ccd29d with merge 7d34628...

bors · 2024-01-15T06:25:03Z

💥 Test timed out

rust-log-analyzer · 2024-01-15T06:46:14Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Nadrieril · 2024-01-15T11:17:28Z

@bors retry

bors · 2024-01-15T21:24:16Z

⌛ Testing commit 5ccd29d with merge 714b29a...

bors · 2024-01-15T23:20:49Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 714b29a to master...

rust-timer · 2024-01-16T04:20:29Z

Finished benchmarking commit (714b29a): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.3%	[0.3%, 0.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.4%]	3

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)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	3.8%	[2.0%, 5.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

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)	7.8%	[7.8%, 7.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.719s -> 667.744s (0.00%)
Artifact size: 308.26 MiB -> 308.24 MiB (-0.01%)

pnkfelix · 2024-01-16T16:13:03Z

This impacted 3 variants of unicode-normalization-0.1.19: debug incr-unchanged and check {incr-unchanged, incr-patched:println}.
Interestingly, during two different try runs, those three variants were found to have improved by similar amounts by this PR.
there's some weird interaction between that benchmark and the code paths impacted by this PR, and I do not think its worth investing effort in further investigation.
marking as triaged.

@rustbot label: +perf-regression-triaged

Nadrieril added the F-never_patterns `#![feature(never_patterns)]` label Jan 5, 2024

rustbot assigned compiler-errors Jan 5, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 5, 2024