never_patterns: Parse match arms with no body

[Nadrieril]

Never patterns are meant to signal unreachable cases, and thus don't take bodies:

let ptr: *const Option<!> = ...;
match *ptr {
    None => { foo(); }
    Some(!),
}

This PR makes rustc accept the above, and enforces that an arm has a body xor is a never pattern. This affects parsing of match arms even with the feature off, so this is delicate. (Plus this is my first non-trivial change to the parser).

The last commit is optional; it introduces a bit of churn to allow the new suggestions to be machine-applicable. There may be a better solution? I'm not sure. EDIT: I removed that commit

r? @compiler-errors

[Nadrieril]

(only the last commit changes to the size of AST and/or HIR nodes)

[Nadrieril]

Ok giving up on that last commit, I'll find a better solution later

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

[calebcartwright]

@compiler-errors do you think this (or perhaps the associated tracking issue/rfc) would be worth t-style discussion?

[compiler-errors]

@calebcartwright: I will I-style-nominate the tracking issue (if that doesn't exist, I'll add it).

@bors r+ rollup=never

[bors]

📌 Commit 431cc4a has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 431cc4a with merge 2b399b5...

[bors]

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

[rust-timer]

Finished benchmarking commit (2b399b5): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.0%, -0.8%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-1.0%, 0.1%]	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)	1.7%	[1.7%, 1.7%]	1
Regressions ❌ (secondary)	3.0%	[2.1%, 4.6%]	3
Improvements ✅ (primary)	-0.9%	[-1.4%, -0.4%]	2
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	-0.0%	[-1.4%, 1.7%]	3

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

Binary size

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

Bootstrap: 675.635s -> 674.909s (-0.11%)
Artifact size: 314.02 MiB -> 314.05 MiB (0.01%)

[Mark-Simulacrum]

This affects parsing of match arms even with the feature off, so this is delicate.

Is this change FCP'd or nightly-only? It sounds like this is adding new syntax to the language -- is there a lang sign-off on that somewhere?

(I'm surprised this landed without that).

[Mark-Simulacrum]

Perf regression looks to be genuine but likely not worth investigating, limited to single primary incr-unchanged scenario.

[Nadrieril]

It sounds like this is adding new syntax to the language -- is there a lang sign-off on that somewhere?

It's not adding syntax to stable rust. What's different is that now the parser accepts match arms with no body and they're rejected later, instead of them being rejected by the parser directly.

[petrochenkov]

What's different is that now the parser accepts match arms with no body and they're rejected later, instead of them being rejected by the parser directly.

The difference is that

match *ptr {
    None => { foo(); }
    #[cfg(FALSE)]
    Some(!),
}

will compile on stable.

Any unstable syntax needs to be feature-gated at parser level using gated_spans.

[Nadrieril]

Oh shoot yeah I didn't consider that. I knew there would be a subtlety. To be precise,

match *ptr {
    None => { foo(); }
    #[cfg(FALSE)]
    Some(!),
}

does not compile on stable because ! is correctly gated at parser level: https://github.com/rust-lang/rust/blob/master/tests/ui/feature-gates/feature-gate-never_patterns.rs

However this now compiles on stable:

match Some(0) {
    None => { foo(); }
    #[cfg(FALSE)]
    Some(_)
}

which is much worse.

[Nadrieril]

I don't want to gate a token here though, I want to give a user a normal parse error. Is there a way in the parser to know which feature gates are active? This may not even make sense if we aren't done parsing though...

[Nadrieril]

Fix is up: #118868