Check pattern refutability on THIR #108504

cjgillot · 2023-02-26T21:50:41Z

The current check_match query is based on HIR, but partially re-lowers HIR into THIR.
This PR proposed to use the results of the thir_body query to check matches, instead of re-building THIR.

Most of the diagnostic changes are spans getting shorter, or commas/semicolons not getting removed.

This PR degrades the diagnostic for confusing constants in patterns (let A = foo() where A resolves to a const A somewhere): it does not point ot the definition of const A any more.

cjgillot · 2023-02-27T21:39:35Z

@bors try @rust-timer queue

bors · 2023-02-27T21:39:51Z

⌛ Trying commit 0e1d9e82c70ffb828f26152e8b76ed3f8669c6ac with merge 61b7e245cc6359c50ab2968b978381ec8a9716ff...

bors · 2023-02-28T00:30:14Z

☀️ Try build successful - checks-actions
Build commit: 61b7e245cc6359c50ab2968b978381ec8a9716ff (61b7e245cc6359c50ab2968b978381ec8a9716ff)

bors · 2023-02-28T00:30:14Z

☀️ Try build successful - checks-actions
Build commit: 61b7e245cc6359c50ab2968b978381ec8a9716ff (61b7e245cc6359c50ab2968b978381ec8a9716ff)

rust-timer · 2023-02-28T01:47:53Z

Finished benchmarking commit (61b7e245cc6359c50ab2968b978381ec8a9716ff): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.6%	[0.6%, 0.7%]	3
Regressions ❌ (secondary)	0.3%	[0.3%, 0.5%]	7
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-2.9%	[-3.1%, -2.7%]	6
All ❌✅ (primary)	0.3%	[-0.6%, 0.7%]	4

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.6%	[0.7%, 2.8%]	17
Regressions ❌ (secondary)	3.4%	[3.1%, 3.7%]	2
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[-2.4%, 2.8%]	18

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

cjgillot · 2023-02-28T07:20:28Z

r? compiler

Noratrieb

That's some great cleanup, thanks! I especially love how some of the diagnostics got better, like how let None = Some(()); now suggests if let when it didn't previously.

Noratrieb · 2023-03-01T19:05:47Z

compiler/rustc_mir_build/src/build/mod.rs

@@ -61,6 +61,7 @@ fn mir_build(tcx: TyCtxt<'_>, def: ty::WithOptConstParam<LocalDefId>) -> Body<'_
        ty::WithOptConstParam { did, const_param_did: None } => {
            tcx.ensure().thir_check_unsafety(did);
            drop(tcx.thir_abstract_const(did));
+            tcx.ensure().check_match(did);


I don't quite understand this change. Is this needed now because the THIR might already be stolen for bodies that required const eval? Why is this not also done for the Some case, is it just unnecessary or would something break?

check_match only fetches thir_body(ty::WithOptConstParam::unknown(did)), so we only need to ensure if we are stealing this exact one.

Makes sense, const args can't contain patterns. Is the loop over all body owners in rustc_interface still needed then? It seems redundant (except maybe for time passes niceness)

AnonConsts can definitely contain patterns to check. If we want to get rid of the rustc_interface loop, we'd need to force check_match in both cases, with a WithOptConstParam as argument. Like we do for thir_check_unsafety. I tried to avoid writing all the boilerplate just to remove it once #96840 lands.

Ah right, makes sense. Seems fine to keep it as-is then

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

Noratrieb · 2023-03-01T20:51:03Z

compiler/rustc_middle/src/dep_graph/mod.rs

-    fn dep_kind_info(&self, dep_kind: DepKind) -> &DepKindStruct<'tcx> {
-        &self.query_kinds[dep_kind as usize]
+    fn dep_kind_info(&self, dk: DepKind) -> &DepKindStruct<'tcx> {
+        &self.query_kinds[dk as usize]


Why was this necessary?

There is a DepKind::dep_kind variant. The former version did not handle hygiene correctly, so did not fire in case of macro expansion. The new version just ignores hygiene, so fires. (In that case, it's actually the more correct behaviour.)

Noratrieb · 2023-03-02T21:07:10Z

Looks good to me, but I'm not familiar enough with the code to approve it.
r? compiler

michaelwoerister · 2023-03-03T11:03:05Z

Same here.
r? compiler

michaelwoerister · 2023-03-03T11:03:43Z

You should give that another try, rustbot 😉
r? compiler

cjgillot · 2023-04-03T16:15:32Z

I had to adapt the handling of the non_exhaustive_omitted_patterns lint.

Noratrieb · 2023-04-06T12:05:21Z

@bors r=compiler-errors,Nilstrieb

bors · 2023-04-06T12:05:24Z

📌 Commit 1dde34b has been approved by compiler-errors,Nilstrieb

It is now in the queue for this repository.

bors · 2023-04-06T12:42:05Z

⌛ Testing commit 1dde34b with merge 0534655...

bors · 2023-04-06T14:57:04Z

☀️ Test successful - checks-actions
Approved by: compiler-errors,Nilstrieb
Pushing 0534655 to master...

rust-timer · 2023-04-06T16:22:27Z

Finished benchmarking commit (0534655): 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
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	3
Improvements ✅ (primary)	-0.4%	[-0.5%, -0.2%]	4
Improvements ✅ (secondary)	-3.0%	[-3.2%, -2.8%]	6
All ❌✅ (primary)	-0.4%	[-0.5%, -0.2%]	4

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.8%	[0.9%, 3.8%]	14
Regressions ❌ (secondary)	3.2%	[1.1%, 5.1%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[0.9%, 3.8%]	14

Cycles

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

rylev · 2023-04-19T08:06:24Z

Marking as triaged since the improvements outweigh the regressions and the regressions are small and in secondary benchmarks.

@rustbot label: +perf-regression-triaged

…er-errors Remove wrong assertion in match checking. This assertions is completely misguided, introduced by rust-lang#108504. The responsible PR is on beta, nominating for backport. Instead of checking that this is not a `&&`, it would make sense to check that neither arms of that `&&` is a `let`. This seems like a lot of code for unclear benefit. r? `@saethlin`

…=eholk Consider lint check attributes on match arms Currently, lint check attributes on match arms have no effect for some lints. This PR makes some lint passes to take those attributes into account. - `LateContextAndPass` for late lint doesn't update `last_node_with_lint_attrs` when it visits match arms. This leads to lint check attributes on match arms taking no effects on late lints that operate on the arms' pattern: ```rust match value { #[deny(non_snake_case)] PAT => {} // `non_snake_case` only warned due to default lint level } ``` To be honest, I'm not sure whether this is intentional or just an oversight. I've dug the implementation history and searched up issues/PRs but couldn't find any discussion on this. - `MatchVisitor` doesn't update its lint level when it visits match arms. This leads to check lint attributes on match arms taking no effect on some lints handled by this visitor, namely: `bindings_with_variant_name` and `irrefutable_let_patterns`. This seems to be a fallout from rust-lang#108504. Before 05082f5, when the visitor operated on HIR rather than THIR, check lint attributes for the said lints were effective. [This playground][play] compiles successfully on current stable (1.69) but fails on current beta and nightly. I wasn't sure where best to place the test for this. Let me know if there's a better place. [play]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=38432b79e535cb175f8f7d6d236d29c3 [play-match]: https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=629aa71b7c84b269beadeba664e2221d

This comment has been minimized.

Sign in to view

cjgillot force-pushed the thir-pattern branch from 808031f to 9f7d761 Compare February 27, 2023 18:13

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 27, 2023

cjgillot force-pushed the thir-pattern branch 2 times, most recently from 197249f to 0e1d9e8 Compare February 27, 2023 20:57

cjgillot changed the title ~~WIP: Check pattern refutability on THIR~~ Check pattern refutability on THIR Feb 27, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 27, 2023

cjgillot marked this pull request as ready for review February 27, 2023 21:40

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 28, 2023

rustbot assigned Noratrieb Feb 28, 2023

cjgillot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2023

Noratrieb reviewed Mar 1, 2023

View reviewed changes

cjgillot force-pushed the thir-pattern branch from 0e1d9e8 to d12890f Compare March 1, 2023 21:43

rustbot assigned michaelwoerister and unassigned Noratrieb Mar 2, 2023

rustbot assigned Noratrieb and unassigned michaelwoerister Mar 3, 2023

cjgillot added 2 commits April 3, 2023 15:59

Expand parameters.

03fbb3d

Only emit lint on refutable patterns.

03a6ef6

cjgillot force-pushed the thir-pattern branch from 76ec488 to b7c0599 Compare April 3, 2023 16:14

cjgillot added 2 commits April 3, 2023 16:16

Rename hir_id to lint_root.

a8187f8

Bless ui test.

1dde34b

cjgillot force-pushed the thir-pattern branch from b7c0599 to 1dde34b Compare April 3, 2023 16:16

Noratrieb approved these changes Apr 6, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2023

bors merged commit 0534655 into rust-lang:master Apr 6, 2023

rustbot added this to the 1.70.0 milestone Apr 6, 2023

This was referenced Apr 6, 2023

Report all lints, even if other errors already occurred. #108813

Closed

Migrate rustc_mir_build's non_exhaustive_match diagnostic #108278

Closed

cjgillot deleted the thir-pattern branch April 6, 2023 15:18

bors mentioned this pull request Apr 6, 2023

Always fall back to PartialEq when a constant in a pattern is not recursively structural-eq #105750

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 19, 2023

cjgillot mentioned this pull request Apr 30, 2023

Remove wrong assertion in match checking. #111015

Merged

lowr mentioned this pull request May 19, 2023

Consider lint check attributes on match arms #111757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check pattern refutability on THIR #108504

Check pattern refutability on THIR #108504

cjgillot commented Feb 26, 2023 •

edited

Loading

This comment has been minimized.

cjgillot commented Feb 27, 2023

This comment has been minimized.

bors commented Feb 27, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

This comment has been minimized.

rust-timer commented Feb 28, 2023

cjgillot commented Feb 28, 2023

Noratrieb left a comment

Noratrieb Mar 1, 2023

cjgillot Mar 1, 2023

Noratrieb Mar 1, 2023 •

edited

Loading

cjgillot Mar 1, 2023

Noratrieb Mar 1, 2023

Noratrieb Mar 1, 2023

cjgillot Mar 1, 2023

Noratrieb commented Mar 2, 2023

michaelwoerister commented Mar 3, 2023

michaelwoerister commented Mar 3, 2023

cjgillot commented Apr 3, 2023

Noratrieb commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

rust-timer commented Apr 6, 2023

rylev commented Apr 19, 2023

Check pattern refutability on THIR #108504

Check pattern refutability on THIR #108504

Conversation

cjgillot commented Feb 26, 2023 • edited Loading

This comment has been minimized.

cjgillot commented Feb 27, 2023

This comment has been minimized.

bors commented Feb 27, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

This comment has been minimized.

rust-timer commented Feb 28, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

cjgillot commented Feb 28, 2023

Noratrieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Mar 2, 2023

michaelwoerister commented Mar 3, 2023

michaelwoerister commented Mar 3, 2023

cjgillot commented Apr 3, 2023

Noratrieb commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

rust-timer commented Apr 6, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

rylev commented Apr 19, 2023

cjgillot commented Feb 26, 2023 •

edited

Loading

Noratrieb Mar 1, 2023 •

edited

Loading