Change unreachable pattern ICEs to warnings #39127
Conversation
Allow code with unreachable `?` and `for` patterns to compile. Add some tests.
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
|
Ugh, bloody make tidy. Every single time. |
|
So to summarise: An unreachable pattern in a |
|
cc @rust-lang/compiler |
| @@ -1833,8 +1835,12 @@ impl<'a> LoweringContext<'a> { | |||
| ExprKind::Try(ref sub_expr) => { | |||
| // to: | |||
| // | |||
| // #[allow(unreachable_patterns)] | |||
There was a problem hiding this comment.
Hmm, so, this #[allow(unreachable_patterns)] annotation presumably affects the <expr>, right? That seems unfortunate. i.e., if I do (match { ... })?, do we still see warnings from the inner match?
There was a problem hiding this comment.
Good point, I'll need to look into this :/
There was a problem hiding this comment.
Can't you key things directly on the MatchSource?
There was a problem hiding this comment.
I still seem to get warnings without these. At least the unreachable_code attribute seems to have an effect. I was thinking I'll change the code to this:
match Carrier::translate(<expr>) {
#[allow(unused)]
Ok(val) => val,
#[allow(unused)]
Err(err) => return Carrier::from_error(From::from(err)),
}There was a problem hiding this comment.
The unreachable_code attribute is indeed fine - no user-defined code exists under it. It's the unreachable_patterns attribute that is the problem - could you check why is it needed?
|
r? @nikomatsakis (I'm assuming @arielb1 doesn't have time) |
|
Wait, do expr-level |
|
I do have time. |
|
@arielb1 ok :) either way. In any case, I think we both agree that we should try to avoid having warnings get squelched in user code somehow. I agree with the suggestion of using the |
|
The PR is required. #39151 just gates the uninhabitableness of references, it does not revert the changes for matching |
| val_ident, | ||
| val_pat.id, | ||
| attrs)); | ||
| let val_block = P(self.block_expr(val_expr)); |
There was a problem hiding this comment.
Do we need the block_expr/expr_block pair?
There was a problem hiding this comment.
No.. for some reason I thought we did.
|
Sorry for spamming travis. I've now changed the try desugaring to this: match Carrier::translate(<expr>) {
Ok(val) => #[allow(unreachable_code)] val,
Err(err) => #[allow(unreachable_code)] return Carrier::from_error(From::from(err)),
}That seems to be all we need to crush the warnings. |
| // Ok(val) => { | ||
| // #[allow(unreachable_code)] | ||
| // val | ||
| // } | ||
| // Err(err) => return Carrier::from_error(From::from(err)) |
There was a problem hiding this comment.
nit: update the comment here (including the absence of BlockExpr/ExprBlock pair.
| let val_expr = P(self.expr_ident_with_attrs(e.span, | ||
| val_ident, | ||
| val_pat.id, | ||
| From::from(attrs.clone()))); |
There was a problem hiding this comment.
I prefer ThinVec::from, but whatever you want.
| hir::MatchSource::TryDesugar => { | ||
| span_bug!(pat.span, "unreachable try pattern") | ||
| }, | ||
| hir::MatchSource::TryDesugar => {} |
There was a problem hiding this comment.
A comment would be nice, for example:
// unreachable patterns in a try expression occur when one of the Result
// variants is uninhabited, and should cause a warning.
|
@bors r+ |
|
📌 Commit 0aad529 has been approved by |
…ngs, r=arielb1 Change unreachable pattern ICEs to warnings Allow code with unreachable `?` and `for` patterns to compile. Add some tests.
|
☀️ Test successful - status-appveyor, status-travis |
Allow code with unreachable
?andforpatterns to compile.Add some tests.