thir::pattern: update some comments and error type names by RalfJung · Pull Request #115887 · rust-lang/rust

RalfJung · 2023-09-16T09:34:35Z

Follow-up to these comments. Please carefully fact-check, I'm new to this area of the compiler!

rustbot · 2023-09-16T09:34:42Z

r? @jackh726

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

rustbot · 2023-09-16T09:34:44Z

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Nadrieril · 2023-09-16T11:47:55Z

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

                self.saw_const_match_error.set(true);
                let err = TypeNotStructural { span, non_sm_ty: ty };
                tcx.sess.emit_err(err);
+                // We errored, so the pattern we generate is irrelevant.


That comment was much needed. This always bothered me because if we forgot to error this would cause unsoundness (non exhaustive match considered exhaustive)

Yeah I'd much prefer something that ICEs later... in particular in that one case where we don't actually emit the error, we check that the flag says that we emitted an error earlier.

In fact, let me add a delay_span_bug in that case just to be sure.

Nadrieril · 2023-09-17T19:47:46Z

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


                        // FIXME: introduce PatKind::Error to silence follow up diagnostics due to unreachable patterns.
+                        // We errored, so the pattern we generate is irrelevant.
                        PatKind::Wild


Should this one set saw_const_match_error too?

Hm, it doesn't check for saw_const_match_error either. But I guess yes? Not sure. This PR was not meant to have any behavior changes in it, just comments and clarifications.

Ah fair; I was just thinking that should also trigger a delay_span_bug

The emit_err is directly above here so theres'a definitely a hard error. In the other case where I added a delay_span_bug, the emit_err is conditional on something, so it becomes a non-local property, and I felt it was worth simplifying the reasoning, so that the failure mode for if saw_const_match_error gets set incorrectly is an ICE rather than a weird miscompilation.

RalfJung · 2023-09-18T05:41:00Z

r? @oli-obk
AFAIK you wrote (most of) this code so this probably makes sense.

oli-obk · 2023-09-27T10:42:27Z

Marking as blocked on #115937 (as some of the comments don't apply anymore afterwards)

oli-obk · 2023-09-27T10:42:45Z

r=me after that

RalfJung · 2023-09-27T11:42:48Z

Hm, I was thinking to land this first to document the status quo, and then #115937 would update the comments. But in the end I don't care strongly, I mostly wanted to get confirmation that the comments here are correct, in particular around FallbackToConstRef being completely outdated (its use has nothing to do with references any more: we use it even without references, and we don't use it with all references).

oli-obk · 2023-09-27T11:45:02Z

@bors r+ rollup

wfm

I mostly wanted to get confirmation that the comments here are correct, in particular around FallbackToConstRef being completely outdated (its use has nothing to do with references any more: we use it even without references, and we don't use it with all references).

Yea, I just started reusing it without adjusting the name

bors · 2023-09-27T11:45:04Z

📌 Commit 12a1b55 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-09-27T13:20:56Z

⌛ Testing commit 12a1b55 with merge c4ce33c...

bors · 2023-09-27T15:08:35Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c4ce33c to master...

rust-timer · 2023-09-27T18:00:15Z

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

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

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.4%	[2.4%, 2.5%]	2
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Binary size

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

Bootstrap: 632.158s -> 632.345s (0.03%)
Artifact size: 317.25 MiB -> 317.20 MiB (-0.02%)

rustbot assigned jackh726 Sep 16, 2023

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 Sep 16, 2023

This comment has been minimized.

Sign in to view

thir::pattern: update some comments and error type names

d34e15e

RalfJung force-pushed the pat branch from cebb338 to d34e15e Compare September 16, 2023 10:25

Nadrieril reviewed Sep 16, 2023

View reviewed changes

Nadrieril reviewed Sep 17, 2023

View reviewed changes

rustbot assigned oli-obk and unassigned jackh726 Sep 18, 2023

better ICE than sorry

12a1b55

RalfJung force-pushed the pat branch from 5753e59 to 12a1b55 Compare September 18, 2023 05:42

oli-obk added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2023

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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 27, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2023

bors merged commit c4ce33c into rust-lang:master Sep 27, 2023

rustbot added this to the 1.74.0 milestone Sep 27, 2023

bors mentioned this pull request Sep 27, 2023

Prevent spurious unreachable pattern lints #115937

Merged

RalfJung deleted the pat branch September 30, 2023 20:27

Uh oh!

Conversation

RalfJung commented Sep 16, 2023

Uh oh!

rustbot commented Sep 16, 2023

Uh oh!

rustbot commented Sep 16, 2023

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Sep 18, 2023

Uh oh!

oli-obk commented Sep 27, 2023

Uh oh!

oli-obk commented Sep 27, 2023

Uh oh!

RalfJung commented Sep 27, 2023

Uh oh!

oli-obk commented Sep 27, 2023

Uh oh!

bors commented Sep 27, 2023

Uh oh!

bors commented Sep 27, 2023

Uh oh!

bors commented Sep 27, 2023

Uh oh!

rust-timer commented Sep 27, 2023

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

RalfJung Sep 17, 2023 •

edited

Loading