diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs index 9c4c8edd10b7..9aded3f92c76 100644 --- a/compiler/rustc_hir_typeck/src/coercion.rs +++ b/compiler/rustc_hir_typeck/src/coercion.rs @@ -1163,8 +1163,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { exprs.len() ); - // The following check fixes #88097, where the compiler erroneously - // attempted to coerce a closure type to itself via a function pointer. + // Fast Path: don't go through the coercion logic if we're coercing + // a type to itself. This is unfortunately quite perf relevant so + // we do it even though it may mask bugs in the coercion logic. if prev_ty == new_ty { return Ok(prev_ty); } @@ -1193,34 +1194,51 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if is_capturing_closure(prev_ty) || is_capturing_closure(new_ty) { (None, None) } else { - match (prev_ty.kind(), new_ty.kind()) { - (ty::FnDef(..), ty::FnDef(..)) => { - // Don't reify if the function types have a LUB, i.e., they - // are the same function and their parameters have a LUB. - match self.commit_if_ok(|_| { - // We need to eagerly handle nested obligations due to lazy norm. - if self.next_trait_solver() { - let ocx = ObligationCtxt::new(self); - let value = ocx.lub(cause, self.param_env, prev_ty, new_ty)?; - if ocx.try_evaluate_obligations().is_empty() { - Ok(InferOk { - value, - obligations: ocx.into_pending_obligations(), - }) - } else { - Err(TypeError::Mismatch) - } + let lubbed_tys = || { + self.commit_if_ok(|snapshot| { + let outer_universe = self.infcx.universe(); + + // We need to eagerly handle nested obligations due to lazy norm. + let result = if self.next_trait_solver() { + let ocx = ObligationCtxt::new(self); + let value = ocx.lub(cause, self.param_env, prev_ty, new_ty)?; + if ocx.try_evaluate_obligations().is_empty() { + Ok(InferOk { value, obligations: ocx.into_pending_obligations() }) } else { - self.at(cause, self.param_env).lub(prev_ty, new_ty) - } - }) { - // We have a LUB of prev_ty and new_ty, just return it. - Ok(ok) => return Ok(self.register_infer_ok_obligations(ok)), - Err(_) => { - (Some(prev_ty.fn_sig(self.tcx)), Some(new_ty.fn_sig(self.tcx))) + Err(TypeError::Mismatch) } + } else { + self.at(cause, self.param_env).lub(prev_ty, new_ty) + }; + + self.leak_check(outer_universe, Some(snapshot))?; + result + }) + }; + + match (prev_ty.kind(), new_ty.kind()) { + // Don't coerce pairs of fndefs or pairs of closures to fn ptrs + // if they can just be lubbed. + // + // See #88097 or `lub_closures_before_fnptr_coercion.rs` for where + // we would erroneously coerce closures to fnptrs when attempting to + // coerce a closure to itself. + (ty::FnDef(..), ty::FnDef(..)) => match lubbed_tys() { + Ok(ok) => return Ok(self.register_infer_ok_obligations(ok)), + Err(_) => (Some(prev_ty.fn_sig(self.tcx)), Some(new_ty.fn_sig(self.tcx))), + }, + (ty::Closure(_, args_a), ty::Closure(_, args_b)) => match lubbed_tys() { + Ok(ok) => return Ok(self.register_infer_ok_obligations(ok)), + Err(_) => { + let a_sig = self + .tcx + .signature_unclosure(args_a.as_closure().sig(), hir::Safety::Safe); + let b_sig = self + .tcx + .signature_unclosure(args_b.as_closure().sig(), hir::Safety::Safe); + (Some(a_sig), Some(b_sig)) } - } + }, (ty::Closure(_, args), ty::FnDef(..)) => { let b_sig = new_ty.fn_sig(self.tcx); let a_sig = @@ -1233,16 +1251,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.tcx.signature_unclosure(args.as_closure().sig(), a_sig.safety()); (Some(a_sig), Some(b_sig)) } - (ty::Closure(_, args_a), ty::Closure(_, args_b)) => ( - Some( - self.tcx - .signature_unclosure(args_a.as_closure().sig(), hir::Safety::Safe), - ), - Some( - self.tcx - .signature_unclosure(args_b.as_closure().sig(), hir::Safety::Safe), - ), - ), + // ty::FnPtr x ty::FnPtr is fine to just be handled through a normal `unify` + // call using `lub` which is what will happen on the normal path. _ => (None, None), } } diff --git a/tests/ui/coercion/issue-88097.rs b/tests/ui/coercion/issue-88097.rs index f636323d6236..a5804e3b789c 100644 --- a/tests/ui/coercion/issue-88097.rs +++ b/tests/ui/coercion/issue-88097.rs @@ -3,6 +3,9 @@ // behavior has been fixed. //@ check-pass +//@ revisions: current next +//@ ignore-compare-mode-next-solver (explicit revisions) +//@[next] compile-flags: -Znext-solver fn peculiar() -> impl Fn(u8) -> u8 { return |x| x + 1 diff --git a/tests/ui/coercion/leak_check_fndef_lub.rs b/tests/ui/coercion/leak_check_fndef_lub.rs new file mode 100644 index 000000000000..b13a62db20be --- /dev/null +++ b/tests/ui/coercion/leak_check_fndef_lub.rs @@ -0,0 +1,32 @@ +//@ check-pass + +fn foo() {} + +fn fndef_lub_leak_check() { + macro_rules! lub { + ($lhs:expr, $rhs:expr) => { + if true { $lhs } else { $rhs } + }; + } + + // These don't currently lub but could in theory one day. + // If that happens this test should be adjusted to use + // fn ptrs that can't be lub'd. + let lhs = foo:: fn(&'static (), &'a ())>; + let rhs = foo:: fn(&'a (), &'static ())>; + + // If we leak check then we know we should coerce these + // to `fn()`, if we don't leak check we may try to keep + // them as `FnDef`s which would result in a borrowck + // error. + let lubbed = lub!(lhs, rhs); + + // assert that we coerced lhs/rhs to a fn ptr + is_fnptr(lubbed); +} + +trait FnPtr {} +impl FnPtr for fn() {} +fn is_fnptr(_: T) {} + +fn main() {} diff --git a/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs new file mode 100644 index 000000000000..dda6bd6101c4 --- /dev/null +++ b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs @@ -0,0 +1,29 @@ +fn foo() {} + +fn fndef_lub_leak_check() { + macro_rules! lub { + ($lhs:expr, $rhs:expr) => { + if true { $lhs } else { $rhs } + }; + } + + // These don't currently lub but could in theory one day. + // If that happens this test should be adjusted to use + // fn ptrs that can't be lub'd. + let lhs = foo:: fn(&'static (), &'a ())>; + let rhs = foo:: fn(&'a (), &'static ())>; + + loop {} + + // If we leak check then we know we should coerce these + // to `fn()`, if we don't leak check we may try to keep + // them as `FnDef`s which would cause this code to compile + // as borrowck won't emit errors for deadcode. + let lubbed = lub!(lhs, rhs); + + // assert that `lubbed` is a ZST/`FnDef` + unsafe { std::mem::transmute::<_, ()>(lubbed) } + //~^ ERROR: cannot transmute between types of different sizes +} + +fn main() {} diff --git a/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr new file mode 100644 index 000000000000..f7336d49e8ca --- /dev/null +++ b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr @@ -0,0 +1,12 @@ +error[E0512]: cannot transmute between types of different sizes, or dependently-sized types + --> $DIR/leak_check_fndef_lub_deadcode_breakage.rs:25:14 + | +LL | unsafe { std::mem::transmute::<_, ()>(lubbed) } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: source type: `fn()` (64 bits) + = note: target type: `()` (0 bits) + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0512`. diff --git a/tests/ui/coercion/lub_closures_before_fnptr_coercion.rs b/tests/ui/coercion/lub_closures_before_fnptr_coercion.rs new file mode 100644 index 000000000000..b2c421ece570 --- /dev/null +++ b/tests/ui/coercion/lub_closures_before_fnptr_coercion.rs @@ -0,0 +1,70 @@ +//@ check-pass +//@ compile-flags: -Znext-solver + +#![feature(type_alias_impl_trait)] + +// Test that when lubbing two equal closure tys with different +// structural identities (i.e. `PartialEq::eq` on `ty::Ty` would be false) +// we don't coerce-lub to a fnptr. +// +// Most of this test is involved jank to be able to leak the hidden type +// of an opaque with a hidden type of `Closure`. This then allows +// us to substitute `C1` and `C2` for arbitrary types in the parent scope. +// +// See: + +struct WaddupGamers(Option, U); +impl, U> Unpin for WaddupGamers {} +unsafe impl, U> Send for WaddupGamers {} +pub trait Leak { + type Unpin; + type Send; +} +impl Leak for (T,) { + type Unpin = T; + type Send = T; +} +fn define() -> impl Sized { + WaddupGamers(None::, || ()) +} + +fn require_unpin(_: T) {} +fn require_send(_: T) {} +fn mk() -> T { todo!() } + +type NameMe = impl Sized; +type NameMe2 = impl Sized; + +#[define_opaque(NameMe, NameMe2)] +fn leak() +where + T: Leak, Send = NameMe2>, +{ + require_unpin(define:: fn(&'a ())>()); + require_send(define:: fn(&'a ())>()); + + // This is the actual logic for lubbing two closures + // with syntactically different `ty::Ty`s: + + // lhs: Closure fn(&'a1 ())> + let lhs = mk::>(); + // lhs: Closure fn(&'a2 ())> + let rhs = mk::>(); + + macro_rules! lub { + ($lhs:expr, $rhs:expr) => { + if true { $lhs } else { $rhs } + }; + } + + // Lubbed to either: + // - `Closure fn(&'a ())>` + // - `fn(&())` + let lubbed = lub!(lhs, rhs); + + // Use transmute to assert the size of `lubbed` is (), i.e. + // that it is a ZST closure type not a fnptr. + unsafe { std::mem::transmute::<_, ()>(lubbed) }; +} + +fn main() {}