-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Better typeck divergence #50262
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
Better typeck divergence #50262
Changes from all commits
969542c
2e9b296
02f3b8f
39313bb
e61cccf
ca5afd8
2e23d8d
d485cc4
9d4b466
30b1942
a778a73
e7dc075
c07fcd8
3daf17c
38c3e0d
b80a490
5efa7c9
b9ea715
168d2a3
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 |
---|---|---|
|
@@ -270,7 +270,7 @@ pub fn create_function_debug_context<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, | |
} | ||
None => {} | ||
}; | ||
if cx.layout_of(sig.output()).abi == ty::layout::Abi::Uninhabited { | ||
if sig.output().conservative_is_uninhabited() { | ||
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. Tell LLVM that functions that return uninhabited types can never return |
||
flags = flags | DIFlags::FlagNoReturn; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1083,8 +1083,7 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> { | |
} | ||
} | ||
None => { | ||
// FIXME(canndrew): This is_never should probably be an is_uninhabited | ||
if !sig.output().is_never() { | ||
if !sig.output().conservative_is_uninhabited() { | ||
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. seems ok 👍 |
||
span_mirbug!(self, term, "call to converging function {:?} w/o dest", sig); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -213,8 +213,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |
exit_block.unit() | ||
} | ||
ExprKind::Call { ty, fun, args } => { | ||
// FIXME(canndrew): This is_never should probably be an is_uninhabited | ||
let diverges = expr.ty.is_never(); | ||
let diverges = expr.ty.conservative_is_uninhabited(); | ||
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. seems safe 👍 |
||
let intrinsic = match ty.sty { | ||
ty::TyFnDef(def_id, _) => { | ||
let f = ty.fn_sig(this.hir.tcx()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -227,7 +227,11 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> { | |
let scrutinee_is_uninhabited = if self.tcx.features().exhaustive_patterns { | ||
self.tcx.is_ty_uninhabited_from(module, pat_ty) | ||
} else { | ||
self.conservative_is_uninhabited(pat_ty) | ||
match pat_ty.sty { | ||
ty::TyNever => true, | ||
ty::TyAdt(def, _) => def.variants.is_empty(), | ||
_ => false | ||
} | ||
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. Why this change here? 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. In other words, why not invoke 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. I think it would be identical? |
||
}; | ||
if !scrutinee_is_uninhabited { | ||
// We know the type is inhabited, so this must be wrong | ||
|
@@ -255,15 +259,6 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> { | |
}) | ||
} | ||
|
||
fn conservative_is_uninhabited(&self, scrutinee_ty: Ty<'tcx>) -> bool { | ||
// "rustc-1.0-style" uncontentious uninhabitableness check | ||
match scrutinee_ty.sty { | ||
ty::TyNever => true, | ||
ty::TyAdt(def, _) => def.variants.is_empty(), | ||
_ => false | ||
} | ||
} | ||
|
||
fn check_irrefutable(&self, pat: &'tcx Pat, origin: &str) { | ||
let module = self.tcx.hir.get_module_parent(pat.id); | ||
MatchCheckCtxt::create_and_enter(self.tcx, module, |ref mut cx| { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -434,6 +434,16 @@ pub enum PlaceOp { | |
/// wake). Tracked semi-automatically (through type variables marked | ||
/// as diverging), with some manual adjustments for control-flow | ||
/// primitives (approximating a CFG). | ||
/// | ||
/// We know a node diverges in the following (conservative) situations: | ||
/// - A function with a parameter whose type is uninhabited necessarily diverges. | ||
/// - A match expression with no arms necessarily diverges. | ||
/// - A match expression whose arms patterns all diverge necessarily diverges. | ||
/// - A match expression whose arms all diverge necessarily diverges. | ||
/// - An expression whose type is uninhabited necessarily diverges. | ||
/// | ||
/// In the above, the node will be marked as diverging `Always` or `WarnedAlways`. | ||
/// In any other situation, it will be marked as `Maybe`. | ||
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
pub enum Diverges { | ||
/// Potentially unknown, some cases converge, | ||
|
@@ -445,11 +455,18 @@ pub enum Diverges { | |
Always, | ||
|
||
/// Same as `Always` but with a reachability | ||
/// warning already emitted | ||
WarnedAlways | ||
/// warning already emitted. | ||
WarnedAlways, | ||
|
||
/// Same as `Always` but without a reachability | ||
/// warning emitted. Unlike `Always`, cannot be | ||
/// converted to `WarnedAlways`. Used when | ||
/// unreachable code is expected (e.g. in | ||
/// function parameters as part of trait impls). | ||
UnwarnedAlways, | ||
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. If I understand the role of this value, it is basically used to signal when the fn has "uninhabited" arguments -- but only if the body of the function looks "sufficiently trivial". Did we anticipate this having a "flow sensitive" role at some point? If not, I feel like it might be clearer to add a field like (Otherwise, it kind of messes with the lattice) |
||
} | ||
|
||
// Convenience impls for combinig `Diverges`. | ||
// Convenience impls for combining `Diverges`. | ||
|
||
impl ops::BitAnd for Diverges { | ||
type Output = Self; | ||
|
@@ -1047,6 +1064,24 @@ fn check_fn<'a, 'gcx, 'tcx>(inherited: &'a Inherited<'a, 'gcx, 'tcx>, | |
fcx.check_pat_walk(&arg.pat, arg_ty, | ||
ty::BindingMode::BindByValue(hir::Mutability::MutImmutable), true); | ||
|
||
// If any of a function's parameters have a type that is uninhabited, then it | ||
// it is not possible to call that function (because its arguments cannot be constructed). | ||
// Therefore, it must always diverge. | ||
if fcx.tcx.features().exhaustive_patterns { | ||
if arg_ty.conservative_is_uninhabited() { | ||
let mut diverges = Diverges::Always; | ||
if let hir::ExprKind::Block(ref block, _) = body.value.node { | ||
// If the function is completely empty, or has a single trailing | ||
// expression, then we do not issue a warning (as it was likely | ||
// mandated by a trait, rather than being an oversight). | ||
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. Hmm, these rules seem reasonable, but not perfect. For example, by these rules, the following would not warn: fn foo(x: !) {
{
println!("Hi"):
}
} but this would, right? fn foo(x: !) {
{
println!("Hi"):
}
} (What happens if you use 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. hmm... did you mean to write something else in the second snippet? They are identical... |
||
if block.stmts.is_empty() { | ||
diverges = Diverges::UnwarnedAlways; | ||
} | ||
} | ||
fcx.diverges.set(fcx.diverges.get() | diverges); | ||
} | ||
} | ||
|
||
// Check that argument is Sized. | ||
// The check for a non-trivial pattern is a hack to avoid duplicate warnings | ||
// for simple cases like `fn foo(x: Trait)`, | ||
|
@@ -3626,9 +3661,9 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |
/// that there are actually multiple representations for `TyError`, so avoid | ||
/// that when err needs to be handled differently. | ||
fn check_expr_with_expectation_and_needs(&self, | ||
expr: &'gcx hir::Expr, | ||
expected: Expectation<'tcx>, | ||
needs: Needs) -> Ty<'tcx> { | ||
expr: &'gcx hir::Expr, | ||
expected: Expectation<'tcx>, | ||
needs: Needs) -> Ty<'tcx> { | ||
debug!(">> typechecking: expr={:?} expected={:?}", | ||
expr, expected); | ||
|
||
|
@@ -3652,9 +3687,11 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |
_ => self.warn_if_unreachable(expr.id, expr.span, "expression") | ||
} | ||
|
||
// Any expression that produces a value of type `!` must have diverged | ||
if ty.is_never() { | ||
self.diverges.set(self.diverges.get() | Diverges::Always); | ||
// Any expression that produces a value of an uninhabited type must have diverged. | ||
if ty.conservative_is_uninhabited() { | ||
if ty.is_never() || self.tcx.features().exhaustive_patterns { | ||
self.diverges.set(self.diverges.get() | Diverges::Always); | ||
} | ||
} | ||
|
||
// Record the type, which applies it effects. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -310,7 +310,7 @@ impl Error for string::FromUtf16Error { | |
#[stable(feature = "str_parse_error2", since = "1.8.0")] | ||
impl Error for string::ParseError { | ||
fn description(&self) -> &str { | ||
match *self {} | ||
match self {} | ||
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. Why this change? Also, would this code compile on stable...? 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. 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. Well, I see that |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ enum Void {} | |
impl From<Void> for i32 { | ||
fn from(v: Void) -> i32 { | ||
match v {} | ||
//~^ WARN unreachable expression | ||
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. This is a bit surprising to me — I feel like this code should compile without warning. Matching with no arms feels like a great way (to me) to declare to the compiler "I am well aware this code is unreachable". 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. If not this code, what would you expect the user to write? |
||
} | ||
} | ||
|
||
|
@@ -39,6 +40,7 @@ fn qux(x: Result<u32, Void>) -> Result<u32, i32> { | |
fn vom(x: Result<u32, Void>) -> Result<u32, i32> { | ||
let y = (match x { Ok(n) => Ok(n), Err(e) => Err(e) })?; | ||
//~^ WARN unreachable pattern | ||
//~| WARN unreachable expression | ||
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. Similarly here, it feels unfortunate to start warning unless we have a nicer way to write this code (but, at present, we don't...?). If we had the match x {
Ok(n) => Ok(n),
Err(!)
} |
||
Ok(y) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,31 +15,20 @@ | |
// compile-flags:-g | ||
// gdb-command:run | ||
|
||
// gdb-command:print first | ||
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. Why did you remove the |
||
// gdb-command:print *first | ||
// gdbg-check:$1 = {<No data fields>} | ||
// gdbr-check:$1 = <error reading variable> | ||
|
||
// gdb-command:print second | ||
varkor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// gdbg-check:$2 = {<No data fields>} | ||
// gdbr-check:$2 = <error reading variable> | ||
|
||
#![allow(unused_variables)] | ||
#![feature(omit_gdb_pretty_printer_section)] | ||
#![omit_gdb_pretty_printer_section] | ||
|
||
enum ANilEnum {} | ||
enum AnotherNilEnum {} | ||
enum Void {} | ||
|
||
// This test relies on gdbg printing the string "{<No data fields>}" for empty | ||
// structs (which may change some time) | ||
// The error from gdbr is expected since nil enums are not supposed to exist. | ||
fn main() { | ||
unsafe { | ||
let first: ANilEnum = ::std::mem::zeroed(); | ||
let second: AnotherNilEnum = ::std::mem::zeroed(); | ||
let first: *const Void = 1 as *const _; | ||
|
||
zzz(); // #break | ||
} | ||
zzz(); // #break | ||
} | ||
|
||
fn zzz() {()} | ||
fn zzz() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Copyright 2018 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(exhaustive_patterns)] | ||
#![feature(never_type)] | ||
#![deny(unreachable_code)] | ||
|
||
pub enum Void {} | ||
|
||
pub fn uninhabited_parameter_i(_v: Void) { | ||
// A function with an uninhabited parameter | ||
// is permitted if its body is empty. | ||
} | ||
|
||
pub fn uninhabited_parameter_ii(v: !) -> i32 { | ||
// A function with an uninhabited parameter | ||
// is permitted if it simply returns a value | ||
// as a trailing expression to satisfy the | ||
// return type. | ||
v | ||
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. I guess this answers my question to "how should the user write this code" -- instead of But that will only work if the uninhabited parameter has type |
||
} | ||
|
||
pub fn uninhabited_parameter_iii(_v: Void, x: i32) -> i32 { | ||
println!("Call me if you can!"); //~^ ERROR unreachable expression | ||
x | ||
} | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
error: unreachable expression | ||
--> $DIR/better_divergence_checking.rs:30:59 | ||
| | ||
LL | pub fn uninhabited_parameter_iii(_v: Void, x: i32) -> i32 { | ||
| ___________________________________________________________^ | ||
LL | | println!("Call me if you can!"); //~^ ERROR unreachable expression | ||
LL | | x | ||
LL | | } | ||
| |_^ | ||
| | ||
note: lint level defined here | ||
--> $DIR/better_divergence_checking.rs:13:9 | ||
| | ||
LL | #![deny(unreachable_code)] | ||
| ^^^^^^^^^^^^^^^^ | ||
|
||
error: aborting due to previous error | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
error[E0658]: box expression syntax is experimental; you can call `Box::new` instead. (see issue #27779) | ||
--> $DIR/feature-gate-box-expr.rs:22:13 | ||
| | ||
LL | let x = box 'c'; //~ ERROR box expression syntax is experimental | ||
| ^^^^^^^ | ||
| | ||
= help: add #![feature(box_syntax)] to the crate attributes to enable | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0658`. |
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.
probably safe 👍