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

Better typeck divergence #50262

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
3 changes: 1 addition & 2 deletions src/librustc/cfg/construct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,8 +415,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
args: I) -> CFGIndex {
let func_or_rcvr_exit = self.expr(func_or_rcvr, pred);
let ret = self.straightline(call_expr, func_or_rcvr_exit, args);
// FIXME(canndrew): This is_never should probably be an is_uninhabited.
if self.tables.expr_ty(call_expr).is_never() {
if self.tables.expr_ty(call_expr).conservative_is_uninhabited() {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably safe 👍

self.add_unreachable_node()
} else {
ret
Expand Down
6 changes: 2 additions & 4 deletions src/librustc/middle/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1115,8 +1115,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
}

hir::ExprKind::Call(ref f, ref args) => {
// FIXME(canndrew): This is_never should really be an is_uninhabited
let succ = if self.tables.expr_ty(expr).is_never() {
let succ = if self.tables.expr_ty(expr).conservative_is_uninhabited() {
self.s.exit_ln
} else {
succ
Expand All @@ -1126,8 +1125,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
}

hir::ExprKind::MethodCall(.., ref args) => {
// FIXME(canndrew): This is_never should really be an is_uninhabited
let succ = if self.tables.expr_ty(expr).is_never() {
let succ = if self.tables.expr_ty(expr).conservative_is_uninhabited() {
self.s.exit_ln
} else {
succ
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,8 +645,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {

// If this error is due to `!: Trait` not implemented but `(): Trait` is
// implemented, and fallback has occured, then it could be due to a
// variable that used to fallback to `()` now falling back to `!`. Issue a
// note informing about the change in behaviour.
// variable that used to fallback to `()` now falling back to `!`.
// Issue a note informing about the change in behaviour.
if trait_predicate.skip_binder().self_ty().is_never()
&& fallback_has_occurred
{
Expand Down
9 changes: 9 additions & 0 deletions src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1481,6 +1481,15 @@ impl<'a, 'gcx, 'tcx> TyS<'tcx> {
}
}

pub fn conservative_is_uninhabited(&self) -> bool {
// Uncontentious uninhabitableness check
match self.sty {
ty::TyNever => true,
ty::TyAdt(def, _) => def.variants.is_empty(),
_ => false
}
}

pub fn is_primitive(&self) -> bool {
match self.sty {
TyBool | TyChar | TyInt(_) | TyUint(_) | TyFloat(_) => true,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}

Expand Down
3 changes: 1 addition & 2 deletions src/librustc_codegen_llvm/declare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
use llvm::{self, ValueRef};
use llvm::AttributePlace::Function;
use rustc::ty::{self, Ty};
use rustc::ty::layout::{self, LayoutOf};
use rustc::session::config::Sanitizer;
use rustc_target::spec::PanicStrategy;
use abi::{Abi, FnType, FnTypeExt};
Expand Down Expand Up @@ -134,7 +133,7 @@ pub fn declare_fn<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, name: &str,
let fty = FnType::new(cx, sig, &[]);
let llfn = declare_raw_fn(cx, name, fty.llvm_cconv(), fty.llvm_type(cx));

if cx.layout_of(sig.output()).abi == layout::Abi::Uninhabited {
if sig.output().conservative_is_uninhabited() {
llvm::Attribute::NoReturn.apply_llfn(Function, llfn);
}

Expand Down
3 changes: 1 addition & 2 deletions src/librustc_mir/borrow_check/nll/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_mir/build/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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());
Expand Down
15 changes: 5 additions & 10 deletions src/librustc_mir/hair/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, why not invoke the conservative_is_uninhabited fn we are invoking elsewhere

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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| {
Expand Down
15 changes: 11 additions & 4 deletions src/librustc_typeck/check/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,12 @@ https://doc.rust-lang.org/reference/types.html#trait-objects");

// If there are no arms, that is a diverging match; a special case.
if arms.is_empty() {
self.diverges.set(self.diverges.get() | Diverges::Always);
let diverges = match self.diverges.get() {
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
Diverges::Maybe | Diverges::Always => Diverges::Always,
Diverges::WarnedAlways => Diverges::WarnedAlways,
Diverges::UnwarnedAlways => Diverges::UnwarnedAlways,
};
self.diverges.set(diverges);
return tcx.types.never;
}

Expand All @@ -633,18 +638,20 @@ https://doc.rust-lang.org/reference/types.html#trait-objects");
match all_pats_diverge {
Diverges::Maybe => Diverges::Maybe,
Diverges::Always | Diverges::WarnedAlways => Diverges::WarnedAlways,
Diverges::UnwarnedAlways => Diverges::UnwarnedAlways,
}
}).collect();

// Now typecheck the blocks.
//
// The result of the match is the common supertype of all the
// arms. Start out the value as bottom, since it's the, well,
// bottom the type lattice, and we'll be moving up the lattice as
// arms. We start the value as the logical bottom (skipping
// `UnwarnedAlways`, which is a special case), since it's the, well,
// bottom of the type lattice, and we'll be moving up the lattice as
// we process each arm. (Note that any match with 0 arms is matching
// on any empty type and is therefore unreachable; should the flow
// of execution reach it, we will panic, so bottom is an appropriate
// type in that case)
// type in that case.)
let mut all_arms_diverge = Diverges::WarnedAlways;

let expected = expected.adjust_for_branches(self);
Expand Down
55 changes: 46 additions & 9 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 suppress_dead_code_warnings: bool into the FnCtxt.

(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;
Expand Down Expand Up @@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The 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 unimplemented! macro? I guess it expands to a block or something.)

Copy link
Contributor

Choose a reason for hiding this comment

The 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)`,
Expand Down Expand Up @@ -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);

Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Also, would this code compile on stable...?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I see that libstd/lib.rs has the exhaustive_patterns feature, so perhaps that accounts for it.

}
}

Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/unreachable-try-pattern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ enum Void {}
impl From<Void> for i32 {
fn from(v: Void) -> i32 {
match v {}
//~^ WARN unreachable expression
Copy link
Contributor

Choose a reason for hiding this comment

The 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".

Copy link
Contributor

Choose a reason for hiding this comment

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

If not this code, what would you expect the user to write?

}
}

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ! pattern concept, we could perhaps do

match x { 
    Ok(n) => Ok(n),
    Err(!)
}

Ok(y)
}

Expand Down
21 changes: 5 additions & 16 deletions src/test/debuginfo/nil-enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,20 @@
// compile-flags:-g
// gdb-command:run

// gdb-command:print first
Copy link
Contributor

@arielb1 arielb1 Apr 29, 2018

Choose a reason for hiding this comment

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

Why did you remove the gdbr-check? Please leave it here

// 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() {}
35 changes: 35 additions & 0 deletions src/test/ui/better_divergence_checking.rs
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 match void { } they would simply write void?

But that will only work if the uninhabited parameter has type !...? Or, maybe we changed that in this PR somewhere?

}

pub fn uninhabited_parameter_iii(_v: Void, x: i32) -> i32 {
println!("Call me if you can!"); //~^ ERROR unreachable expression
x
}

fn main() {}
18 changes: 18 additions & 0 deletions src/test/ui/better_divergence_checking.stderr
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

11 changes: 11 additions & 0 deletions src/test/ui/feature-gate-better_divergence_checking.err
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`.
Loading