From da33dc082802019b1aac0a89f063ae0950e81730 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 12 Jan 2020 10:41:00 -0500 Subject: [PATCH 01/24] Initial work for linting on bad never fallback --- src/librustc_typeck/check/mod.rs | 123 ++++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 2 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index dff68b9986cc9..69022b5b876e8 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -135,6 +135,7 @@ use syntax::util::parser::ExprPrecedence; use rustc_error_codes::*; +use std::borrow::Cow; use std::cell::{Cell, Ref, RefCell, RefMut}; use std::cmp; use std::collections::hash_map::Entry; @@ -254,6 +255,8 @@ pub struct Inherited<'a, 'tcx> { /// not clear. implicit_region_bound: Option>, + inferred_paths: RefCell>>, + body_id: Option, } @@ -619,6 +622,13 @@ pub struct FnCtxt<'a, 'tcx> { inh: &'a Inherited<'a, 'tcx>, } +#[derive(Clone, Debug)] +struct InferredPath<'tcx> { + span: Span, + ty: Option>, + args: Option]>>, +} + impl<'a, 'tcx> Deref for FnCtxt<'a, 'tcx> { type Target = Inherited<'a, 'tcx>; fn deref(&self) -> &Self::Target { @@ -685,6 +695,7 @@ impl Inherited<'a, 'tcx> { opaque_types: RefCell::new(Default::default()), opaque_types_vars: RefCell::new(Default::default()), implicit_region_bound, + inferred_paths: RefCell::new(Default::default()), body_id, } } @@ -1053,6 +1064,32 @@ fn typeck_tables_of_with_fallback<'tcx>( // All type checking constraints were added, try to fallback unsolved variables. fcx.select_obligations_where_possible(false, |_| {}); let mut fallback_has_occurred = false; + let unresolved_paths: FxHashMap> = fcx + .inferred_paths + .borrow() + .iter() + .map(|(id, path)| (*id, path.clone())) + .filter(|(hir_id, path)| { + debug!( + "typeck_tables_of_with_fallback: inspecting path ({:?}, {:?})", + hir_id, path + ); + let debug_resolved = fcx.infcx.resolve_vars_if_possible(&path.ty); + if fcx.infcx.unresolved_type_vars(&path.ty).is_some() { + debug!( + "typeck_tables_of_with_fallback: unresolved vars in ty: {:?}", + debug_resolved + ); + true + } else { + debug!( + "typeck_tables_of_with_fallback: all vars resolved in ty: {:?}", + debug_resolved + ); + false + } + }) + .collect(); // We do fallback in two passes, to try to generate // better error messages. @@ -1095,6 +1132,42 @@ fn typeck_tables_of_with_fallback<'tcx>( // See if we can make any more progress. fcx.select_obligations_where_possible(fallback_has_occurred, |_| {}); + for (call_id, path) in unresolved_paths { + debug!( + "Resolved ty: {:?} at span {:?} : expr={:?} parent={:?} path={:?}", + path.span, + path.ty, + tcx.hir().get(call_id), + tcx.hir().get(tcx.hir().get_parent_node(call_id)), + path + ); + + let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); + debug!("Fully resolved ty: {:?}", ty); + + let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); + + if let ty::FnDef(_, substs) = ty.kind { + debug!("Got substs: {:?}", substs); + let mut inhabited = true; + for arg in &*path.args.unwrap() { + let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); + + if resolved_arg.conservative_is_privately_uninhabited(tcx) { + debug!("Arg is uninhabited: {:?}", resolved_arg); + inhabited = false; + break; + } else { + debug!("Arg is inhabited: {:?}", resolved_arg); + } + } + + if inhabited { + debug!("All arguments are inhabited!"); + } + } + } + // Even though coercion casts provide type hints, we check casts after fallback for // backwards compatibility. This makes fallback a stronger type hint than a cast coercion. fcx.check_casts(); @@ -3624,7 +3697,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.check_argument_types( sp, expr, - &err_inputs[..], + err_inputs, &[], args_no_rcvr, false, @@ -3732,13 +3805,30 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &self, sp: Span, expr: &'tcx hir::Expr<'tcx>, - fn_inputs: &[Ty<'tcx>], + fn_inputs: impl Into]>>, expected_arg_tys: &[Ty<'tcx>], args: &'tcx [hir::Expr<'tcx>], c_variadic: bool, tuple_arguments: TupleArgumentsFlag, def_span: Option, ) { + let fn_inputs = fn_inputs.into(); + debug!("check_argument_types: storing arguments for expr {:?}", expr); + match self.inferred_paths.borrow_mut().entry(expr.hir_id) { + Entry::Vacant(e) => { + debug!("check_argument_types: making new entry for types {:?}", fn_inputs); + e.insert(InferredPath { span: sp, ty: None, args: Some(fn_inputs.clone()) }); + } + Entry::Occupied(mut e) => { + debug!( + "check_argument_types: modifiying exsting entry {:?} with types {:?}", + e.get(), + fn_inputs + ); + e.get_mut().args = Some(fn_inputs.clone()); + } + } + let tcx = self.tcx; // Grab the argument types, supplying fresh type variables // if the wrong number of arguments were supplied @@ -5425,6 +5515,35 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // the referenced item. let ty_substituted = self.instantiate_type_scheme(span, &substs, &ty); + if ty_substituted.has_infer_types() { + debug!( + "instantiate_value_path: saving path with infer: ({:?}, {:?})", + span, ty_substituted + ); + let parent_id = tcx.hir().get_parent_node(hir_id); + let parent = tcx.hir().get(parent_id); + match parent { + Node::Expr(hir::Expr { span: p_span, kind: ExprKind::Call(..), .. }) + | Node::Expr(hir::Expr { span: p_span, kind: ExprKind::MethodCall(..), .. }) => { + match self.inferred_paths.borrow_mut().entry(parent_id) { + Entry::Vacant(e) => { + debug!("instantiate_value_path: inserting new path"); + e.insert(InferredPath { + span: *p_span, + ty: Some(ty_substituted), + args: None, + }); + } + Entry::Occupied(mut e) => { + debug!("instantiate_value_path: updating existing path {:?}", e.get()); + e.get_mut().ty = Some(ty_substituted); + } + } + } + _ => {} + } + } + if let Some(UserSelfTy { impl_def_id, self_ty }) = user_self_ty { // In the case of `Foo::method` and `>::method`, if `method` // is inherent, there is no `Self` parameter; instead, the impl needs From 0c498931c09f2931a2506b1b2cfa9e30acd5839d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 12 Jan 2020 16:11:13 -0500 Subject: [PATCH 02/24] Emit warning --- src/librustc_typeck/check/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 69022b5b876e8..0ff3a98c44d76 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -1164,6 +1164,13 @@ fn typeck_tables_of_with_fallback<'tcx>( if inhabited { debug!("All arguments are inhabited!"); + fcx.tcx() + .sess + .struct_span_warn( + path.span, + "Fallback to `!` may introduce undefined behavior", + ) + .emit(); } } } From 3a22be96d224e39378f72036560dfdc4bbce1df7 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 12 Jan 2020 16:15:33 -0500 Subject: [PATCH 03/24] Improve lint to check substs --- src/librustc_typeck/check/mod.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 0ff3a98c44d76..c5074661abbc0 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -1149,21 +1149,34 @@ fn typeck_tables_of_with_fallback<'tcx>( if let ty::FnDef(_, substs) = ty.kind { debug!("Got substs: {:?}", substs); - let mut inhabited = true; + let mut args_inhabited = true; + let mut substs_inhabited = true; + for arg in &*path.args.unwrap() { let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); if resolved_arg.conservative_is_privately_uninhabited(tcx) { debug!("Arg is uninhabited: {:?}", resolved_arg); - inhabited = false; + args_inhabited = false; break; } else { debug!("Arg is inhabited: {:?}", resolved_arg); } } - if inhabited { - debug!("All arguments are inhabited!"); + for subst_ty in substs.types() { + let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); + if resolved_subst.conservative_is_privately_uninhabited(tcx) { + debug!("Subst is uninhabited: {:?}", resolved_subst); + substs_inhabited = false; + break; + } else { + debug!("Subst is inhabited: {:?}", resolved_subst); + } + } + + if args_inhabited && !substs_inhabited { + debug!("All arguments are inhabited, at least one subst is not inhabited!"); fcx.tcx() .sess .struct_span_warn( From d0dc1e32928eade0ca5ba19e92a67b029d00246c Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 12 Jan 2020 17:14:10 -0500 Subject: [PATCH 04/24] Make the error message much better --- src/librustc_typeck/check/mod.rs | 136 +++++++++++++++++++++++++++---- 1 file changed, 122 insertions(+), 14 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index c5074661abbc0..a90d44283e92f 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -627,6 +627,7 @@ struct InferredPath<'tcx> { span: Span, ty: Option>, args: Option]>>, + unresolved_vars: Vec>>, } impl<'a, 'tcx> Deref for FnCtxt<'a, 'tcx> { @@ -1069,28 +1070,72 @@ fn typeck_tables_of_with_fallback<'tcx>( .borrow() .iter() .map(|(id, path)| (*id, path.clone())) - .filter(|(hir_id, path)| { + .filter_map(|(hir_id, mut path)| { debug!( "typeck_tables_of_with_fallback: inspecting path ({:?}, {:?})", hir_id, path ); - let debug_resolved = fcx.infcx.resolve_vars_if_possible(&path.ty); - if fcx.infcx.unresolved_type_vars(&path.ty).is_some() { + + let ty_resolved = fcx.infcx.resolve_vars_if_possible(&path.ty); + + let fn_substs = match ty_resolved { + Some(ty::TyS { kind: ty::FnDef(_, substs), .. }) => substs, + _ => { + debug!( + "typeck_tables_of_with_fallback: non-fn ty {:?}, skipping", + ty_resolved + ); + return None; + } + }; + + if fcx.infcx.unresolved_type_vars(fn_substs).is_some() { + struct TyVarFinder<'a, 'tcx> { + infcx: &'a InferCtxt<'a, 'tcx>, + vars: Vec>, + } + impl<'a, 'tcx> TypeFolder<'tcx> for TyVarFinder<'a, 'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.infcx.tcx + } + + fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { + if let ty::Infer(ty::InferTy::TyVar(_)) = t.kind { + self.vars.push(t); + } + t.super_fold_with(self) + } + } + + for subst in fn_substs.types() { + let mut finder = TyVarFinder { infcx: &fcx.infcx, vars: vec![] }; + path.ty.fold_with(&mut finder); + path.unresolved_vars.push(finder.vars); + } + debug!( - "typeck_tables_of_with_fallback: unresolved vars in ty: {:?}", - debug_resolved + "typeck_tables_of_with_fallback: unresolved vars in ty {:?} : {:?}", + ty_resolved, path.unresolved_vars ); - true + + Some((hir_id, path)) } else { debug!( "typeck_tables_of_with_fallback: all vars resolved in ty: {:?}", - debug_resolved + ty_resolved ); - false + None } }) .collect(); + let unconstrained_diverging: Vec<_> = fcx + .unsolved_variables() + .iter() + .cloned() + .filter(|ty| fcx.infcx.type_var_diverges(ty)) + .collect(); + // We do fallback in two passes, to try to generate // better error messages. // The first time, we do *not* replace opaque types. @@ -1150,7 +1195,7 @@ fn typeck_tables_of_with_fallback<'tcx>( if let ty::FnDef(_, substs) = ty.kind { debug!("Got substs: {:?}", substs); let mut args_inhabited = true; - let mut substs_inhabited = true; + let mut uninhabited_subst = None; for arg in &*path.args.unwrap() { let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); @@ -1164,25 +1209,82 @@ fn typeck_tables_of_with_fallback<'tcx>( } } - for subst_ty in substs.types() { + for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.into_iter()) { let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); if resolved_subst.conservative_is_privately_uninhabited(tcx) { debug!("Subst is uninhabited: {:?}", resolved_subst); - substs_inhabited = false; - break; + if !vars.is_empty() { + debug!("Found fallback vars: {:?}", vars); + uninhabited_subst = Some((resolved_subst, vars)); + break; + } else { + debug!("No fallback vars") + } } else { debug!("Subst is inhabited: {:?}", resolved_subst); } } - if args_inhabited && !substs_inhabited { + if let (true, Some((subst, vars))) = (args_inhabited, uninhabited_subst) { debug!("All arguments are inhabited, at least one subst is not inhabited!"); + + let mut best_diverging_var = None; + let mut best_var = None; + + for var in vars { + for diverging_var in &unconstrained_diverging { + match (&var.kind, &diverging_var.kind) { + ( + ty::Infer(ty::InferTy::TyVar(vid1)), + ty::Infer(ty::InferTy::TyVar(vid2)), + ) => { + if fcx + .infcx + .type_variables + .borrow_mut() + .sub_unified(*vid1, *vid2) + { + debug!( + "Type variable {:?} is equal to diverging var {:?}", + var, diverging_var + ); + best_var = Some(var); + best_diverging_var = Some(diverging_var); + } + } + _ => bug!( + "Unexpected types: var={:?} diverging_var={:?}", + var, + diverging_var + ), + } + } + } + + let (var_span, diverging_var_span) = + match (&best_var.unwrap().kind, &best_diverging_var.unwrap().kind) { + ( + ty::Infer(ty::InferTy::TyVar(var_vid)), + ty::Infer(ty::InferTy::TyVar(diverging_var_vid)), + ) => ( + fcx.infcx.type_variables.borrow().var_origin(*var_vid).span, + fcx.infcx + .type_variables + .borrow() + .var_origin(*diverging_var_vid) + .span, + ), + _ => bug!("Type is not a ty variable: {:?}", best_var), + }; + fcx.tcx() .sess .struct_span_warn( path.span, "Fallback to `!` may introduce undefined behavior", ) + .span_note(var_span, "the type here was inferred to `!`") + .span_note(diverging_var_span, "... due to this expression") .emit(); } } @@ -3837,7 +3939,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { match self.inferred_paths.borrow_mut().entry(expr.hir_id) { Entry::Vacant(e) => { debug!("check_argument_types: making new entry for types {:?}", fn_inputs); - e.insert(InferredPath { span: sp, ty: None, args: Some(fn_inputs.clone()) }); + e.insert(InferredPath { + span: sp, + ty: None, + args: Some(fn_inputs.clone()), + unresolved_vars: vec![], + }); } Entry::Occupied(mut e) => { debug!( @@ -5552,6 +5659,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { span: *p_span, ty: Some(ty_substituted), args: None, + unresolved_vars: vec![], }); } Entry::Occupied(mut e) => { From ebf191893f6fc04a1c6980914ad028c46812e0e3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 13 Jan 2020 07:21:56 -0500 Subject: [PATCH 05/24] Provide a better error when no substs is provided for generic param --- src/librustc_typeck/check/mod.rs | 65 +++++++++++++++++++------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index a90d44283e92f..470c04b821dc5 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -1248,8 +1248,13 @@ fn typeck_tables_of_with_fallback<'tcx>( "Type variable {:?} is equal to diverging var {:?}", var, diverging_var ); - best_var = Some(var); - best_diverging_var = Some(diverging_var); + + debug!( + "Var origin: {:?}", + fcx.infcx.type_variables.borrow().var_origin(*vid1) + ); + best_var = Some(vid1); + best_diverging_var = Some(vid2); } } _ => bug!( @@ -1261,31 +1266,39 @@ fn typeck_tables_of_with_fallback<'tcx>( } } - let (var_span, diverging_var_span) = - match (&best_var.unwrap().kind, &best_diverging_var.unwrap().kind) { - ( - ty::Infer(ty::InferTy::TyVar(var_vid)), - ty::Infer(ty::InferTy::TyVar(diverging_var_vid)), - ) => ( - fcx.infcx.type_variables.borrow().var_origin(*var_vid).span, - fcx.infcx - .type_variables - .borrow() - .var_origin(*diverging_var_vid) - .span, - ), - _ => bug!("Type is not a ty variable: {:?}", best_var), - }; + let var_origin = + *fcx.infcx.type_variables.borrow().var_origin(*best_var.unwrap()); + let diverging_var_span = fcx + .infcx + .type_variables + .borrow() + .var_origin(*best_diverging_var.unwrap()) + .span; + + let mut err = fcx.tcx().sess.struct_span_warn( + path.span, + "Fallback to `!` may introduce undefined behavior", + ); - fcx.tcx() - .sess - .struct_span_warn( - path.span, - "Fallback to `!` may introduce undefined behavior", - ) - .span_note(var_span, "the type here was inferred to `!`") - .span_note(diverging_var_span, "... due to this expression") - .emit(); + match var_origin.kind { + TypeVariableOriginKind::TypeParameterDefinition(name, did) => { + err.span_note( + var_origin.span, + &format!("the type parameter {} here was inferred to `!`", name), + ); + if let Some(did) = did { + err.span_note( + fcx.tcx.def_span(did), + "(type parameter defined here)", + ); + } + } + _ => { + err.span_note(var_origin.span, "the type here was inferred to `!`"); + } + } + + err.span_note(diverging_var_span, "... due to this expression").emit(); } } } From 5253bb62e8d88891ed2768327d8306feaa381a29 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 13 Jan 2020 07:35:35 -0500 Subject: [PATCH 06/24] Factor out code into its own module --- src/librustc_typeck/check/mod.rs | 198 +------------------- src/librustc_typeck/check/never_compat.rs | 215 ++++++++++++++++++++++ 2 files changed, 218 insertions(+), 195 deletions(-) create mode 100644 src/librustc_typeck/check/never_compat.rs diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 470c04b821dc5..04148cb383493 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -80,6 +80,7 @@ mod expr; mod generator_interior; pub mod intrinsic; pub mod method; +mod never_compat; mod op; mod pat; mod regionck; @@ -1065,76 +1066,7 @@ fn typeck_tables_of_with_fallback<'tcx>( // All type checking constraints were added, try to fallback unsolved variables. fcx.select_obligations_where_possible(false, |_| {}); let mut fallback_has_occurred = false; - let unresolved_paths: FxHashMap> = fcx - .inferred_paths - .borrow() - .iter() - .map(|(id, path)| (*id, path.clone())) - .filter_map(|(hir_id, mut path)| { - debug!( - "typeck_tables_of_with_fallback: inspecting path ({:?}, {:?})", - hir_id, path - ); - - let ty_resolved = fcx.infcx.resolve_vars_if_possible(&path.ty); - - let fn_substs = match ty_resolved { - Some(ty::TyS { kind: ty::FnDef(_, substs), .. }) => substs, - _ => { - debug!( - "typeck_tables_of_with_fallback: non-fn ty {:?}, skipping", - ty_resolved - ); - return None; - } - }; - - if fcx.infcx.unresolved_type_vars(fn_substs).is_some() { - struct TyVarFinder<'a, 'tcx> { - infcx: &'a InferCtxt<'a, 'tcx>, - vars: Vec>, - } - impl<'a, 'tcx> TypeFolder<'tcx> for TyVarFinder<'a, 'tcx> { - fn tcx(&self) -> TyCtxt<'tcx> { - self.infcx.tcx - } - - fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { - if let ty::Infer(ty::InferTy::TyVar(_)) = t.kind { - self.vars.push(t); - } - t.super_fold_with(self) - } - } - - for subst in fn_substs.types() { - let mut finder = TyVarFinder { infcx: &fcx.infcx, vars: vec![] }; - path.ty.fold_with(&mut finder); - path.unresolved_vars.push(finder.vars); - } - - debug!( - "typeck_tables_of_with_fallback: unresolved vars in ty {:?} : {:?}", - ty_resolved, path.unresolved_vars - ); - - Some((hir_id, path)) - } else { - debug!( - "typeck_tables_of_with_fallback: all vars resolved in ty: {:?}", - ty_resolved - ); - None - } - }) - .collect(); - - let unconstrained_diverging: Vec<_> = fcx - .unsolved_variables() - .iter() - .cloned() - .filter(|ty| fcx.infcx.type_var_diverges(ty)) - .collect(); + let never_compat = never_compat::NeverCompatHandler::pre_fallback(&fcx); // We do fallback in two passes, to try to generate // better error messages. @@ -1177,131 +1109,7 @@ fn typeck_tables_of_with_fallback<'tcx>( // See if we can make any more progress. fcx.select_obligations_where_possible(fallback_has_occurred, |_| {}); - for (call_id, path) in unresolved_paths { - debug!( - "Resolved ty: {:?} at span {:?} : expr={:?} parent={:?} path={:?}", - path.span, - path.ty, - tcx.hir().get(call_id), - tcx.hir().get(tcx.hir().get_parent_node(call_id)), - path - ); - - let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); - debug!("Fully resolved ty: {:?}", ty); - - let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); - - if let ty::FnDef(_, substs) = ty.kind { - debug!("Got substs: {:?}", substs); - let mut args_inhabited = true; - let mut uninhabited_subst = None; - - for arg in &*path.args.unwrap() { - let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); - - if resolved_arg.conservative_is_privately_uninhabited(tcx) { - debug!("Arg is uninhabited: {:?}", resolved_arg); - args_inhabited = false; - break; - } else { - debug!("Arg is inhabited: {:?}", resolved_arg); - } - } - - for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.into_iter()) { - let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); - if resolved_subst.conservative_is_privately_uninhabited(tcx) { - debug!("Subst is uninhabited: {:?}", resolved_subst); - if !vars.is_empty() { - debug!("Found fallback vars: {:?}", vars); - uninhabited_subst = Some((resolved_subst, vars)); - break; - } else { - debug!("No fallback vars") - } - } else { - debug!("Subst is inhabited: {:?}", resolved_subst); - } - } - - if let (true, Some((subst, vars))) = (args_inhabited, uninhabited_subst) { - debug!("All arguments are inhabited, at least one subst is not inhabited!"); - - let mut best_diverging_var = None; - let mut best_var = None; - - for var in vars { - for diverging_var in &unconstrained_diverging { - match (&var.kind, &diverging_var.kind) { - ( - ty::Infer(ty::InferTy::TyVar(vid1)), - ty::Infer(ty::InferTy::TyVar(vid2)), - ) => { - if fcx - .infcx - .type_variables - .borrow_mut() - .sub_unified(*vid1, *vid2) - { - debug!( - "Type variable {:?} is equal to diverging var {:?}", - var, diverging_var - ); - - debug!( - "Var origin: {:?}", - fcx.infcx.type_variables.borrow().var_origin(*vid1) - ); - best_var = Some(vid1); - best_diverging_var = Some(vid2); - } - } - _ => bug!( - "Unexpected types: var={:?} diverging_var={:?}", - var, - diverging_var - ), - } - } - } - - let var_origin = - *fcx.infcx.type_variables.borrow().var_origin(*best_var.unwrap()); - let diverging_var_span = fcx - .infcx - .type_variables - .borrow() - .var_origin(*best_diverging_var.unwrap()) - .span; - - let mut err = fcx.tcx().sess.struct_span_warn( - path.span, - "Fallback to `!` may introduce undefined behavior", - ); - - match var_origin.kind { - TypeVariableOriginKind::TypeParameterDefinition(name, did) => { - err.span_note( - var_origin.span, - &format!("the type parameter {} here was inferred to `!`", name), - ); - if let Some(did) = did { - err.span_note( - fcx.tcx.def_span(did), - "(type parameter defined here)", - ); - } - } - _ => { - err.span_note(var_origin.span, "the type here was inferred to `!`"); - } - } - - err.span_note(diverging_var_span, "... due to this expression").emit(); - } - } - } + never_compat.post_fallback(&fcx); // Even though coercion casts provide type hints, we check casts after fallback for // backwards compatibility. This makes fallback a stronger type hint than a cast coercion. diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs new file mode 100644 index 0000000000000..15e882625d780 --- /dev/null +++ b/src/librustc_typeck/check/never_compat.rs @@ -0,0 +1,215 @@ +use crate::check::{FnCtxt, InferredPath}; +use crate::rustc::ty::TypeFoldable; +use rustc::infer::type_variable::TypeVariableOriginKind; +use rustc::infer::InferCtxt; +use rustc::ty; +use rustc::ty::fold::TypeFolder; +use rustc::ty::{Ty, TyCtxt}; +use rustc_data_structures::fx::FxHashMap; +use rustc_hir::HirId; + +/// Code to detect cases where using `!` (never-type) fallback instead of `()` fallback +/// may result in the introduction of undefined behavior +/// + +pub struct NeverCompatHandler<'tcx> { + unresolved_paths: FxHashMap>, + unconstrained_diverging: Vec>, +} + +struct TyVarFinder<'a, 'tcx> { + infcx: &'a InferCtxt<'a, 'tcx>, + vars: Vec>, +} +impl<'a, 'tcx> TypeFolder<'tcx> for TyVarFinder<'a, 'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.infcx.tcx + } + + fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { + if let ty::Infer(ty::InferTy::TyVar(_)) = t.kind { + self.vars.push(t); + } + t.super_fold_with(self) + } +} + +impl<'tcx> NeverCompatHandler<'tcx> { + pub fn pre_fallback(fcx: &FnCtxt<'a, 'tcx>) -> NeverCompatHandler<'tcx> { + let unresolved_paths: FxHashMap> = fcx + .inferred_paths + .borrow() + .iter() + .map(|(id, path)| (*id, path.clone())) + .filter_map(|(hir_id, mut path)| { + debug!("pre_fallback: inspecting path ({:?}, {:?})", hir_id, path); + + let ty_resolved = fcx.infcx.resolve_vars_if_possible(&path.ty); + + let fn_substs = match ty_resolved { + Some(ty::TyS { kind: ty::FnDef(_, substs), .. }) => substs, + _ => { + debug!("pre_fallback: non-fn ty {:?}, skipping", ty_resolved); + return None; + } + }; + + if fcx.infcx.unresolved_type_vars(fn_substs).is_some() { + for subst in fn_substs.types() { + let mut finder = TyVarFinder { infcx: &fcx.infcx, vars: vec![] }; + path.ty.fold_with(&mut finder); + path.unresolved_vars.push(finder.vars); + } + + debug!( + "pre_fallback: unresolved vars in ty {:?} : {:?}", + ty_resolved, path.unresolved_vars + ); + + Some((hir_id, path)) + } else { + debug!("pre_fallback: all vars resolved in ty: {:?}", ty_resolved); + None + } + }) + .collect(); + + let unconstrained_diverging: Vec<_> = fcx + .unsolved_variables() + .iter() + .cloned() + .filter(|ty| fcx.infcx.type_var_diverges(ty)) + .collect(); + + NeverCompatHandler { unresolved_paths, unconstrained_diverging } + } + + pub fn post_fallback(self, fcx: &FnCtxt<'a, 'tcx>) { + let tcx = fcx.tcx; + for (call_id, path) in self.unresolved_paths { + debug!( + "Resolved ty: {:?} at span {:?} : expr={:?} parent={:?} path={:?}", + path.span, + path.ty, + tcx.hir().get(call_id), + tcx.hir().get(tcx.hir().get_parent_node(call_id)), + path + ); + + let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); + debug!("Fully resolved ty: {:?}", ty); + + let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); + + if let ty::FnDef(_, substs) = ty.kind { + debug!("Got substs: {:?}", substs); + let mut args_inhabited = true; + let mut uninhabited_subst = None; + + for arg in &*path.args.unwrap() { + let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); + + if resolved_arg.conservative_is_privately_uninhabited(tcx) { + debug!("Arg is uninhabited: {:?}", resolved_arg); + args_inhabited = false; + break; + } else { + debug!("Arg is inhabited: {:?}", resolved_arg); + } + } + + for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.into_iter()) { + let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); + if resolved_subst.conservative_is_privately_uninhabited(tcx) { + debug!("Subst is uninhabited: {:?}", resolved_subst); + if !vars.is_empty() { + debug!("Found fallback vars: {:?}", vars); + uninhabited_subst = Some((resolved_subst, vars)); + break; + } else { + debug!("No fallback vars") + } + } else { + debug!("Subst is inhabited: {:?}", resolved_subst); + } + } + + if let (true, Some((subst, vars))) = (args_inhabited, uninhabited_subst) { + debug!("All arguments are inhabited, at least one subst is not inhabited!"); + + let mut best_diverging_var = None; + let mut best_var = None; + + for var in vars { + for diverging_var in &self.unconstrained_diverging { + match (&var.kind, &diverging_var.kind) { + ( + ty::Infer(ty::InferTy::TyVar(vid1)), + ty::Infer(ty::InferTy::TyVar(vid2)), + ) => { + if fcx + .infcx + .type_variables + .borrow_mut() + .sub_unified(*vid1, *vid2) + { + debug!( + "Type variable {:?} is equal to diverging var {:?}", + var, diverging_var + ); + + debug!( + "Var origin: {:?}", + fcx.infcx.type_variables.borrow().var_origin(*vid1) + ); + best_var = Some(vid1); + best_diverging_var = Some(vid2); + } + } + _ => bug!( + "Unexpected types: var={:?} diverging_var={:?}", + var, + diverging_var + ), + } + } + } + + let var_origin = + *fcx.infcx.type_variables.borrow().var_origin(*best_var.unwrap()); + let diverging_var_span = fcx + .infcx + .type_variables + .borrow() + .var_origin(*best_diverging_var.unwrap()) + .span; + + let mut err = tcx.sess.struct_span_warn( + path.span, + "Fallback to `!` may introduce undefined behavior", + ); + + match var_origin.kind { + TypeVariableOriginKind::TypeParameterDefinition(name, did) => { + err.span_note( + var_origin.span, + &format!("the type parameter {} here was inferred to `!`", name), + ); + if let Some(did) = did { + err.span_note( + fcx.tcx.def_span(did), + "(type parameter defined here)", + ); + } + } + _ => { + err.span_note(var_origin.span, "the type here was inferred to `!`"); + } + } + + err.span_note(diverging_var_span, "... due to this expression").emit(); + } + } + } + } +} From 087af859c078f242943c26f34777982947dbe62c Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 07:47:48 -0500 Subject: [PATCH 07/24] Tweak error message --- src/librustc_typeck/check/never_compat.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 15e882625d780..0fce38f09cf45 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -207,7 +207,11 @@ impl<'tcx> NeverCompatHandler<'tcx> { } } - err.span_note(diverging_var_span, "... due to this expression").emit(); + err.span_note( + diverging_var_span, + "... due to this expression evaluating to `!`", + ) + .emit(); } } } From 932d7c2eac810a6de8cf46c9271ee290e8451174 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 07:54:37 -0500 Subject: [PATCH 08/24] Some fixup and cleanup --- src/librustc_typeck/check/never_compat.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 0fce38f09cf45..53c27364e110a 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -57,7 +57,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { if fcx.infcx.unresolved_type_vars(fn_substs).is_some() { for subst in fn_substs.types() { let mut finder = TyVarFinder { infcx: &fcx.infcx, vars: vec![] }; - path.ty.fold_with(&mut finder); + subst.fold_with(&mut finder); path.unresolved_vars.push(finder.vars); } @@ -124,7 +124,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { debug!("Subst is uninhabited: {:?}", resolved_subst); if !vars.is_empty() { debug!("Found fallback vars: {:?}", vars); - uninhabited_subst = Some((resolved_subst, vars)); + uninhabited_subst = Some(vars); break; } else { debug!("No fallback vars") @@ -134,7 +134,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { } } - if let (true, Some((subst, vars))) = (args_inhabited, uninhabited_subst) { + if let (true, Some(vars)) = (args_inhabited, uninhabited_subst) { debug!("All arguments are inhabited, at least one subst is not inhabited!"); let mut best_diverging_var = None; @@ -211,6 +211,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { diverging_var_span, "... due to this expression evaluating to `!`", ) + .note("If you want the `!` type to be used here, add explicit type annotations") .emit(); } } From 10cd2e3efe50504c9ec7510221b67eac2b55a4df Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 08:05:55 -0500 Subject: [PATCH 09/24] Work on splitting up post_fallback --- src/librustc_typeck/check/never_compat.rs | 213 +++++++++++----------- 1 file changed, 104 insertions(+), 109 deletions(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 53c27364e110a..a06ed40a4e7d1 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -34,6 +34,58 @@ impl<'a, 'tcx> TypeFolder<'tcx> for TyVarFinder<'a, 'tcx> { } } +fn find_questionable_call( + path: InferredPath<'tcx>, + fcx: &FnCtxt<'a, 'tcx>, +) -> Option>> { + let tcx = fcx.tcx; + let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); + debug!("post_fallback: Fully resolved ty: {:?}", ty); + + let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); + + if let ty::FnDef(_, substs) = ty.kind { + debug!("Got substs: {:?}", substs); + let mut args_inhabited = true; + + for arg in &*path.args.unwrap() { + let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); + + if resolved_arg.conservative_is_privately_uninhabited(tcx) { + debug!("post_fallback: Arg is uninhabited: {:?}", resolved_arg); + args_inhabited = false; + break; + } else { + debug!("post_fallback: Arg is inhabited: {:?}", resolved_arg); + } + } + + if !args_inhabited { + debug!("post_fallback: Not all arguments are inhabited"); + return None; + } + + for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.into_iter()) { + let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); + if resolved_subst.conservative_is_privately_uninhabited(tcx) { + debug!("post_fallback: Subst is uninhabited: {:?}", resolved_subst); + if !vars.is_empty() { + debug!("Found fallback vars: {:?}", vars); + debug!( + "post_fallback: All arguments are inhabited, at least one subst is not inhabited!" + ); + return Some(vars); + } else { + debug!("No fallback vars") + } + } else { + debug!("post_fallback: Subst is inhabited: {:?}", resolved_subst); + } + } + } + return None; +} + impl<'tcx> NeverCompatHandler<'tcx> { pub fn pre_fallback(fcx: &FnCtxt<'a, 'tcx>) -> NeverCompatHandler<'tcx> { let unresolved_paths: FxHashMap> = fcx @@ -88,7 +140,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { let tcx = fcx.tcx; for (call_id, path) in self.unresolved_paths { debug!( - "Resolved ty: {:?} at span {:?} : expr={:?} parent={:?} path={:?}", + "post_fallback: resolved ty: {:?} at span {:?} : expr={:?} parent={:?} path={:?}", path.span, path.ty, tcx.hir().get(call_id), @@ -96,124 +148,67 @@ impl<'tcx> NeverCompatHandler<'tcx> { path ); - let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); - debug!("Fully resolved ty: {:?}", ty); - - let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); - - if let ty::FnDef(_, substs) = ty.kind { - debug!("Got substs: {:?}", substs); - let mut args_inhabited = true; - let mut uninhabited_subst = None; - - for arg in &*path.args.unwrap() { - let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); - - if resolved_arg.conservative_is_privately_uninhabited(tcx) { - debug!("Arg is uninhabited: {:?}", resolved_arg); - args_inhabited = false; - break; - } else { - debug!("Arg is inhabited: {:?}", resolved_arg); - } - } - - for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.into_iter()) { - let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); - if resolved_subst.conservative_is_privately_uninhabited(tcx) { - debug!("Subst is uninhabited: {:?}", resolved_subst); - if !vars.is_empty() { - debug!("Found fallback vars: {:?}", vars); - uninhabited_subst = Some(vars); - break; - } else { - debug!("No fallback vars") - } - } else { - debug!("Subst is inhabited: {:?}", resolved_subst); - } - } - - if let (true, Some(vars)) = (args_inhabited, uninhabited_subst) { - debug!("All arguments are inhabited, at least one subst is not inhabited!"); - - let mut best_diverging_var = None; - let mut best_var = None; - - for var in vars { - for diverging_var in &self.unconstrained_diverging { - match (&var.kind, &diverging_var.kind) { - ( - ty::Infer(ty::InferTy::TyVar(vid1)), - ty::Infer(ty::InferTy::TyVar(vid2)), - ) => { - if fcx - .infcx - .type_variables - .borrow_mut() - .sub_unified(*vid1, *vid2) - { - debug!( - "Type variable {:?} is equal to diverging var {:?}", - var, diverging_var - ); - - debug!( - "Var origin: {:?}", - fcx.infcx.type_variables.borrow().var_origin(*vid1) - ); - best_var = Some(vid1); - best_diverging_var = Some(vid2); - } + let span = path.span; + if let Some(vars) = find_questionable_call(path, fcx) { + let mut best_diverging_var = None; + let mut best_var = None; + + for var in vars { + for diverging_var in &self.unconstrained_diverging { + match (&var.kind, &diverging_var.kind) { + ( + ty::Infer(ty::InferTy::TyVar(vid1)), + ty::Infer(ty::InferTy::TyVar(vid2)), + ) => { + if fcx.infcx.type_variables.borrow_mut().sub_unified(*vid1, *vid2) { + debug!( + "Type variable {:?} is equal to diverging var {:?}", + var, diverging_var + ); + + debug!( + "Var origin: {:?}", + fcx.infcx.type_variables.borrow().var_origin(*vid1) + ); + best_var = Some(vid1); + best_diverging_var = Some(vid2); } - _ => bug!( - "Unexpected types: var={:?} diverging_var={:?}", - var, - diverging_var - ), } + _ => bug!( + "Unexpected types: var={:?} diverging_var={:?}", + var, + diverging_var + ), } } + } - let var_origin = - *fcx.infcx.type_variables.borrow().var_origin(*best_var.unwrap()); - let diverging_var_span = fcx - .infcx - .type_variables - .borrow() - .var_origin(*best_diverging_var.unwrap()) - .span; - - let mut err = tcx.sess.struct_span_warn( - path.span, - "Fallback to `!` may introduce undefined behavior", - ); - - match var_origin.kind { - TypeVariableOriginKind::TypeParameterDefinition(name, did) => { - err.span_note( - var_origin.span, - &format!("the type parameter {} here was inferred to `!`", name), - ); - if let Some(did) = did { - err.span_note( - fcx.tcx.def_span(did), - "(type parameter defined here)", - ); - } - } - _ => { - err.span_note(var_origin.span, "the type here was inferred to `!`"); + let var_origin = *fcx.infcx.type_variables.borrow().var_origin(*best_var.unwrap()); + let diverging_var_span = + fcx.infcx.type_variables.borrow().var_origin(*best_diverging_var.unwrap()).span; + + let mut err = tcx + .sess + .struct_span_warn(span, "Fallback to `!` may introduce undefined behavior"); + + match var_origin.kind { + TypeVariableOriginKind::TypeParameterDefinition(name, did) => { + err.span_note( + var_origin.span, + &format!("the type parameter {} here was inferred to `!`", name), + ); + if let Some(did) = did { + err.span_note(fcx.tcx.def_span(did), "(type parameter defined here)"); } } + _ => { + err.span_note(var_origin.span, "the type here was inferred to `!`"); + } + } - err.span_note( - diverging_var_span, - "... due to this expression evaluating to `!`", - ) + err.span_note(diverging_var_span, "... due to this expression evaluating to `!`") .note("If you want the `!` type to be used here, add explicit type annotations") .emit(); - } } } } From a8ce8235a74a8c6ef32e8b11246f1b86c303c856 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 08:18:15 -0500 Subject: [PATCH 10/24] More refactoring --- src/librustc_typeck/check/never_compat.rs | 85 +++++++++++------------ 1 file changed, 42 insertions(+), 43 deletions(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index a06ed40a4e7d1..77c5ea3f3eb5b 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -4,7 +4,7 @@ use rustc::infer::type_variable::TypeVariableOriginKind; use rustc::infer::InferCtxt; use rustc::ty; use rustc::ty::fold::TypeFolder; -use rustc::ty::{Ty, TyCtxt}; +use rustc::ty::{Ty, TyCtxt, TyVid}; use rustc_data_structures::fx::FxHashMap; use rustc_hir::HirId; @@ -34,10 +34,10 @@ impl<'a, 'tcx> TypeFolder<'tcx> for TyVarFinder<'a, 'tcx> { } } -fn find_questionable_call( - path: InferredPath<'tcx>, +fn find_questionable_call<'a, 'tcx>( + path: &'a InferredPath<'tcx>, fcx: &FnCtxt<'a, 'tcx>, -) -> Option>> { +) -> Option<&'a [Ty<'tcx>]> { let tcx = fcx.tcx; let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); debug!("post_fallback: Fully resolved ty: {:?}", ty); @@ -48,7 +48,7 @@ fn find_questionable_call( debug!("Got substs: {:?}", substs); let mut args_inhabited = true; - for arg in &*path.args.unwrap() { + for arg in &**path.args.as_ref().unwrap() { let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); if resolved_arg.conservative_is_privately_uninhabited(tcx) { @@ -65,7 +65,7 @@ fn find_questionable_call( return None; } - for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.into_iter()) { + for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.iter()) { let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); if resolved_subst.conservative_is_privately_uninhabited(tcx) { debug!("post_fallback: Subst is uninhabited: {:?}", resolved_subst); @@ -86,6 +86,11 @@ fn find_questionable_call( return None; } +struct VarData { + best_var: TyVid, + best_diverging_var: TyVid, +} + impl<'tcx> NeverCompatHandler<'tcx> { pub fn pre_fallback(fcx: &FnCtxt<'a, 'tcx>) -> NeverCompatHandler<'tcx> { let unresolved_paths: FxHashMap> = fcx @@ -136,56 +141,50 @@ impl<'tcx> NeverCompatHandler<'tcx> { NeverCompatHandler { unresolved_paths, unconstrained_diverging } } + fn find_best_vars(&self, fcx: &FnCtxt<'a, 'tcx>, vars: &[Ty<'tcx>]) -> VarData { + for var in vars { + for diverging_var in &self.unconstrained_diverging { + match (&var.kind, &diverging_var.kind) { + (ty::Infer(ty::InferTy::TyVar(vid1)), ty::Infer(ty::InferTy::TyVar(vid2))) => { + if fcx.infcx.type_variables.borrow_mut().sub_unified(*vid1, *vid2) { + debug!( + "Type variable {:?} is equal to diverging var {:?}", + var, diverging_var + ); + + debug!( + "Var origin: {:?}", + fcx.infcx.type_variables.borrow().var_origin(*vid1) + ); + return VarData { best_var: *vid1, best_diverging_var: *vid2 }; + } + } + _ => bug!("Unexpected types: var={:?} diverging_var={:?}", var, diverging_var), + } + } + } + bug!("No vars were equated to divering vars: {:?}", vars) + } + pub fn post_fallback(self, fcx: &FnCtxt<'a, 'tcx>) { let tcx = fcx.tcx; - for (call_id, path) in self.unresolved_paths { + for (call_id, path) in &self.unresolved_paths { debug!( "post_fallback: resolved ty: {:?} at span {:?} : expr={:?} parent={:?} path={:?}", path.span, path.ty, - tcx.hir().get(call_id), - tcx.hir().get(tcx.hir().get_parent_node(call_id)), + tcx.hir().get(*call_id), + tcx.hir().get(tcx.hir().get_parent_node(*call_id)), path ); let span = path.span; if let Some(vars) = find_questionable_call(path, fcx) { - let mut best_diverging_var = None; - let mut best_var = None; - - for var in vars { - for diverging_var in &self.unconstrained_diverging { - match (&var.kind, &diverging_var.kind) { - ( - ty::Infer(ty::InferTy::TyVar(vid1)), - ty::Infer(ty::InferTy::TyVar(vid2)), - ) => { - if fcx.infcx.type_variables.borrow_mut().sub_unified(*vid1, *vid2) { - debug!( - "Type variable {:?} is equal to diverging var {:?}", - var, diverging_var - ); - - debug!( - "Var origin: {:?}", - fcx.infcx.type_variables.borrow().var_origin(*vid1) - ); - best_var = Some(vid1); - best_diverging_var = Some(vid2); - } - } - _ => bug!( - "Unexpected types: var={:?} diverging_var={:?}", - var, - diverging_var - ), - } - } - } + let VarData { best_var, best_diverging_var } = self.find_best_vars(fcx, &vars); - let var_origin = *fcx.infcx.type_variables.borrow().var_origin(*best_var.unwrap()); + let var_origin = *fcx.infcx.type_variables.borrow().var_origin(best_var); let diverging_var_span = - fcx.infcx.type_variables.borrow().var_origin(*best_diverging_var.unwrap()).span; + fcx.infcx.type_variables.borrow().var_origin(best_diverging_var).span; let mut err = tcx .sess From 65318da7c6e7764a20a2a3e2a40f35525ff5fe71 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 09:22:14 -0500 Subject: [PATCH 11/24] Add a bunch of comments --- src/librustc_typeck/check/never_compat.rs | 255 ++++++++++++++++++++-- 1 file changed, 236 insertions(+), 19 deletions(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 77c5ea3f3eb5b..c0d81c2010d86 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -9,14 +9,162 @@ use rustc_data_structures::fx::FxHashMap; use rustc_hir::HirId; /// Code to detect cases where using `!` (never-type) fallback instead of `()` fallback -/// may result in the introduction of undefined behavior +/// may result in the introduction of undefined behavior; /// +/// TL;DR: We look for method calls whose arguments are all inhabited, but +/// that have a generic parameter (e.g. `T` in `fn foo() { ... } `) +/// that's uninhabited due to fallback. We emit an error, forcing +/// users to add explicit type annotations indicating whether they +/// actually want the `!` type, or something else. +/// +/// Background: When we first tried to enable never-type fallback, it was found to +/// cause code in the 'obj' crate to segfault. The root cause was a piece +/// of code that looked like this: +/// +/// ```rust +///fn unconstrained_return() -> Result { +/// let ffi: fn() -> T = transmute(some_pointer); +/// Ok(ffi()) +/// } +///fn foo() { +/// match unconstrained_return::<_>() { +/// Ok(x) => x, // `x` has type `_`, which is unconstrained +/// Err(s) => panic!(s), // … except for unifying with the type of `panic!()` +/// // so that both `match` arms have the same type. +/// // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. +/// }; +///} +/// ``` +/// +/// Previously, the return type of `panic!()` would end up as `()` due to fallback +/// (even though `panic()` is declared as returning `!`). This meant that +/// the function pointer would get transmuted to `fn() -> ()`. +/// +/// With never-type fallback enabled, the function pointer was transmuted +/// to `fn() -> !`, despite the fact that it actually returned. This lead +/// to undefined behavior, since we actually "produced" an instance of `!` +/// at runtime. +/// +/// We want to prevent this code from compiling, unless the user provides +/// explicit type annotations indicating that they want the `!` type to be used. +/// +/// However, we want code like this to continue working: +/// +/// ```rust +/// fn foo(e: !) -> Box { +/// Box::<_>::new(e) +/// }``` +/// +/// From the perspective of fallback, these two snippets of code are very +/// similar. We have some expression that starts out with type `!`, which +/// we replace with a divergent inference variable. When fallback runs, +/// we unify the inference variable with `!`, which leads to other +/// expressions getting a type of `!` as well (`Box` in this case), +/// becoming uninhabited as well. +/// +/// Our task is to distinguish things that are "legitimately" uninhabited, +/// (such as `Box` in the second example), from things that are +/// "suspiciously" uninhabited and may lead to undefined behavior +/// (the `Result` in the first example). +/// +/// The key difference between these two snippets of code turns out +/// to be the *arguments* used in a function call. In the first example, +/// we call `unconstrained_return` with no arguments, and an inferred +/// type parameter of `!` (a.k.a `unconstrained_return::()`). +/// +/// In the second example, we call `Box::new(!)`, again inferring +/// the type parameter to `!`. Since at least one argument to `Box::new`, +/// is uninhabited, we know that this call is *statically unreachable* - +/// if it were ever executed, we would have instantaited an uninhabited +/// type for a parameter, which is impossible. This means that while +/// the fallback to `!` has affected the type-checking of this call, +/// it won't actually affect the runtime behavior of the call, since +/// the call can never be executed. +/// +/// In the first example, the call to `unconstrained_return::()` is +/// still (potentially) live, since it trivially has no uninhabited arguments +/// (it has no arguments at all). This means that our fallback to `!` may +/// have changed the runtime behavior of `unconstrained_return` (in this case, +/// it did) - we may actually execute the call at runtime, but with an uninhabited +/// type parameter that the user did not intend to provide. +/// +/// This distinction forms the basis for our lint. We look for +/// any function/method calls that meet the following requirements: +/// +/// * Unresolved inference variables are present in the generic substs prior to fallback +/// * All arguments are inhabited +/// * After fallback, at least one generic subsst is uninhabited. +/// +/// Let's break down each of these requirements: +/// +/// 1. We only look at method calls, not other expressions. +/// This is because the issue boils down to a generic subst being +/// uninhabited due to fallback. Generic parameters can only be used +/// in two places - as arguments to types, and arguments to methods. +/// We don't care about types - the only relevant expression would be +/// a constructor (e.g. `MyStruct { a: true, b: 25 }`). In and of itself, +/// constructing a type this way (after all field values are evaluated) +/// can never cause undefined behavior. +/// +/// Methods calls, on the other hand, might cause undefined behavior +/// due to how they use their generic paramters. Note that this +/// still true if the undefineed behavior occurs in the same function +/// where fallback occurs - there still must be a call `transmute` +/// or some other intrinsic that allows 'instantating' an `!` instance +/// somehow. +/// +/// 2. Only function calls with inference variables in their generic +/// substs can possibly be affected by fallback - if all types are resolved, +/// then fallback cannot possibly have any effect. +/// +/// 3. If any arguments are uninhabited, then the function call cannot ever +/// cause undefined behavior, since it can never be executed. This check +/// ensures that we don't lint on `Box::<_>::new(!)` - even though +/// we're inferring a generic subst (`_`) to `!`, it has no runtime +/// effect. +/// +/// 4. If a generic substs is uninhabited after fallback, then we have +/// the potential for undefined behavior. Note that this check can +/// have false positives - if a generic argument was already uninhabited +/// prior to fallback, we might end up triggering the lint even if fallback +/// didn't cause any generic substs to become uninhabited. However, this isn't +/// a big deal for a few reasons: +/// +/// * This lint should be incredibly rare (as of the time of writing, only +/// `obj` has run into the relevant issue). +/// * Even if fallback didn't cause anything to become uninhabited +/// (e.g. a `*const _` being inferred to `*const !`), it's still influencing +/// the generic substs of a potentially live (all arguments inhabited) function +/// call. This is arguably something worth linting against, since the user +/// might not be aware that a live call is being affected by a seemingly +/// unrelated expression. +/// +/// * It should always be possible to resolve this lint while keeping the +/// existing behavior, simply by adding explicit type annotations. This +/// ensures that fallback never runs (since the type variable will +/// actually be constrained to `!` or something else). At worst, +/// we will force users to write some arguably unecessary type annotation, +/// but we will not permanently break any code. +/// The main interface to this module. +/// It exposes two hooks: `pre_fallback` and `post_fallback`, +/// which are invoked by `typeck_tables_of_with_fallback` respectively before +/// and after fallback is run. pub struct NeverCompatHandler<'tcx> { + /// A map from method call `HirId`'s to `InferredPaths`. + /// This is computed from `FnCtxt.inferred_paths`, which + /// is in turn populated during typecheck. unresolved_paths: FxHashMap>, + /// All divering type variables that were unconstrained + /// prior to fallback. We use this to help generate + /// better diagnostics by determining which + /// inference variables were equated with a diverging + /// fallback variable. unconstrained_diverging: Vec>, } +/// A simpler helper to collect all `InferTy::TyVar`s found +/// in a `TypeFoldable`. struct TyVarFinder<'a, 'tcx> { infcx: &'a InferCtxt<'a, 'tcx>, vars: Vec>, @@ -34,64 +182,104 @@ impl<'a, 'tcx> TypeFolder<'tcx> for TyVarFinder<'a, 'tcx> { } } +/// The core logic of this check. Given +/// an `InferredPath`, we determine +/// if this method call is "questionable" using the criteria +/// described at the top of the module. +/// +/// If the method call is "questionable", and should +/// be linted, we return Ok(tys), where `tys` +/// is a list of all inference variables involved +/// with the generic paramter affected by fallback. +/// +/// Otherwise, we return None fn find_questionable_call<'a, 'tcx>( path: &'a InferredPath<'tcx>, fcx: &FnCtxt<'a, 'tcx>, ) -> Option<&'a [Ty<'tcx>]> { let tcx = fcx.tcx; let ty = fcx.infcx.resolve_vars_if_possible(&path.ty); - debug!("post_fallback: Fully resolved ty: {:?}", ty); + debug!("find_questionable_call: Fully resolved ty: {:?}", ty); let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); + /// Check that this InferredPath actually corresponds to a method + /// invocation. Currently, type-checking resolves generic paths + /// (e.g. `Box::<_>::new` separately from determining that a method call + /// is occuring). We use this check to skip over `InferredPaths` for + /// non-method-calls (e.g. struct constructors). if let ty::FnDef(_, substs) = ty.kind { - debug!("Got substs: {:?}", substs); + debug!("find_questionable_call: Got substs: {:?}", substs); let mut args_inhabited = true; + // See if we can find any arguments that are definitely uninhabited. + // If we can, we're done - the method call is dead, so fallback + // cannot affect its runtime behavior. for arg in &**path.args.as_ref().unwrap() { let resolved_arg = fcx.infcx.resolve_vars_if_possible(arg); + // We use `conservative_is_privately_uninhabited` so that we only + // bail out if we're certain that a type is uninhabited. if resolved_arg.conservative_is_privately_uninhabited(tcx) { - debug!("post_fallback: Arg is uninhabited: {:?}", resolved_arg); - args_inhabited = false; - break; + debug!("find_questionable_call: bailing out due to uninhabited arg: {:?}", resolved_arg); + return None; } else { - debug!("post_fallback: Arg is inhabited: {:?}", resolved_arg); + debug!("find_questionable_call: Arg is inhabited: {:?}", resolved_arg); } } - if !args_inhabited { - debug!("post_fallback: Not all arguments are inhabited"); - return None; - } - + // Now, check the generic substs for the method (e.g. `T` and `V` in `foo::()`. + // If we find an uninhabited subst, for (subst_ty, vars) in substs.types().zip(path.unresolved_vars.iter()) { let resolved_subst = fcx.infcx.resolve_vars_if_possible(&subst_ty); - if resolved_subst.conservative_is_privately_uninhabited(tcx) { - debug!("post_fallback: Subst is uninhabited: {:?}", resolved_subst); + // We use `is_ty_uninhabited_from_any_module` instead of `conservative_is_privately_uninhabited`. + // Using a broader check for uninhabitedness ensures that we lint + // whenever the subst is uninhabted, regardless of whether or not + // the user code could know this. + if tcx.is_ty_uninhabited_from_any_module(resolved_subst){ + debug!("find_questionable_call: Subst is uninhabited: {:?}", resolved_subst); if !vars.is_empty() { - debug!("Found fallback vars: {:?}", vars); + debug!("find_questionable_call: Found fallback vars: {:?}", vars); debug!( - "post_fallback: All arguments are inhabited, at least one subst is not inhabited!" + "find_questionable_call: All arguments are inhabited, at least one subst is not inhabited!" ); + // These variables were recorded prior to fallback runntime. + // When generating a diagnostic message, we will use these + // to determine which spans will we show to the user. return Some(vars); } else { - debug!("No fallback vars") + debug!("find_questionable_call: No fallback vars") } } else { - debug!("post_fallback: Subst is inhabited: {:?}", resolved_subst); + debug!("find_questionable_call: Subst is inhabited: {:?}", resolved_subst); } } } return None; } +/// Holds the "best" type variables that we want +/// to use to generate a message for the user. +/// +/// Note that these two variables are unified with each other, +/// and therefore represent the same type. However, they have different +/// origin information, which we can use to generate a nice error message. struct VarData { + /// A type variable that was actually used in an uninhabited + /// generic subst (e.g. `foo::<_#0t>()` where `_#0t = !`) + /// This is used to point at the span where the U.B potentially + /// occurs best_var: TyVid, + /// The diverging type variable that is ultimately responsible + /// for the U.B. occuring. In the `obj` example, this would + /// be the type variable corresponding the `panic!()` expression. best_diverging_var: TyVid, } impl<'tcx> NeverCompatHandler<'tcx> { + /// The pre-fallback hook invoked by typecheck. We collect + /// all unconstrained inference variables used in method call substs, + /// so that we can check if any changes occur due to fallback. pub fn pre_fallback(fcx: &FnCtxt<'a, 'tcx>) -> NeverCompatHandler<'tcx> { let unresolved_paths: FxHashMap> = fcx .inferred_paths @@ -103,6 +291,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { let ty_resolved = fcx.infcx.resolve_vars_if_possible(&path.ty); + // We only care about method calls, not other uses of generic paths. let fn_substs = match ty_resolved { Some(ty::TyS { kind: ty::FnDef(_, substs), .. }) => substs, _ => { @@ -111,6 +300,8 @@ impl<'tcx> NeverCompatHandler<'tcx> { } }; + // Any method call with inference variables in its substs + // could potentially be affected by fallback. if fcx.infcx.unresolved_type_vars(fn_substs).is_some() { for subst in fn_substs.types() { let mut finder = TyVarFinder { infcx: &fcx.infcx, vars: vec![] }; @@ -131,6 +322,8 @@ impl<'tcx> NeverCompatHandler<'tcx> { }) .collect(); + // Collect all divering inference variables, so that we + // can later compare them against other inference variables. let unconstrained_diverging: Vec<_> = fcx .unsolved_variables() .iter() @@ -141,6 +334,18 @@ impl<'tcx> NeverCompatHandler<'tcx> { NeverCompatHandler { unresolved_paths, unconstrained_diverging } } + /// Finds the 'best' type variables to use for display to the user, + /// given a list of the type variables originally used in the + /// generic substs for a method. + /// + /// We look for a type variable that ended up unified with a diverging + /// inference variable. This represents a place where fallback to a never + /// type ended up affecting a live method call. We then return the + /// inference variable, along with the corresponding divering inference + /// variable that was unified with it + /// + /// Note that their be multiple such pairs of inference variables - howver, + /// we only display one to the user, to avoid overwhelming them with information. fn find_best_vars(&self, fcx: &FnCtxt<'a, 'tcx>, vars: &[Ty<'tcx>]) -> VarData { for var in vars { for diverging_var in &self.unconstrained_diverging { @@ -163,9 +368,12 @@ impl<'tcx> NeverCompatHandler<'tcx> { } } } - bug!("No vars were equated to divering vars: {:?}", vars) + bug!("No vars were equated to diverging vars: {:?}", vars) } + /// The hook used by typecheck after fallback has been run. + /// We look for any "questionable" calls, generating diagnostic + /// message for them. pub fn post_fallback(self, fcx: &FnCtxt<'a, 'tcx>) { let tcx = fcx.tcx; for (call_id, path) in &self.unresolved_paths { @@ -190,7 +398,13 @@ impl<'tcx> NeverCompatHandler<'tcx> { .sess .struct_span_warn(span, "Fallback to `!` may introduce undefined behavior"); + // There are two possible cases here: match var_origin.kind { + // 1. This inference variable was automatically generated due to the user + // not using the turbofish syntax. For example, `foo()` where foo is deinfed + // as `fn foo() { ... }`. + // In this case, we point at the definition of the type variable (e.g. the `T` + // in `foo`), to better explain to the user what's going on. TypeVariableOriginKind::TypeParameterDefinition(name, did) => { err.span_note( var_origin.span, @@ -200,6 +414,9 @@ impl<'tcx> NeverCompatHandler<'tcx> { err.span_note(fcx.tcx.def_span(did), "(type parameter defined here)"); } } + // The user wrote an explicit inference variable: e.g. `foo::<_>()`. We point + // directly at its span, since this is sufficient to explain to the user + // what's going on. _ => { err.span_note(var_origin.span, "the type here was inferred to `!`"); } From 61b879ca4d410a9457cfddc449bb685727513c79 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 09:23:14 -0500 Subject: [PATCH 12/24] Some additional cleanup --- src/librustc_typeck/check/mod.rs | 49 ++++++++++++++++++++++- src/librustc_typeck/check/never_compat.rs | 18 +++++---- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 04148cb383493..e5a63f6540bdb 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -256,6 +256,12 @@ pub struct Inherited<'a, 'tcx> { /// not clear. implicit_region_bound: Option>, + /// Maps each expression with a generic path + /// (e.g. `foo::()` to `InferredPath` containing + /// additional information used by `NeverCompatHandler`. + /// + /// Each entry in this map is gradually filled in during typecheck, + /// as the information we need is not available all at once. inferred_paths: RefCell>>, body_id: Option, @@ -623,11 +629,45 @@ pub struct FnCtxt<'a, 'tcx> { inh: &'a Inherited<'a, 'tcx>, } +/// Stores additional data about a generic path +/// containing inference variables (e.g. `my_method::<_, u8>(bar)`). +/// This is used by `NeverCompatHandler` to inspect +/// all method calls that contain inference variables. +/// +/// This struct is a little strange, in that its data +/// is filled in from two different places in typecheck. +/// Thw two `Option` fields are written by `check_argument_types` +/// and `instantiate_value_path`, since neither method +/// has all of the information it needs. #[derive(Clone, Debug)] struct InferredPath<'tcx> { + /// The span of the corresponding expression. span: Span, + /// The type of this path. For method calls, + /// this is a `ty::FnDef` ty: Option>, + /// The types of the arguments (*not* generic substs) + /// provided to this path, if it represents a method + /// call. For example, `foo(true, 25)` would have + /// types `[bool, i32]`. If this path does not + /// correspond to a method, then this will be `None` + /// + /// This is a `Cow` rather than a `Vec` or slice + /// to accommodate `check_argument_types`, which may + /// be called with either an interned slice or a Vec. + /// A `Cow` lets us avoid unecessary interning + /// and Vec construction, since we just need to iterate + /// over this args: Option]>>, + /// The unresolved inference variables for each + /// generic substs. Each entry in the outer vec + /// corresponds to a generic substs in the function. + /// + /// For example, suppose we have the function + /// `fn foo (){ ... }`. + /// + /// The method call `foo::, true>>()` + /// will have an `unresolved_vars` of `[[_#0t, _#1t], []]` unresolved_vars: Vec>>, } @@ -3757,6 +3797,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ) { let fn_inputs = fn_inputs.into(); debug!("check_argument_types: storing arguments for expr {:?}", expr); + // We now have the arguments types available for this msthod call, + // so store them in the `inferred_paths` entry for this method call. + // We set `ty` as `None` if we are the first to access the entry + // for this method, and leave it untouched otherwise. match self.inferred_paths.borrow_mut().entry(expr.hir_id) { Entry::Vacant(e) => { debug!("check_argument_types: making new entry for types {:?}", fn_inputs); @@ -3769,7 +3813,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } Entry::Occupied(mut e) => { debug!( - "check_argument_types: modifiying exsting entry {:?} with types {:?}", + "check_argument_types: modifying existing {:?} with types {:?}", e.get(), fn_inputs ); @@ -5473,6 +5517,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { match parent { Node::Expr(hir::Expr { span: p_span, kind: ExprKind::Call(..), .. }) | Node::Expr(hir::Expr { span: p_span, kind: ExprKind::MethodCall(..), .. }) => { + // Fill in the type for our parent expression. This might not be + // a method call - if it is, the argumetns will be filled in by + // `check_argument_types` match self.inferred_paths.borrow_mut().entry(parent_id) { Entry::Vacant(e) => { debug!("instantiate_value_path: inserting new path"); diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index c0d81c2010d86..7201bac6a0632 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -203,14 +203,13 @@ fn find_questionable_call<'a, 'tcx>( let ty = ty.unwrap_or_else(|| bug!("Missing ty in path: {:?}", path)); - /// Check that this InferredPath actually corresponds to a method - /// invocation. Currently, type-checking resolves generic paths - /// (e.g. `Box::<_>::new` separately from determining that a method call - /// is occuring). We use this check to skip over `InferredPaths` for - /// non-method-calls (e.g. struct constructors). + // Check that this InferredPath actually corresponds to a method + // invocation. Currently, type-checking resolves generic paths + // (e.g. `Box::<_>::new` separately from determining that a method call + // is occurring). We use this check to skip over `InferredPaths` for + // non-method-calls (e.g. struct constructors). if let ty::FnDef(_, substs) = ty.kind { debug!("find_questionable_call: Got substs: {:?}", substs); - let mut args_inhabited = true; // See if we can find any arguments that are definitely uninhabited. // If we can, we're done - the method call is dead, so fallback @@ -221,7 +220,10 @@ fn find_questionable_call<'a, 'tcx>( // We use `conservative_is_privately_uninhabited` so that we only // bail out if we're certain that a type is uninhabited. if resolved_arg.conservative_is_privately_uninhabited(tcx) { - debug!("find_questionable_call: bailing out due to uninhabited arg: {:?}", resolved_arg); + debug!( + "find_questionable_call: bailing out due to uninhabited arg: {:?}", + resolved_arg + ); return None; } else { debug!("find_questionable_call: Arg is inhabited: {:?}", resolved_arg); @@ -236,7 +238,7 @@ fn find_questionable_call<'a, 'tcx>( // Using a broader check for uninhabitedness ensures that we lint // whenever the subst is uninhabted, regardless of whether or not // the user code could know this. - if tcx.is_ty_uninhabited_from_any_module(resolved_subst){ + if tcx.is_ty_uninhabited_from_any_module(resolved_subst) { debug!("find_questionable_call: Subst is uninhabited: {:?}", resolved_subst); if !vars.is_empty() { debug!("find_questionable_call: Found fallback vars: {:?}", vars); From c5691a80de216bf1741a22b20d01e27a26b052b3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 15 Jan 2020 09:43:10 -0500 Subject: [PATCH 13/24] Change warning to error --- src/librustc_typeck/check/never_compat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 7201bac6a0632..39cb076f6ec4f 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -398,7 +398,7 @@ impl<'tcx> NeverCompatHandler<'tcx> { let mut err = tcx .sess - .struct_span_warn(span, "Fallback to `!` may introduce undefined behavior"); + .struct_span_err(span, "Fallback to `!` may introduce undefined behavior"); // There are two possible cases here: match var_origin.kind { From 1ed39bcbac4912e31157b4231d5455a809155367 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 17 Jan 2020 15:58:06 -0500 Subject: [PATCH 14/24] Add some tests --- src/test/ui/never-fallback/box_it.rs | 11 ++++++++ src/test/ui/never-fallback/from.rs | 14 ++++++++++ src/test/ui/never-fallback/from_try.rs | 17 +++++++++++ src/test/ui/never-fallback/obj.rs | 28 +++++++++++++++++++ src/test/ui/never-fallback/obj.stderr | 21 ++++++++++++++ src/test/ui/never-fallback/obj_implicit.rs | 28 +++++++++++++++++++ .../ui/never-fallback/obj_implicit.stderr | 26 +++++++++++++++++ 7 files changed, 145 insertions(+) create mode 100644 src/test/ui/never-fallback/box_it.rs create mode 100644 src/test/ui/never-fallback/from.rs create mode 100644 src/test/ui/never-fallback/from_try.rs create mode 100644 src/test/ui/never-fallback/obj.rs create mode 100644 src/test/ui/never-fallback/obj.stderr create mode 100644 src/test/ui/never-fallback/obj_implicit.rs create mode 100644 src/test/ui/never-fallback/obj_implicit.stderr diff --git a/src/test/ui/never-fallback/box_it.rs b/src/test/ui/never-fallback/box_it.rs new file mode 100644 index 0000000000000..e98024a518467 --- /dev/null +++ b/src/test/ui/never-fallback/box_it.rs @@ -0,0 +1,11 @@ +// check-pass +// Verifies that our fallback-UB check doesn't trigger +// on correct code +#![feature(never_type)] +#![feature(never_type_fallback)] + +fn foo(e: !) -> Box { + Box::<_>::new(e) +} + +fn main() {} diff --git a/src/test/ui/never-fallback/from.rs b/src/test/ui/never-fallback/from.rs new file mode 100644 index 0000000000000..ede419dbb8ea1 --- /dev/null +++ b/src/test/ui/never-fallback/from.rs @@ -0,0 +1,14 @@ +// check-pass +// Tests that we correctly infer types to ! +#![feature(never_type)] +#![feature(never_type_fallback)] + +struct E; +impl From for E { + fn from(x: !) -> E { x } +} +fn foo(never: !) { + >::from(never); +} + +fn main() {} diff --git a/src/test/ui/never-fallback/from_try.rs b/src/test/ui/never-fallback/from_try.rs new file mode 100644 index 0000000000000..5b445f0f716c5 --- /dev/null +++ b/src/test/ui/never-fallback/from_try.rs @@ -0,0 +1,17 @@ +// check-pass +use std::convert::{TryFrom, Infallible}; + +struct E; + +impl From for E { + fn from(_: Infallible) -> E { + E + } +} + +fn foo() -> Result<(), E> { + u32::try_from(1u32)?; + Ok(()) +} + +fn main() {} diff --git a/src/test/ui/never-fallback/obj.rs b/src/test/ui/never-fallback/obj.rs new file mode 100644 index 0000000000000..740bb679d64b4 --- /dev/null +++ b/src/test/ui/never-fallback/obj.rs @@ -0,0 +1,28 @@ +#![feature(never_type)] +#![feature(never_type_fallback)] + +fn get_type(_: T) -> &'static str { + std::any::type_name::() +} + +fn unconstrained_return() -> Result { + Err("Hi".to_string()) +} + +fn foo() { + let a = || { + match unconstrained_return::<_>() { //~ ERROR Fallback to `!` may introduce undefined behavior + Ok(x) => x, // `x` has type `_`, which is unconstrained + Err(s) => panic!(s), // … except for unifying with the type of `panic!()` + // so that both `match` arms have the same type. + // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. + } + }; + + let cast: &dyn FnOnce() -> _ = &a; + println!("Return type: {:?}", get_type(cast)); +} + +fn main() { + foo() +} diff --git a/src/test/ui/never-fallback/obj.stderr b/src/test/ui/never-fallback/obj.stderr new file mode 100644 index 0000000000000..e4dfcd278bfaa --- /dev/null +++ b/src/test/ui/never-fallback/obj.stderr @@ -0,0 +1,21 @@ +error: Fallback to `!` may introduce undefined behavior + --> $DIR/obj.rs:14:15 + | +LL | match unconstrained_return::<_>() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: the type here was inferred to `!` + --> $DIR/obj.rs:14:38 + | +LL | match unconstrained_return::<_>() { + | ^ +note: ... due to this expression evaluating to `!` + --> $DIR/obj.rs:16:23 + | +LL | Err(s) => panic!(s), // … except for unifying with the type of `panic!()` + | ^^^^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: aborting due to previous error + diff --git a/src/test/ui/never-fallback/obj_implicit.rs b/src/test/ui/never-fallback/obj_implicit.rs new file mode 100644 index 0000000000000..1b67ed425c30e --- /dev/null +++ b/src/test/ui/never-fallback/obj_implicit.rs @@ -0,0 +1,28 @@ +#![feature(never_type)] +#![feature(never_type_fallback)] + +fn get_type(_: T) -> &'static str { + std::any::type_name::() +} + +fn unconstrained_return() -> Result { + Err("Hi".to_string()) +} + +fn foo() { + let a = || { + match unconstrained_return() { //~ ERROR Fallback to `!` may introduce undefined behavior + Ok(x) => x, // `x` has type `_`, which is unconstrained + Err(s) => panic!(s), // … except for unifying with the type of `panic!()` + // so that both `match` arms have the same type. + // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. + } + }; + + let cast: &dyn FnOnce() -> _ = &a; + println!("Return type: {:?}", get_type(cast)); +} + +fn main() { + foo() +} diff --git a/src/test/ui/never-fallback/obj_implicit.stderr b/src/test/ui/never-fallback/obj_implicit.stderr new file mode 100644 index 0000000000000..554d1a110c823 --- /dev/null +++ b/src/test/ui/never-fallback/obj_implicit.stderr @@ -0,0 +1,26 @@ +error: Fallback to `!` may introduce undefined behavior + --> $DIR/obj_implicit.rs:14:15 + | +LL | match unconstrained_return() { + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter T here was inferred to `!` + --> $DIR/obj_implicit.rs:14:15 + | +LL | match unconstrained_return() { + | ^^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/obj_implicit.rs:8:25 + | +LL | fn unconstrained_return() -> Result { + | ^ +note: ... due to this expression evaluating to `!` + --> $DIR/obj_implicit.rs:16:23 + | +LL | Err(s) => panic!(s), // … except for unifying with the type of `panic!()` + | ^^^^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: aborting due to previous error + From d4bd42219032341576bbd1905d12c7550a012c80 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 18 Jan 2020 13:30:27 -0500 Subject: [PATCH 15/24] Fix tidy errors --- src/test/ui/never-fallback/obj.rs | 6 +++--- src/test/ui/never-fallback/obj_implicit.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/ui/never-fallback/obj.rs b/src/test/ui/never-fallback/obj.rs index 740bb679d64b4..3c79ba87233c3 100644 --- a/src/test/ui/never-fallback/obj.rs +++ b/src/test/ui/never-fallback/obj.rs @@ -10,15 +10,15 @@ fn unconstrained_return() -> Result { } fn foo() { - let a = || { - match unconstrained_return::<_>() { //~ ERROR Fallback to `!` may introduce undefined behavior + let a = || { + match unconstrained_return::<_>() { //~ ERROR Fallback to `!` may introduce undefined Ok(x) => x, // `x` has type `_`, which is unconstrained Err(s) => panic!(s), // … except for unifying with the type of `panic!()` // so that both `match` arms have the same type. // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. } }; - + let cast: &dyn FnOnce() -> _ = &a; println!("Return type: {:?}", get_type(cast)); } diff --git a/src/test/ui/never-fallback/obj_implicit.rs b/src/test/ui/never-fallback/obj_implicit.rs index 1b67ed425c30e..025eee1213e20 100644 --- a/src/test/ui/never-fallback/obj_implicit.rs +++ b/src/test/ui/never-fallback/obj_implicit.rs @@ -10,7 +10,7 @@ fn unconstrained_return() -> Result { } fn foo() { - let a = || { + let a = || { match unconstrained_return() { //~ ERROR Fallback to `!` may introduce undefined behavior Ok(x) => x, // `x` has type `_`, which is unconstrained Err(s) => panic!(s), // … except for unifying with the type of `panic!()` @@ -18,7 +18,7 @@ fn foo() { // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value. } }; - + let cast: &dyn FnOnce() -> _ = &a; println!("Return type: {:?}", get_type(cast)); } From f1961b59553397e3f92fc2b1454b8161419901ff Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 18 Jan 2020 14:54:02 -0500 Subject: [PATCH 16/24] Fix failing test --- .../diverging-fallback-control-flow.rs | 18 +- .../diverging-fallback-control-flow.stderr | 209 ++++++++++++++++++ 2 files changed, 217 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/never_type/diverging-fallback-control-flow.stderr diff --git a/src/test/ui/never_type/diverging-fallback-control-flow.rs b/src/test/ui/never_type/diverging-fallback-control-flow.rs index ea4881049d792..6cb28f66389d5 100644 --- a/src/test/ui/never_type/diverging-fallback-control-flow.rs +++ b/src/test/ui/never_type/diverging-fallback-control-flow.rs @@ -1,5 +1,3 @@ -// run-pass - #![allow(dead_code)] #![allow(unused_assignments)] #![allow(unused_variables)] @@ -33,7 +31,7 @@ fn assignment() { let x; if true { - x = BadDefault::default(); + x = BadDefault::default(); //~ ERROR Fallback to `!` } else { x = return; } @@ -45,13 +43,13 @@ fn assignment_rev() { if true { x = return; } else { - x = BadDefault::default(); + x = BadDefault::default(); //~ ERROR Fallback to `!` } } fn if_then_else() { let _x = if true { - BadDefault::default() + BadDefault::default() //~ ERROR Fallback to `!` } else { return; }; @@ -61,19 +59,19 @@ fn if_then_else_rev() { let _x = if true { return; } else { - BadDefault::default() + BadDefault::default() //~ ERROR Fallback to `!` }; } fn match_arm() { - let _x = match Ok(BadDefault::default()) { + let _x = match Ok(BadDefault::default()) { //~ ERROR Fallback to `!` Ok(v) => v, Err(()) => return, }; } fn match_arm_rev() { - let _x = match Ok(BadDefault::default()) { + let _x = match Ok(BadDefault::default()) { //~ ERROR Fallback to `!` Err(()) => return, Ok(v) => v, }; @@ -84,7 +82,7 @@ fn loop_break() { if false { break return; } else { - break BadDefault::default(); + break BadDefault::default(); //~ ERROR Fallback to `!` } }; } @@ -94,7 +92,7 @@ fn loop_break_rev() { if false { break return; } else { - break BadDefault::default(); + break BadDefault::default(); //~ ERROR Fallback to `!` } }; } diff --git a/src/test/ui/never_type/diverging-fallback-control-flow.stderr b/src/test/ui/never_type/diverging-fallback-control-flow.stderr new file mode 100644 index 0000000000000..a3154b92bcb20 --- /dev/null +++ b/src/test/ui/never_type/diverging-fallback-control-flow.stderr @@ -0,0 +1,209 @@ +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:34:13 + | +LL | x = BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:34:13 + | +LL | x = BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:36:13 + | +LL | x = return; + | ^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:46:13 + | +LL | x = BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:46:13 + | +LL | x = BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:44:13 + | +LL | x = return; + | ^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:52:9 + | +LL | BadDefault::default() + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:52:9 + | +LL | BadDefault::default() + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:54:9 + | +LL | return; + | ^^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:62:9 + | +LL | BadDefault::default() + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:62:9 + | +LL | BadDefault::default() + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:59:22 + | +LL | let _x = if true { + | ______________________^ +LL | | return; +LL | | } else { + | |_____^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:67:23 + | +LL | let _x = match Ok(BadDefault::default()) { + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:67:23 + | +LL | let _x = match Ok(BadDefault::default()) { + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:67:14 + | +LL | let _x = match Ok(BadDefault::default()) { + | ______________^ +LL | | Ok(v) => v, +LL | | Err(()) => return, +LL | | }; + | |_____^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:74:23 + | +LL | let _x = match Ok(BadDefault::default()) { + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:74:23 + | +LL | let _x = match Ok(BadDefault::default()) { + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:75:20 + | +LL | Err(()) => return, + | ^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:85:19 + | +LL | break BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:85:19 + | +LL | break BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:83:19 + | +LL | break return; + | ^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: Fallback to `!` may introduce undefined behavior + --> $DIR/diverging-fallback-control-flow.rs:95:19 + | +LL | break BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^^^ + | +note: the type parameter Self here was inferred to `!` + --> $DIR/diverging-fallback-control-flow.rs:95:19 + | +LL | break BadDefault::default(); + | ^^^^^^^^^^^^^^^^^^^ +note: (type parameter defined here) + --> $DIR/diverging-fallback-control-flow.rs:14:1 + | +LL | / trait BadDefault { +LL | | fn default() -> Self; +LL | | } + | |_^ +note: ... due to this expression evaluating to `!` + --> $DIR/diverging-fallback-control-flow.rs:93:19 + | +LL | break return; + | ^^^^^^ + = note: If you want the `!` type to be used here, add explicit type annotations + +error: aborting due to 8 previous errors + From 643a708d05458830e7e185adfb3e74464d6d699d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 18 Jan 2020 16:58:05 -0500 Subject: [PATCH 17/24] Unconditionally enable never-type fallback for Crater --- src/librustc/ty/context.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index a51f0f7f24c36..ff69f2e06ad62 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -2286,7 +2286,8 @@ impl<'tcx> TyCtxt<'tcx> { #[inline] pub fn mk_diverging_default(self) -> Ty<'tcx> { - if self.features().never_type_fallback { self.types.never } else { self.types.unit } + self.types.never + //if self.features().never_type_fallback { self.types.never } else { self.types.unit } } #[inline] From a81c513dd14e6bd72a74616b684b89c403c22536 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 18 Jan 2020 20:18:38 -0500 Subject: [PATCH 18/24] Bail out on inference variables in function arguments --- src/librustc_typeck/check/never_compat.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 39cb076f6ec4f..672eb7b2f6fab 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -7,6 +7,7 @@ use rustc::ty::fold::TypeFolder; use rustc::ty::{Ty, TyCtxt, TyVid}; use rustc_data_structures::fx::FxHashMap; use rustc_hir::HirId; +use std::borrow::Cow; /// Code to detect cases where using `!` (never-type) fallback instead of `()` fallback /// may result in the introduction of undefined behavior; @@ -302,6 +303,27 @@ impl<'tcx> NeverCompatHandler<'tcx> { } }; + let args_infer = match path.args.as_ref().unwrap() { + Cow::Borrowed(b) => b.iter().any(|ty| { + fcx.infcx + .unresolved_type_vars(&fcx.infcx.resolve_vars_if_possible(ty)) + .is_some() + }), + Cow::Owned(o) => fcx + .infcx + .unresolved_type_vars(&fcx.infcx.resolve_vars_if_possible(o)) + .is_some(), + }; + + if args_infer { + debug!( + "pre_fallback: skipping due to inference vars in fn {:?} args {:?}", + ty_resolved, + path.args.unwrap() + ); + return None; + } + // Any method call with inference variables in its substs // could potentially be affected by fallback. if fcx.infcx.unresolved_type_vars(fn_substs).is_some() { From 9eb803a4bc011ba8088ccd505b4518ead3363955 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 19 Jan 2020 10:19:58 -0500 Subject: [PATCH 19/24] Run never-compat post_fallback after errors are reported --- src/librustc_typeck/check/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index e5a63f6540bdb..56f9ec0bda720 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -1149,8 +1149,6 @@ fn typeck_tables_of_with_fallback<'tcx>( // See if we can make any more progress. fcx.select_obligations_where_possible(fallback_has_occurred, |_| {}); - never_compat.post_fallback(&fcx); - // Even though coercion casts provide type hints, we check casts after fallback for // backwards compatibility. This makes fallback a stronger type hint than a cast coercion. fcx.check_casts(); @@ -1168,6 +1166,10 @@ fn typeck_tables_of_with_fallback<'tcx>( fcx.select_all_obligations_or_error(); + // Report other errors in preference to never-type + // fallback errors. + never_compat.post_fallback(&fcx); + if fn_decl.is_some() { fcx.regionck_fn(id, body); } else { From 78a831fb6bbb6a20fc9011a212dc965e7f96400e Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 19 Jan 2020 10:20:31 -0500 Subject: [PATCH 20/24] Bail out if errors have occured --- src/librustc_typeck/check/never_compat.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 672eb7b2f6fab..90f62f9bb8d16 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -399,6 +399,11 @@ impl<'tcx> NeverCompatHandler<'tcx> { /// We look for any "questionable" calls, generating diagnostic /// message for them. pub fn post_fallback(self, fcx: &FnCtxt<'a, 'tcx>) { + if fcx.tcx.sess.has_errors() { + debug!("post_fallback: sess has errors, bailing out"); + return; + } + let tcx = fcx.tcx; for (call_id, path) in &self.unresolved_paths { debug!( From 3aefe613351725372440a0c334badefd1901ec45 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 19 Jan 2020 10:20:42 -0500 Subject: [PATCH 21/24] Never consider built-in constructor fns to be 'questionable' --- src/librustc_typeck/check/never_compat.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 90f62f9bb8d16..16c62d09f4d41 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -209,9 +209,14 @@ fn find_questionable_call<'a, 'tcx>( // (e.g. `Box::<_>::new` separately from determining that a method call // is occurring). We use this check to skip over `InferredPaths` for // non-method-calls (e.g. struct constructors). - if let ty::FnDef(_, substs) = ty.kind { + if let ty::FnDef(did, substs) = ty.kind { debug!("find_questionable_call: Got substs: {:?}", substs); + if tcx.is_constructor(did) { + debug!("find_questionable_call: found constructor {:?}, bailing out", did); + return None; + } + // See if we can find any arguments that are definitely uninhabited. // If we can, we're done - the method call is dead, so fallback // cannot affect its runtime behavior. From 409e8d71a9a12225f8fee6a80b0d795c34e3d2e3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 19 Jan 2020 10:33:20 -0500 Subject: [PATCH 22/24] Only skip fallback when errors have been reported in the same function --- src/librustc_typeck/check/mod.rs | 17 +++++++++++------ src/librustc_typeck/check/never_compat.rs | 5 ----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 56f9ec0bda720..d84673a512488 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -1164,11 +1164,12 @@ fn typeck_tables_of_with_fallback<'tcx>( fcx.require_type_is_sized(ty, span, code); } - fcx.select_all_obligations_or_error(); - - // Report other errors in preference to never-type - // fallback errors. - never_compat.post_fallback(&fcx); + if !fcx.select_all_obligations_or_error() { + // If we just reported some errors, don't run + // never-type fallbakc - inference variables may + // be in a weird state, leading to spurious errors + never_compat.post_fallback(&fcx); + } if fn_decl.is_some() { fcx.regionck_fn(id, body); @@ -3473,10 +3474,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { true } - fn select_all_obligations_or_error(&self) { + /// Returns whether or not errors were reported + fn select_all_obligations_or_error(&self) -> bool { debug!("select_all_obligations_or_error"); if let Err(errors) = self.fulfillment_cx.borrow_mut().select_all_or_error(&self) { self.report_fulfillment_errors(&errors, self.inh.body_id, false); + true + } else { + false } } diff --git a/src/librustc_typeck/check/never_compat.rs b/src/librustc_typeck/check/never_compat.rs index 16c62d09f4d41..c8c0f65717027 100644 --- a/src/librustc_typeck/check/never_compat.rs +++ b/src/librustc_typeck/check/never_compat.rs @@ -404,11 +404,6 @@ impl<'tcx> NeverCompatHandler<'tcx> { /// We look for any "questionable" calls, generating diagnostic /// message for them. pub fn post_fallback(self, fcx: &FnCtxt<'a, 'tcx>) { - if fcx.tcx.sess.has_errors() { - debug!("post_fallback: sess has errors, bailing out"); - return; - } - let tcx = fcx.tcx; for (call_id, path) in &self.unresolved_paths { debug!( From 39af89661783369795c1478cc8a693ed2e456b06 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 19 Jan 2020 10:40:29 -0500 Subject: [PATCH 23/24] Update test output --- .../associated-types-ICE-when-projecting-out-of-err.rs | 2 +- .../associated-types-ICE-when-projecting-out-of-err.stderr | 4 ++-- src/test/ui/never_type/feature-gate-never_type_fallback.rs | 2 +- .../ui/never_type/feature-gate-never_type_fallback.stderr | 6 +++--- src/test/ui/never_type/issue-13352.stderr | 6 +++--- src/test/ui/never_type/issue-2149.stderr | 6 +++--- .../different_defining_uses_never_type.stderr | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.rs b/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.rs index 707bcac78bf92..408e55f890317 100644 --- a/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.rs +++ b/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.rs @@ -21,5 +21,5 @@ trait Add { fn ice(a: A) { let r = loop {}; r = r + a; - //~^ ERROR the trait bound `(): Add` is not satisfied + //~^ ERROR the trait bound `!: Add` is not satisfied } diff --git a/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.stderr b/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.stderr index 8c3463a2832e0..f85c8a1a515a5 100644 --- a/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.stderr +++ b/src/test/ui/associated-types/associated-types-ICE-when-projecting-out-of-err.stderr @@ -1,8 +1,8 @@ -error[E0277]: the trait bound `(): Add` is not satisfied +error[E0277]: the trait bound `!: Add` is not satisfied --> $DIR/associated-types-ICE-when-projecting-out-of-err.rs:23:11 | LL | r = r + a; - | ^ the trait `Add` is not implemented for `()` + | ^ the trait `Add` is not implemented for `!` error: aborting due to previous error diff --git a/src/test/ui/never_type/feature-gate-never_type_fallback.rs b/src/test/ui/never_type/feature-gate-never_type_fallback.rs index 3b896ec9d70c2..1858d8fae0604 100644 --- a/src/test/ui/never_type/feature-gate-never_type_fallback.rs +++ b/src/test/ui/never_type/feature-gate-never_type_fallback.rs @@ -7,6 +7,6 @@ fn main() {} trait T {} fn should_ret_unit() -> impl T { - //~^ ERROR the trait bound `(): T` is not satisfied + //~^ ERROR the trait bound `!: T` is not satisfied panic!() } diff --git a/src/test/ui/never_type/feature-gate-never_type_fallback.stderr b/src/test/ui/never_type/feature-gate-never_type_fallback.stderr index 77288f1badac5..b2d642d4f6772 100644 --- a/src/test/ui/never_type/feature-gate-never_type_fallback.stderr +++ b/src/test/ui/never_type/feature-gate-never_type_fallback.stderr @@ -1,11 +1,11 @@ -error[E0277]: the trait bound `(): T` is not satisfied +error[E0277]: the trait bound `!: T` is not satisfied --> $DIR/feature-gate-never_type_fallback.rs:9:25 | LL | fn should_ret_unit() -> impl T { - | ^^^^^^ the trait `T` is not implemented for `()` + | ^^^^^^ the trait `T` is not implemented for `!` LL | LL | panic!() - | -------- this returned value is of type `()` + | -------- this returned value is of type `!` | = note: the return type of a function must have a statically known size = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) diff --git a/src/test/ui/never_type/issue-13352.stderr b/src/test/ui/never_type/issue-13352.stderr index 58ac74be3e3b7..da914a2b49ce7 100644 --- a/src/test/ui/never_type/issue-13352.stderr +++ b/src/test/ui/never_type/issue-13352.stderr @@ -1,10 +1,10 @@ -error[E0277]: cannot add `()` to `usize` +error[E0277]: cannot add `!` to `usize` --> $DIR/issue-13352.rs:9:13 | LL | 2_usize + (loop {}); - | ^ no implementation for `usize + ()` + | ^ no implementation for `usize + !` | - = help: the trait `std::ops::Add<()>` is not implemented for `usize` + = help: the trait `std::ops::Add` is not implemented for `usize` error: aborting due to previous error diff --git a/src/test/ui/never_type/issue-2149.stderr b/src/test/ui/never_type/issue-2149.stderr index 9645244751dab..bbffb99f019dc 100644 --- a/src/test/ui/never_type/issue-2149.stderr +++ b/src/test/ui/never_type/issue-2149.stderr @@ -1,10 +1,10 @@ -error[E0277]: cannot add `std::vec::Vec` to `()` +error[E0277]: cannot add `std::vec::Vec` to `!` --> $DIR/issue-2149.rs:8:33 | LL | for elt in self { r = r + f(*elt); } - | ^ no implementation for `() + std::vec::Vec` + | ^ no implementation for `! + std::vec::Vec` | - = help: the trait `std::ops::Add>` is not implemented for `()` + = help: the trait `std::ops::Add>` is not implemented for `!` error[E0599]: no method named `bind` found for array `[&str; 1]` in the current scope --> $DIR/issue-2149.rs:13:12 diff --git a/src/test/ui/type-alias-impl-trait/different_defining_uses_never_type.stderr b/src/test/ui/type-alias-impl-trait/different_defining_uses_never_type.stderr index 5be656e8f4461..e6c7b566b4428 100644 --- a/src/test/ui/type-alias-impl-trait/different_defining_uses_never_type.stderr +++ b/src/test/ui/type-alias-impl-trait/different_defining_uses_never_type.stderr @@ -4,7 +4,7 @@ error: concrete type differs from previous defining opaque type use LL | / fn bar() -> Foo { LL | | panic!() LL | | } - | |_^ expected `&'static str`, got `()` + | |_^ expected `&'static str`, got `!` | note: previous use here --> $DIR/different_defining_uses_never_type.rs:8:1 @@ -20,7 +20,7 @@ error: concrete type differs from previous defining opaque type use LL | / fn boo() -> Foo { LL | | loop {} LL | | } - | |_^ expected `&'static str`, got `()` + | |_^ expected `&'static str`, got `!` | note: previous use here --> $DIR/different_defining_uses_never_type.rs:8:1 From c82ea6069a84a25574648d2b28733c2a4414d24c Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 19 Jan 2020 10:41:14 -0500 Subject: [PATCH 24/24] Add explicit type annotations for `vec.push(break)` test This code was being correctly matched by the never-type fallback lint. In this case, the fallback is actually safe - constructing a `Vec` is perfectly fine. However, the lint doesn't know that - without knowing about the inner details of `Vec`, there's no way of knowing that changing `Vec::<()>::new()` to `Vec::::new()` is safe. I think this should be sufficiently rare in practice as to not be an issue. If a Vec<_> gets inferred to Vec due to fallback, you must have never pushed anything to it (or else fallback would not have run due to the type variable being constrained). If the error does come up, adding type annotations is sufficient to fix the problem - using `Vec<()>`, `Vec or (before the `!` type is stabilized) `Vec = Vec::new(); loop { v.push(break) } }