From a474711dab65a208378e7e688f20a0bea4c94789 Mon Sep 17 00:00:00 2001 From: blitzerr Date: Thu, 24 Sep 2020 19:45:22 -0700 Subject: [PATCH 1/6] Lint for never type fallback --- compiler/rustc_session/src/lint/builtin.rs | 8 ++++ compiler/rustc_typeck/src/check/expr.rs | 1 + compiler/rustc_typeck/src/check/fn_ctxt.rs | 5 +++ compiler/rustc_typeck/src/check/mod.rs | 16 +++++++- compiler/rustc_typeck/src/check/writeback.rs | 43 ++++++++++++++++++-- src/test/ui/never_type/never_type_lint.rs | 20 +++++++++ 6 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 src/test/ui/never_type/never_type_lint.rs diff --git a/compiler/rustc_session/src/lint/builtin.rs b/compiler/rustc_session/src/lint/builtin.rs index 3e899e00d11f1..0d5f7c87ec11c 100644 --- a/compiler/rustc_session/src/lint/builtin.rs +++ b/compiler/rustc_session/src/lint/builtin.rs @@ -2493,6 +2493,13 @@ declare_lint! { "using only a subset of a register for inline asm inputs", } +declare_lint! { + //TODO: Add explanation. + pub FALL_BACK_TO_NEVER_TYPE, + Deny, + "Unresolved variable might fall back to never_type `!`" +} + declare_lint! { /// The `unsafe_op_in_unsafe_fn` lint detects unsafe operations in unsafe /// functions without an explicit unsafe block. This lint only works on @@ -2680,6 +2687,7 @@ declare_lint_pass! { SAFE_PACKED_BORROWS, PATTERNS_IN_FNS_WITHOUT_BODY, LATE_BOUND_LIFETIME_ARGUMENTS, + FALL_BACK_TO_NEVER_TYPE, ORDER_DEPENDENT_TRAIT_OBJECTS, COHERENCE_LEAK_CHECK, DEPRECATED, diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index 179e383be2e2b..6daab2b9a1a6d 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -214,6 +214,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // Combine the diverging and has_error flags. self.diverges.set(self.diverges.get() | old_diverges); self.has_errors.set(self.has_errors.get() | old_has_errors); + self.dead_nodes.borrow_mut().insert(expr.hir_id); debug!("type of {} is...", self.tcx.hir().node_to_string(expr.hir_id)); debug!("... {:?}, expected is {:?}", ty, expected); diff --git a/compiler/rustc_typeck/src/check/fn_ctxt.rs b/compiler/rustc_typeck/src/check/fn_ctxt.rs index 79d6c7dbfdae2..6b206adfc4f3a 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt.rs @@ -143,6 +143,10 @@ pub struct FnCtxt<'a, 'tcx> { /// the diverges flag is set to something other than `Maybe`. pub(super) diverges: Cell, + /// This keeps track of the dead nodes. We use this to determine + /// if there are live nodes with the diverging fallback for linting. + pub(super) dead_nodes: RefCell>, + /// Whether any child nodes have any type errors. pub(super) has_errors: Cell, @@ -175,6 +179,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { by_id: Default::default(), }), inh, + dead_nodes: RefCell::new(FxHashSet::default()), } } diff --git a/compiler/rustc_typeck/src/check/mod.rs b/compiler/rustc_typeck/src/check/mod.rs index 97172d391ba65..d7aa8c3eacffb 100644 --- a/compiler/rustc_typeck/src/check/mod.rs +++ b/compiler/rustc_typeck/src/check/mod.rs @@ -552,10 +552,12 @@ fn typeck_with_fallback<'tcx>( fcx.select_obligations_where_possible(false, |_| {}); let mut fallback_has_occurred = false; + let unsolved_variables = fcx.unsolved_variables(); + // We do fallback in two passes, to try to generate // better error messages. // The first time, we do *not* replace opaque types. - for ty in &fcx.unsolved_variables() { + for ty in &unsolved_variables { fallback_has_occurred |= fcx.fallback_if_possible(ty, FallbackMode::NoOpaque); } // We now see if we can make progress. This might @@ -583,6 +585,16 @@ fn typeck_with_fallback<'tcx>( // refer to opaque types. fcx.select_obligations_where_possible(fallback_has_occurred, |_| {}); + // We run through the list of `unsolved_variables` gathered earlier and + // check if there are any that are marked `Diverging` at this point. if + // so, this would imply, that they were assigned Divergent type because + // of fallback. Any type in `unsolved_variables` that is now `!`, is `!` + // as a result of fallback. + let mut from_diverging_fallback = unsolved_variables; + let diverging_default = fcx.tcx.mk_diverging_default(); + from_diverging_fallback + .retain(|ty| fcx.infcx.resolve_vars_if_possible(ty) == diverging_default); + // We now run fallback again, but this time we allow it to replace // unconstrained opaque type variables, in addition to performing // other kinds of fallback. @@ -616,7 +628,7 @@ fn typeck_with_fallback<'tcx>( fcx.regionck_expr(body); } - fcx.resolve_type_vars_in_body(body) + fcx.resolve_type_vars_in_body(body, &from_diverging_fallback) }); // Consistency check our TypeckResults instance can hold all ItemLocalIds diff --git a/compiler/rustc_typeck/src/check/writeback.rs b/compiler/rustc_typeck/src/check/writeback.rs index 5363702a5be6d..376b067d7f82f 100644 --- a/compiler/rustc_typeck/src/check/writeback.rs +++ b/compiler/rustc_typeck/src/check/writeback.rs @@ -34,6 +34,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { pub fn resolve_type_vars_in_body( &self, body: &'tcx hir::Body<'tcx>, + from_diverging_fallback: &Vec>, ) -> &'tcx ty::TypeckResults<'tcx> { let item_id = self.tcx.hir().body_owner(body.id()); let item_def_id = self.tcx.hir().local_def_id(item_id); @@ -43,7 +44,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let rustc_dump_user_substs = self.tcx.has_attr(item_def_id.to_def_id(), sym::rustc_dump_user_substs); - let mut wbcx = WritebackCx::new(self, body, rustc_dump_user_substs); + let mut wbcx = + WritebackCx::new(self, body, rustc_dump_user_substs, from_diverging_fallback); for param in body.params { wbcx.visit_node_id(param.pat.span, param.hir_id); } @@ -100,6 +102,11 @@ struct WritebackCx<'cx, 'tcx> { body: &'tcx hir::Body<'tcx>, rustc_dump_user_substs: bool, + + /// List of type variables which became the never type `!` + /// as a result of fallback. + /// This is used to issue lints and warnings for the user. + from_diverging_fallback: &'cx Vec>, } impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { @@ -107,6 +114,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { fcx: &'cx FnCtxt<'cx, 'tcx>, body: &'tcx hir::Body<'tcx>, rustc_dump_user_substs: bool, + from_diverging_fallback: &'cx Vec>, ) -> WritebackCx<'cx, 'tcx> { let owner = body.id().hir_id.owner; @@ -115,6 +123,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { typeck_results: ty::TypeckResults::new(owner), body, rustc_dump_user_substs, + from_diverging_fallback, } } @@ -521,8 +530,36 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { self.visit_adjustments(span, hir_id); // Resolve the type of the node with id `node_id` - let n_ty = self.fcx.node_ty(hir_id); - let n_ty = self.resolve(&n_ty, &span); + let n_ty_original = self.fcx.node_ty(hir_id); + let n_ty = self.resolve(&n_ty_original, &span); + + debug!("visit_node_id: {:?}", self.from_diverging_fallback); + // check whether the node type contains any of the variables that + // became `!` as a result of type fallback but they are not part of the + // dead nodes. if so, warn. Note that, we concern ourselves with only + // the `n_ty_original` and don't `walk()` the subparts of a type. So, for a + // variable like `Foo>>>` even if `N` is diverging type, + // we will not generate a warning. This might be okay as sometimes we may + // have things like `Result where even though `T` is diverging, + // it might never be used and warning would be confusing for the user. + if !self.from_diverging_fallback.is_empty() { + debug!("hir_id:{}", &hir_id); + debug!("n_ty_original:{}", &n_ty_original); + if !self.fcx.dead_nodes.borrow().contains(&hir_id) + && self.from_diverging_fallback.contains(&n_ty_original) + { + self.tcx() + .struct_span_lint_hir( + rustc_session::lint::builtin::FALL_BACK_TO_NEVER_TYPE, + hir_id, + span, + |lint| { + lint.build(&format!("resulted from diverging fallback: {:?}", n_ty)).emit() + } + ); + } + } + self.write_ty_to_typeck_results(hir_id, n_ty); debug!("node {:?} has type {:?}", hir_id, n_ty); diff --git a/src/test/ui/never_type/never_type_lint.rs b/src/test/ui/never_type/never_type_lint.rs new file mode 100644 index 0000000000000..a0170ce9a0fc0 --- /dev/null +++ b/src/test/ui/never_type/never_type_lint.rs @@ -0,0 +1,20 @@ +#![feature(never_type_fallback)] + +fn make_unit() { +} + +fn unconstrained_return() ->T { + unsafe { + let make_unit_fn: fn() = make_unit; + let ffi: fn() -> T = std::mem::transmute(make_unit_fn); + ffi() + } +} + +fn main() { + let _ = if true { + unconstrained_return() + } else { + panic!() + }; +} \ No newline at end of file From 333c555d70c9560c1e3b3f2ddbfc3c9f6e5b2786 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 8 Oct 2020 13:21:35 -0400 Subject: [PATCH 2/6] WIP: only add to `dead_nodes` if diverges is always --- compiler/rustc_typeck/src/check/expr.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index 6daab2b9a1a6d..588aea989f577 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -214,7 +214,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // Combine the diverging and has_error flags. self.diverges.set(self.diverges.get() | old_diverges); self.has_errors.set(self.has_errors.get() | old_has_errors); - self.dead_nodes.borrow_mut().insert(expr.hir_id); + if self.diverges.get().is_always() { + self.dead_nodes.borrow_mut().insert(expr.hir_id); + } debug!("type of {} is...", self.tcx.hir().node_to_string(expr.hir_id)); debug!("... {:?}, expected is {:?}", ty, expected); From fb4485ef25e354c95d2da3c58a2cbd45214925c2 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 8 Oct 2020 13:31:14 -0400 Subject: [PATCH 3/6] WIP: try to reproduce core problem --- compiler/rustc_session/src/lint/builtin.rs | 2 +- compiler/rustc_typeck/src/check/expr.rs | 1 + .../never_type/never_type_false_warning_unreachable.rs | 9 +++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/never_type/never_type_false_warning_unreachable.rs diff --git a/compiler/rustc_session/src/lint/builtin.rs b/compiler/rustc_session/src/lint/builtin.rs index 0d5f7c87ec11c..4ae6c4215f5e2 100644 --- a/compiler/rustc_session/src/lint/builtin.rs +++ b/compiler/rustc_session/src/lint/builtin.rs @@ -2496,7 +2496,7 @@ declare_lint! { declare_lint! { //TODO: Add explanation. pub FALL_BACK_TO_NEVER_TYPE, - Deny, + Allow, // FIXME -- this is allow by default until core builds "Unresolved variable might fall back to never_type `!`" } diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index 588aea989f577..dfbc5db82e732 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -216,6 +216,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.has_errors.set(self.has_errors.get() | old_has_errors); if self.diverges.get().is_always() { self.dead_nodes.borrow_mut().insert(expr.hir_id); + debug!("expr with HIR id {:?} is dead on exit", expr.hir_id); } debug!("type of {} is...", self.tcx.hir().node_to_string(expr.hir_id)); diff --git a/src/test/ui/never_type/never_type_false_warning_unreachable.rs b/src/test/ui/never_type/never_type_false_warning_unreachable.rs new file mode 100644 index 0000000000000..73b370649c393 --- /dev/null +++ b/src/test/ui/never_type/never_type_false_warning_unreachable.rs @@ -0,0 +1,9 @@ +macro_rules! unreachable1 { + () => {{ panic!("internal error: entered unreachable code") }}; +} + +fn get_unchecked() { + unreachable!(); +} + +fn main() {} From c0c745db1a3caabc08b15bcd5303f7b7a0a3f653 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 8 Oct 2020 13:51:05 -0400 Subject: [PATCH 4/6] test case reproduces --- .../ui/never_type/never_type_false_warning_unreachable.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/ui/never_type/never_type_false_warning_unreachable.rs b/src/test/ui/never_type/never_type_false_warning_unreachable.rs index 73b370649c393..f9b51e42db592 100644 --- a/src/test/ui/never_type/never_type_false_warning_unreachable.rs +++ b/src/test/ui/never_type/never_type_false_warning_unreachable.rs @@ -1,9 +1,11 @@ +#![deny(fall_back_to_never_type)] + macro_rules! unreachable1 { () => {{ panic!("internal error: entered unreachable code") }}; } fn get_unchecked() { - unreachable!(); + unreachable1!(); } fn main() {} From bd9d5b8059589444cb508d9dad25600556c1a211 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 15 Oct 2020 15:36:43 -0400 Subject: [PATCH 5/6] record dead blocks, not just expressions --- compiler/rustc_typeck/src/check/fn_ctxt.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/rustc_typeck/src/check/fn_ctxt.rs b/compiler/rustc_typeck/src/check/fn_ctxt.rs index 6b206adfc4f3a..20e6fdcc4a459 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt.rs @@ -1960,6 +1960,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.diverges.set(prev_diverges); } + if self.diverges.get().is_always() { + self.dead_nodes.borrow_mut().insert(blk.hir_id); + debug!("expr with HIR id {:?} is dead on exit", blk.hir_id); + } + let mut ty = ctxt.coerce.unwrap().complete(self); if self.has_errors.get() || ty.references_error() { From 77ac15deda8c8828047535e81379ff7c0dddae6f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 15 Oct 2020 15:44:22 -0400 Subject: [PATCH 6/6] WIP: make the lint deny by default --- compiler/rustc_session/src/lint/builtin.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_session/src/lint/builtin.rs b/compiler/rustc_session/src/lint/builtin.rs index 4ae6c4215f5e2..0d5f7c87ec11c 100644 --- a/compiler/rustc_session/src/lint/builtin.rs +++ b/compiler/rustc_session/src/lint/builtin.rs @@ -2496,7 +2496,7 @@ declare_lint! { declare_lint! { //TODO: Add explanation. pub FALL_BACK_TO_NEVER_TYPE, - Allow, // FIXME -- this is allow by default until core builds + Deny, "Unresolved variable might fall back to never_type `!`" }