diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index 354b8e26d53..7302aa13cc4 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -707,11 +707,17 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { /// No attempt is made to resolve `ty`. pub fn type_var_diverges(&'a self, ty: Ty<'_>) -> Diverging { match *ty.kind() { - ty::Infer(ty::TyVar(vid)) => self.inner.borrow_mut().type_variables().var_diverges(vid), + ty::Infer(ty::TyVar(vid)) => self.ty_vid_diverges(vid), _ => Diverging::NotDiverging, } } + /// Returns true if the type inference variable `vid` was created + /// as a diverging type variable. No attempt is made to resolve `vid`. + pub fn ty_vid_diverges(&'a self, vid: ty::TyVid) -> Diverging { + self.inner.borrow_mut().type_variables().var_diverges(vid) + } + /// Returns the origin of the type variable identified by `vid`, or `None` /// if this is not a type variable. /// @@ -1070,6 +1076,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { }) } + /// Number of type variables created so far. + pub fn num_ty_vars(&self) -> usize { + self.inner.borrow_mut().type_variables().num_vars() + } + pub fn next_ty_var_id(&self, diverging: Diverging, origin: TypeVariableOrigin) -> TyVid { self.inner.borrow_mut().type_variables().new_var(self.universe(), diverging, origin) } diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 0fbaf81c21e..1e17ba204b2 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -1672,6 +1672,14 @@ impl<'tcx> TyS<'tcx> { matches!(self.kind(), Infer(TyVar(_))) } + #[inline] + pub fn ty_vid(&self) -> Option { + match self.kind() { + &Infer(TyVar(vid)) => Some(vid), + _ => None, + } + } + #[inline] pub fn is_ty_infer(&self) -> bool { matches!(self.kind(), Infer(_)) diff --git a/compiler/rustc_typeck/src/check/fallback.rs b/compiler/rustc_typeck/src/check/fallback.rs index 8f6cdc7bb12..2866cd98758 100644 --- a/compiler/rustc_typeck/src/check/fallback.rs +++ b/compiler/rustc_typeck/src/check/fallback.rs @@ -1,4 +1,7 @@ use crate::check::FnCtxt; +use rustc_data_structures::{ + fx::FxHashMap, graph::vec_graph::VecGraph, graph::WithSuccessors, stable_set::FxHashSet, +}; use rustc_infer::infer::type_variable::Diverging; use rustc_middle::ty::{self, Ty}; @@ -8,22 +11,30 @@ impl<'tcx> FnCtxt<'_, 'tcx> { pub(super) fn type_inference_fallback(&self) -> bool { // All type checking constraints were added, try to fallback unsolved variables. self.select_obligations_where_possible(false, |_| {}); - let mut fallback_has_occurred = false; + // Check if we have any unsolved varibales. If not, no need for fallback. + let unsolved_variables = self.unsolved_variables(); + if unsolved_variables.is_empty() { + return false; + } + + let diverging_fallback = self.calculate_diverging_fallback(&unsolved_variables); + + let mut fallback_has_occurred = false; // We do fallback in two passes, to try to generate // better error messages. // The first time, we do *not* replace opaque types. - for ty in &self.unsolved_variables() { + for ty in unsolved_variables { debug!("unsolved_variable = {:?}", ty); - fallback_has_occurred |= self.fallback_if_possible(ty); + fallback_has_occurred |= self.fallback_if_possible(ty, &diverging_fallback); } - // We now see if we can make progress. This might - // cause us to unify inference variables for opaque types, - // since we may have unified some other type variables - // during the first phase of fallback. - // This means that we only replace inference variables with their underlying - // opaque types as a last resort. + // We now see if we can make progress. This might cause us to + // unify inference variables for opaque types, since we may + // have unified some other type variables during the first + // phase of fallback. This means that we only replace + // inference variables with their underlying opaque types as a + // last resort. // // In code like this: // @@ -62,36 +73,44 @@ impl<'tcx> FnCtxt<'_, 'tcx> { // // - Unconstrained floats are replaced with with `f64`. // - // - Non-numerics get replaced with `!` when `#![feature(never_type_fallback)]` - // is enabled. Otherwise, they are replaced with `()`. + // - Non-numerics may get replaced with `()` or `!`, depending on + // how they were categorized by `calculate_diverging_fallback` + // (and the setting of `#![feature(never_type_fallback)]`). + // + // Fallback becomes very dubious if we have encountered + // type-checking errors. In that case, fallback to Error. // - // Fallback becomes very dubious if we have encountered type-checking errors. - // In that case, fallback to Error. // The return value indicates whether fallback has occurred. - fn fallback_if_possible(&self, ty: Ty<'tcx>) -> bool { + fn fallback_if_possible( + &self, + ty: Ty<'tcx>, + diverging_fallback: &FxHashMap, Ty<'tcx>>, + ) -> bool { // Careful: we do NOT shallow-resolve `ty`. We know that `ty` - // is an unsolved variable, and we determine its fallback based - // solely on how it was created, not what other type variables - // it may have been unified with since then. + // is an unsolved variable, and we determine its fallback + // based solely on how it was created, not what other type + // variables it may have been unified with since then. // - // The reason this matters is that other attempts at fallback may - // (in principle) conflict with this fallback, and we wish to generate - // a type error in that case. (However, this actually isn't true right now, - // because we're only using the builtin fallback rules. This would be - // true if we were using user-supplied fallbacks. But it's still useful - // to write the code to detect bugs.) + // The reason this matters is that other attempts at fallback + // may (in principle) conflict with this fallback, and we wish + // to generate a type error in that case. (However, this + // actually isn't true right now, because we're only using the + // builtin fallback rules. This would be true if we were using + // user-supplied fallbacks. But it's still useful to write the + // code to detect bugs.) // - // (Note though that if we have a general type variable `?T` that is then unified - // with an integer type variable `?I` that ultimately never gets - // resolved to a special integral type, `?T` is not considered unsolved, - // but `?I` is. The same is true for float variables.) + // (Note though that if we have a general type variable `?T` + // that is then unified with an integer type variable `?I` + // that ultimately never gets resolved to a special integral + // type, `?T` is not considered unsolved, but `?I` is. The + // same is true for float variables.) let fallback = match ty.kind() { _ if self.is_tainted_by_errors() => self.tcx.ty_error(), ty::Infer(ty::IntVar(_)) => self.tcx.types.i32, ty::Infer(ty::FloatVar(_)) => self.tcx.types.f64, - _ => match self.type_var_diverges(ty) { - Diverging::Diverges => self.tcx.mk_diverging_default(), - Diverging::NotDiverging => return false, + _ => match diverging_fallback.get(&ty) { + Some(&fallback_ty) => fallback_ty, + None => return false, }, }; debug!("fallback_if_possible(ty={:?}): defaulting to `{:?}`", ty, fallback); @@ -105,11 +124,10 @@ impl<'tcx> FnCtxt<'_, 'tcx> { true } - /// Second round of fallback: Unconstrained type variables - /// created from the instantiation of an opaque - /// type fall back to the opaque type itself. This is a - /// somewhat incomplete attempt to manage "identity passthrough" - /// for `impl Trait` types. + /// Second round of fallback: Unconstrained type variables created + /// from the instantiation of an opaque type fall back to the + /// opaque type itself. This is a somewhat incomplete attempt to + /// manage "identity passthrough" for `impl Trait` types. /// /// For example, in this code: /// @@ -158,4 +176,195 @@ impl<'tcx> FnCtxt<'_, 'tcx> { return false; } } + + /// The "diverging fallback" system is rather complicated. This is + /// a result of our need to balance 'do the right thing' with + /// backwards compatibility. + /// + /// "Diverging" type variables are variables created when we + /// coerce a `!` type into an unbound type variable `?X`. If they + /// never wind up being constrained, the "right and natural" thing + /// is that `?X` should "fallback" to `!`. This means that e.g. an + /// expression like `Some(return)` will ultimately wind up with a + /// type like `Option` (presuming it is not assigned or + /// constrained to have some other type). + /// + /// However, the fallback used to be `()` (before the `!` type was + /// added). Moreover, there are cases where the `!` type 'leaks + /// out' from dead code into type variables that affect live + /// code. The most common case is something like this: + /// + /// ```rust + /// match foo() { + /// 22 => Default::default(), // call this type `?D` + /// _ => return, // return has type `!` + /// } // call the type of this match `?M` + /// ``` + /// + /// Here, coercing the type `!` into `?M` will create a diverging + /// type variable `?X` where `?X <: ?M`. We also have that `?D <: + /// ?M`. If `?M` winds up unconstrained, then `?X` will + /// fallback. If it falls back to `!`, then all the type variables + /// will wind up equal to `!` -- this includes the type `?D` + /// (since `!` doesn't implement `Default`, we wind up a "trait + /// not implemented" error in code like this). But since the + /// original fallback was `()`, this code used to compile with `?D + /// = ()`. This is somewhat surprising, since `Default::default()` + /// on its own would give an error because the types are + /// insufficiently constrained. + /// + /// Our solution to this dilemma is to modify diverging variables + /// so that they can *either* fallback to `!` (the default) or to + /// `()` (the backwards compatibility case). We decide which + /// fallback to use based on whether there is a coercion pattern + /// like this: + /// + /// ``` + /// ?Diverging -> ?V + /// ?NonDiverging -> ?V + /// ?V != ?NonDiverging + /// ``` + /// + /// Here `?Diverging` represents some diverging type variable and + /// `?NonDiverging` represents some non-diverging type + /// variable. `?V` can be any type variable (diverging or not), so + /// long as it is not equal to `?NonDiverging`. + /// + /// Intuitively, what we are looking for is a case where a + /// "non-diverging" type variable (like `?M` in our example above) + /// is coerced *into* some variable `?V` that would otherwise + /// fallback to `!`. In that case, we make `?V` fallback to `!`, + /// along with anything that would flow into `?V`. + /// + /// The algorithm we use: + /// * Identify all variables that are coerced *into* by a + /// diverging variable. Do this by iterating over each + /// diverging, unsolved variable and finding all variables + /// reachable from there. Call that set `D`. + /// * Walk over all unsolved, non-diverging variables, and find + /// any variable that has an edge into `D`. + fn calculate_diverging_fallback( + &self, + unsolved_variables: &[Ty<'tcx>], + ) -> FxHashMap, Ty<'tcx>> { + debug!("calculate_diverging_fallback({:?})", unsolved_variables); + + // Construct a coercion graph where an edge `A -> B` indicates + // a type variable is that is coerced + let coercion_graph = self.create_coercion_graph(); + + // Extract the unsolved type inference variable vids; note that some + // unsolved variables are integer/float variables and are excluded. + let unsolved_vids: Vec<_> = + unsolved_variables.iter().filter_map(|ty| ty.ty_vid()).collect(); + + // Find all type variables that are reachable from a diverging + // type variable. These will typically default to `!`, unless + // we find later that they are *also* reachable from some + // other type variable outside this set. + let mut roots_reachable_from_diverging = FxHashSet::default(); + let mut diverging_vids = vec![]; + let mut non_diverging_vids = vec![]; + for &unsolved_vid in &unsolved_vids { + debug!( + "calculate_diverging_fallback: unsolved_vid={:?} diverges={:?}", + unsolved_vid, + self.infcx.ty_vid_diverges(unsolved_vid) + ); + match self.infcx.ty_vid_diverges(unsolved_vid) { + Diverging::Diverges => { + diverging_vids.push(unsolved_vid); + let root_vid = self.infcx.root_var(unsolved_vid); + debug!( + "calculate_diverging_fallback: root_vid={:?} reaches {:?}", + root_vid, + coercion_graph.depth_first_search(root_vid).collect::>() + ); + roots_reachable_from_diverging + .extend(coercion_graph.depth_first_search(root_vid)); + } + Diverging::NotDiverging => { + non_diverging_vids.push(unsolved_vid); + } + } + } + debug!( + "calculate_diverging_fallback: roots_reachable_from_diverging={:?}", + roots_reachable_from_diverging, + ); + + // Find all type variables N0 that are not reachable from a + // diverging variable, and then compute the set reachable from + // N0, which we call N. These are the *non-diverging* type + // variables. (Note that this set consists of "root variables".) + let mut roots_reachable_from_non_diverging = FxHashSet::default(); + for &non_diverging_vid in &non_diverging_vids { + let root_vid = self.infcx.root_var(non_diverging_vid); + if roots_reachable_from_diverging.contains(&root_vid) { + continue; + } + roots_reachable_from_non_diverging.extend(coercion_graph.depth_first_search(root_vid)); + } + debug!( + "calculate_diverging_fallback: roots_reachable_from_non_diverging={:?}", + roots_reachable_from_non_diverging, + ); + + // For each diverging variable, figure out whether it can + // reach a member of N. If so, it falls back to `()`. Else + // `!`. + let mut diverging_fallback = FxHashMap::default(); + for &diverging_vid in &diverging_vids { + let diverging_ty = self.tcx.mk_ty_var(diverging_vid); + let root_vid = self.infcx.root_var(diverging_vid); + let can_reach_non_diverging = coercion_graph + .depth_first_search(root_vid) + .any(|n| roots_reachable_from_non_diverging.contains(&n)); + if can_reach_non_diverging { + debug!("fallback to (): {:?}", diverging_vid); + diverging_fallback.insert(diverging_ty, self.tcx.types.unit); + } else { + debug!("fallback to !: {:?}", diverging_vid); + diverging_fallback.insert(diverging_ty, self.tcx.mk_diverging_default()); + } + } + + diverging_fallback + } + + /// Returns a graph whose nodes are (unresolved) inference variables and where + /// an edge `?A -> ?B` indicates that the variable `?A` is coerced to `?B`. + fn create_coercion_graph(&self) -> VecGraph { + let pending_obligations = self.fulfillment_cx.borrow_mut().pending_obligations(); + debug!("create_coercion_graph: pending_obligations={:?}", pending_obligations); + let coercion_edges: Vec<(ty::TyVid, ty::TyVid)> = pending_obligations + .into_iter() + .filter_map(|obligation| { + // The predicates we are looking for look like `Coerce(?A -> ?B)`. + // They will have no bound variables. + obligation.predicate.kind().no_bound_vars() + }) + .filter_map(|atom| { + if let ty::PredicateKind::Coerce(ty::CoercePredicate { a, b }) = atom { + let a_vid = self.root_vid(a)?; + let b_vid = self.root_vid(b)?; + Some((a_vid, b_vid)) + } else { + return None; + }; + + let a_vid = self.root_vid(a)?; + let b_vid = self.root_vid(b)?; + Some((a_vid, b_vid)) + }) + .collect(); + debug!("create_coercion_graph: coercion_edges={:?}", coercion_edges); + let num_ty_vars = self.infcx.num_ty_vars(); + VecGraph::new(num_ty_vars, coercion_edges) + } + + /// If `ty` is an unresolved type variable, returns its root vid. + fn root_vid(&self, ty: Ty<'tcx>) -> Option { + Some(self.infcx.root_var(self.infcx.shallow_resolve(ty).ty_vid()?)) + } } 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 ea4881049d7..f323f40ba31 100644 --- a/src/test/ui/never_type/diverging-fallback-control-flow.rs +++ b/src/test/ui/never_type/diverging-fallback-control-flow.rs @@ -4,27 +4,24 @@ #![allow(unused_assignments)] #![allow(unused_variables)] #![allow(unreachable_code)] - // Test various cases where we permit an unconstrained variable -// to fallback based on control-flow. -// -// These represent current behavior, but are pretty dubious. I would -// like to revisit these and potentially change them. --nmatsakis - +// to fallback based on control-flow. In all of these cases, +// the type variable winds up being the target of both a `!` coercion +// and a coercion from a non-`!` variable, and hence falls back to `()`. #![feature(never_type, never_type_fallback)] -trait BadDefault { +trait UnitDefault { fn default() -> Self; } -impl BadDefault for u32 { +impl UnitDefault for u32 { fn default() -> Self { 0 } } -impl BadDefault for ! { - fn default() -> ! { +impl UnitDefault for () { + fn default() -> () { panic!() } } @@ -33,7 +30,7 @@ fn assignment() { let x; if true { - x = BadDefault::default(); + x = UnitDefault::default(); } else { x = return; } @@ -45,13 +42,13 @@ fn assignment_rev() { if true { x = return; } else { - x = BadDefault::default(); + x = UnitDefault::default(); } } fn if_then_else() { let _x = if true { - BadDefault::default() + UnitDefault::default() } else { return; }; @@ -61,19 +58,19 @@ fn if_then_else_rev() { let _x = if true { return; } else { - BadDefault::default() + UnitDefault::default() }; } fn match_arm() { - let _x = match Ok(BadDefault::default()) { + let _x = match Ok(UnitDefault::default()) { Ok(v) => v, Err(()) => return, }; } fn match_arm_rev() { - let _x = match Ok(BadDefault::default()) { + let _x = match Ok(UnitDefault::default()) { Err(()) => return, Ok(v) => v, }; @@ -84,7 +81,7 @@ fn loop_break() { if false { break return; } else { - break BadDefault::default(); + break UnitDefault::default(); } }; } @@ -94,9 +91,9 @@ fn loop_break_rev() { if false { break return; } else { - break BadDefault::default(); + break UnitDefault::default(); } }; } -fn main() { } +fn main() {} diff --git a/src/test/ui/never_type/diverging-fallback-no-leak.rs b/src/test/ui/never_type/diverging-fallback-no-leak.rs new file mode 100644 index 00000000000..a3a15f0ed88 --- /dev/null +++ b/src/test/ui/never_type/diverging-fallback-no-leak.rs @@ -0,0 +1,15 @@ +#![feature(never_type_fallback)] + +fn make_unit() {} + +trait Test {} +impl Test for i32 {} +impl Test for () {} + +fn unconstrained_arg(_: T) {} + +fn main() { + // Here the type variable falls back to `!`, + // and hence we get a type error: + unconstrained_arg(return); //~ ERROR trait bound `!: Test` is not satisfied +} diff --git a/src/test/ui/never_type/diverging-fallback-no-leak.stderr b/src/test/ui/never_type/diverging-fallback-no-leak.stderr new file mode 100644 index 00000000000..27615fe4e77 --- /dev/null +++ b/src/test/ui/never_type/diverging-fallback-no-leak.stderr @@ -0,0 +1,18 @@ +error[E0277]: the trait bound `!: Test` is not satisfied + --> $DIR/diverging-fallback-no-leak.rs:14:5 + | +LL | unconstrained_arg(return); + | ^^^^^^^^^^^^^^^^^ the trait `Test` is not implemented for `!` + | + = note: this trait is implemented for `()`. + = note: this error might have been caused by changes to Rust's type-inference algorithm (see issue #48950 for more information). + = help: did you intend to use the type `()` here instead? +note: required by a bound in `unconstrained_arg` + --> $DIR/diverging-fallback-no-leak.rs:9:25 + | +LL | fn unconstrained_arg(_: T) {} + | ^^^^ required by this bound in `unconstrained_arg` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0277`. diff --git a/src/test/ui/never_type/diverging-fallback-unconstrained-return.rs b/src/test/ui/never_type/diverging-fallback-unconstrained-return.rs new file mode 100644 index 00000000000..9a6c965cefb --- /dev/null +++ b/src/test/ui/never_type/diverging-fallback-unconstrained-return.rs @@ -0,0 +1,34 @@ +// Variant of diverging-falllback-control-flow that tests +// the specific case of a free function with an unconstrained +// return type. This captures the pattern we saw in the wild +// in the objc crate, where changing the fallback from `!` to `()` +// resulted in unsoundness. +// +// check-pass + +#![feature(never_type_fallback)] + +fn make_unit() {} + +trait UnitReturn {} +impl UnitReturn for i32 {} +impl UnitReturn for () {} + +fn unconstrained_return() -> T { + unsafe { + let make_unit_fn: fn() = make_unit; + let ffi: fn() -> T = std::mem::transmute(make_unit_fn); + ffi() + } +} + +fn main() { + // In Ye Olde Days, the `T` parameter of `unconstrained_return` + // winds up "entangled" with the `!` type that results from + // `panic!`, and hence falls back to `()`. This is kind of unfortunate + // and unexpected. When we introduced the `!` type, the original + // idea was to change that fallback to `!`, but that would have resulted + // in this code no longer compiling (or worse, in some cases it injected + // unsound results). + let _ = if true { unconstrained_return() } else { panic!() }; +}