Skip to content
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

Change unreachable pattern ICEs to warnings #39127

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 58 additions & 9 deletions src/librustc/hir/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -1833,8 +1835,12 @@ impl<'a> LoweringContext<'a> {
ExprKind::Try(ref sub_expr) => {
// to:
//
// #[allow(unreachable_patterns)]
Copy link
Contributor

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 inner match?

Copy link
Contributor Author

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 :/

Copy link
Contributor

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?

Copy link
Contributor Author

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:

match Carrier::translate(<expr>) {
    #[allow(unused)]
    Ok(val) => val,
    #[allow(unused)]
    Err(err) => return Carrier::from_error(From::from(err)),
}

Copy link
Contributor

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 the unreachable_patterns attribute that is the problem - could you check why is it needed?

// match Carrier::translate(<expr>) {
// Ok(val) => val,
// Ok(val) => {
// #[allow(unreachable_code)]
// val
// }
// Err(err) => return Carrier::from_error(From::from(err))
Copy link
Contributor

@arielb1 arielb1 Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update the comment here (including the absence of BlockExpr/ExprBlock pair.

// }
let unstable_span = self.allow_internal_unstable("?", e.span);
Expand All @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the block_expr/expr_block pair?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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))
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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))
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
13 changes: 2 additions & 11 deletions src/librustc_const_eval/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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 => {}
Copy link
Contributor

@arielb1 arielb1 Jan 20, 2017

Choose a reason for hiding this comment

The 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 => (),
Expand Down
26 changes: 26 additions & 0 deletions src/test/compile-fail/recursive-types-are-not-uninhabited.rs
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));
}

20 changes: 20 additions & 0 deletions src/test/compile-fail/unreachable-loop-patterns.rs
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
}

22 changes: 22 additions & 0 deletions src/test/run-pass/unreachable-try-pattern.rs
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));
}