-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Change unreachable pattern ICEs to warnings #39127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1c51f60
dd13cc3
cc0d63c
c07aa33
e7e7c36
ef9f0ab
0aad529
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ use util::nodemap::{NodeMap, FxHashMap}; | |
use std::collections::BTreeMap; | ||
use std::iter; | ||
|
||
use syntax::attr; | ||
use syntax::ast::*; | ||
use syntax::errors; | ||
use syntax::ptr::P; | ||
|
@@ -1814,7 +1815,8 @@ impl<'a> LoweringContext<'a> { | |
let match_expr = P(self.expr_match(e.span, | ||
into_iter_expr, | ||
hir_vec![iter_arm], | ||
hir::MatchSource::ForLoopDesugar)); | ||
hir::MatchSource::ForLoopDesugar, | ||
ThinVec::new())); | ||
|
||
// `{ let _result = ...; _result }` | ||
// underscore prevents an unused_variables lint if the head diverges | ||
|
@@ -1833,8 +1835,12 @@ impl<'a> LoweringContext<'a> { | |
ExprKind::Try(ref sub_expr) => { | ||
// to: | ||
// | ||
// #[allow(unreachable_patterns)] | ||
// match Carrier::translate(<expr>) { | ||
// Ok(val) => val, | ||
// Ok(val) => { | ||
// #[allow(unreachable_code)] | ||
// val | ||
// } | ||
// Err(err) => return Carrier::from_error(From::from(err)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: update the comment here (including the absence of |
||
// } | ||
let unstable_span = self.allow_internal_unstable("?", e.span); | ||
|
@@ -1849,14 +1855,34 @@ impl<'a> LoweringContext<'a> { | |
P(self.expr_call(e.span, path, hir_vec![sub_expr])) | ||
}; | ||
|
||
// Ok(val) => val | ||
// Ok(val) => { #[allow(unreachable_code)] val } | ||
let ok_arm = { | ||
let val_ident = self.str_to_ident("val"); | ||
let val_pat = self.pat_ident(e.span, val_ident); | ||
let val_expr = P(self.expr_ident(e.span, val_ident, val_pat.id)); | ||
// #[allow(unreachable_code)] | ||
let val_attr = { | ||
// allow(unreachable_code) | ||
let allow = { | ||
let allow_ident = self.str_to_ident("allow"); | ||
let uc_ident = self.str_to_ident("unreachable_code"); | ||
let uc_meta_item = attr::mk_spanned_word_item(e.span, uc_ident); | ||
let uc_nested = NestedMetaItemKind::MetaItem(uc_meta_item); | ||
let uc_spanned = respan(e.span, uc_nested); | ||
attr::mk_spanned_list_item(e.span, allow_ident, vec![uc_spanned]) | ||
}; | ||
attr::mk_spanned_attr_outer(e.span, attr::mk_attr_id(), allow) | ||
}; | ||
let attrs = From::from(vec![val_attr]); | ||
let val_expr = P(self.expr_ident_with_attrs(e.span, | ||
val_ident, | ||
val_pat.id, | ||
attrs)); | ||
let val_block = P(self.block_expr(val_expr)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No.. for some reason I thought we did. |
||
let ok_expr = P(self.expr_block(val_block, ThinVec::new())); | ||
|
||
let ok_pat = self.pat_ok(e.span, val_pat); | ||
|
||
self.arm(hir_vec![ok_pat], val_expr) | ||
self.arm(hir_vec![ok_pat], ok_expr) | ||
}; | ||
|
||
// Err(err) => return Carrier::from_error(From::from(err)) | ||
|
@@ -1885,8 +1911,23 @@ impl<'a> LoweringContext<'a> { | |
self.arm(hir_vec![err_pat], ret_expr) | ||
}; | ||
|
||
// #[allow(unreachable_patterns)] | ||
let match_attr = { | ||
// allow(unreachable_patterns) | ||
let allow = { | ||
let allow_ident = self.str_to_ident("allow"); | ||
let up_ident = self.str_to_ident("unreachable_patterns"); | ||
let up_meta_item = attr::mk_spanned_word_item(e.span, up_ident); | ||
let up_nested = NestedMetaItemKind::MetaItem(up_meta_item); | ||
let up_spanned = respan(e.span, up_nested); | ||
attr::mk_spanned_list_item(e.span, allow_ident, vec![up_spanned]) | ||
}; | ||
attr::mk_spanned_attr_outer(e.span, attr::mk_attr_id(), allow) | ||
}; | ||
|
||
let attrs = From::from(vec![match_attr]); | ||
return self.expr_match(e.span, discr, hir_vec![err_arm, ok_arm], | ||
hir::MatchSource::TryDesugar); | ||
hir::MatchSource::TryDesugar, attrs); | ||
} | ||
|
||
ExprKind::Mac(_) => panic!("Shouldn't exist here"), | ||
|
@@ -2031,6 +2072,13 @@ impl<'a> LoweringContext<'a> { | |
} | ||
|
||
fn expr_ident(&mut self, span: Span, id: Name, binding: NodeId) -> hir::Expr { | ||
self.expr_ident_with_attrs(span, id, binding, ThinVec::new()) | ||
} | ||
|
||
fn expr_ident_with_attrs(&mut self, span: Span, | ||
id: Name, | ||
binding: NodeId, | ||
attrs: ThinVec<Attribute>) -> hir::Expr { | ||
let def = { | ||
let defs = self.resolver.definitions(); | ||
Def::Local(defs.local_def_id(binding)) | ||
|
@@ -2042,7 +2090,7 @@ impl<'a> LoweringContext<'a> { | |
segments: hir_vec![hir::PathSegment::from_name(id)], | ||
}))); | ||
|
||
self.expr(span, expr_path, ThinVec::new()) | ||
self.expr(span, expr_path, attrs) | ||
} | ||
|
||
fn expr_mut_addr_of(&mut self, span: Span, e: P<hir::Expr>) -> hir::Expr { | ||
|
@@ -2062,9 +2110,10 @@ impl<'a> LoweringContext<'a> { | |
span: Span, | ||
arg: P<hir::Expr>, | ||
arms: hir::HirVec<hir::Arm>, | ||
source: hir::MatchSource) | ||
source: hir::MatchSource, | ||
attrs: ThinVec<Attribute>) | ||
-> hir::Expr { | ||
self.expr(span, hir::ExprMatch(arg, arms, source), ThinVec::new()) | ||
self.expr(span, hir::ExprMatch(arg, arms, source), attrs) | ||
} | ||
|
||
fn expr_block(&mut self, b: P<hir::Block>, attrs: ThinVec<Attribute>) -> hir::Expr { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,14 +308,7 @@ fn check_arms<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>, | |
.emit(); | ||
}, | ||
|
||
hir::MatchSource::ForLoopDesugar => { | ||
// this is a bug, because on `match iter.next()` we cover | ||
// `Some(<head>)` and `None`. It's impossible to have an unreachable | ||
// pattern | ||
// (see libsyntax/ext/expand.rs for the full expansion of a for loop) | ||
span_bug!(pat.span, "unreachable for-loop pattern") | ||
}, | ||
|
||
hir::MatchSource::ForLoopDesugar | | ||
hir::MatchSource::Normal => { | ||
let mut diagnostic = Diagnostic::new(Level::Warning, | ||
"unreachable pattern"); | ||
|
@@ -329,9 +322,7 @@ fn check_arms<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>, | |
hir_pat.id, diagnostic); | ||
}, | ||
|
||
hir::MatchSource::TryDesugar => { | ||
span_bug!(pat.span, "unreachable try pattern") | ||
}, | ||
hir::MatchSource::TryDesugar => {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
Useful => (), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
//#![feature(never_type)] | ||
|
||
struct R<'a> { | ||
r: &'a R<'a>, | ||
} | ||
|
||
fn foo(res: Result<u32, &R>) -> u32 { | ||
let Ok(x) = res; | ||
//~^ ERROR refutable pattern | ||
x | ||
} | ||
|
||
fn main() { | ||
foo(Ok(23)); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
#![feature(never_type)] | ||
#![deny(unreachable_patterns)] | ||
|
||
fn main() { | ||
let x: &[!] = &[]; | ||
|
||
for _ in x {} | ||
//~^ ERROR unreachable pattern | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT | ||
// file at the top-level directory of this distribution and at | ||
// http://rust-lang.org/COPYRIGHT. | ||
// | ||
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
#![feature(never_type)] | ||
#![deny(unreachable_code)] | ||
#![deny(unreachable_patterns)] | ||
|
||
fn bar(x: Result<!, i32>) -> Result<u32, i32> { | ||
x? | ||
} | ||
|
||
fn main() { | ||
let _ = bar(Err(123)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 innermatch
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll need to look into this :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you key things directly on the
MatchSource
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
unreachable_code
attribute is indeed fine - no user-defined code exists under it. It's theunreachable_patterns
attribute that is the problem - could you check why is it needed?