thir::pattern: update some comments and error type names #115887
Conversation
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes might have occurred in exhaustiveness checking cc @Nadrieril |
This comment has been minimized.
This comment has been minimized.
| PatKind::Wild | ||
| } | ||
| ty::Adt(adt_def, _) if !self.type_marked_structural(ty) => { | ||
| debug!("adt_def {:?} has !type_marked_structural for cv.ty: {:?}", adt_def, ty,); | ||
| 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. |
There was a problem hiding this comment.
That comment was much needed. This always bothered me because if we forgot to error this would cause unsoundness (non exhaustive match considered exhaustive)
There was a problem hiding this comment.
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.
| @@ -423,6 +425,7 @@ impl<'tcx> ConstToPat<'tcx> { | |||
| tcx.sess.emit_err(err); | |||
|
|
|||
| // FIXME: introduce PatKind::Error to silence follow up diagnostics due to unreachable patterns. | |||
| // We errored, so the pattern we generate is irrelevant. | |||
| PatKind::Wild | |||
There was a problem hiding this comment.
Should this one set saw_const_match_error too?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah fair; I was just thinking that should also trigger a delay_span_bug
There was a problem hiding this comment.
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.
|
r? @oli-obk |
|
Marking as blocked on #115937 (as some of the comments don't apply anymore afterwards) |
oli-obk
added
S-blocked
|
r=me after that |
|
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 |
|
@bors r+ rollup wfm
Yea, I just started reusing it without adjusting the name |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (c4ce33c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.158s -> 632.345s (0.03%) |
Follow-up to these comments. Please carefully fact-check, I'm new to this area of the compiler!