misc coercion cleanups and handle safety correctly

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),
                 }
             }

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

tests/ui/coercion/leak_check_fndef_lub.rs

@@ -0,0 +1,32 @@
+//@ check-pass
+
+fn foo<T>() {}
+
+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::<for<'a> fn(&'static (), &'a ())>;
+    let rhs = foo::<for<'a> 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: FnPtr>(_: T) {}
+
+fn main() {}

tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs

@@ -0,0 +1,29 @@
+fn foo<T>() {}
+
+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::<for<'a> fn(&'static (), &'a ())>;
+    let rhs = foo::<for<'a> 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() {}

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`.

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<C1, C2>`. This then allows
+// us to substitute `C1` and `C2` for arbitrary types in the parent scope.
+//
+// See: <https://github.com/lcnr/random-rust-snippets/issues/13>
+
+struct WaddupGamers<T, U>(Option<T>, U);
+impl<T: Leak<Unpin = U>, U> Unpin for WaddupGamers<T, U> {}
+unsafe impl<T: Leak<Send = U>, U> Send for WaddupGamers<T, U> {}
+pub trait Leak {
+    type Unpin;
+    type Send;
+}
+impl<T> Leak for (T,) {
+    type Unpin = T;
+    type Send = T;
+}
+fn define<C1, C2>() -> impl Sized {
+    WaddupGamers(None::<C1>, || ())
+}
+
+fn require_unpin<T: Unpin>(_: T) {}
+fn require_send<T: Send>(_: T) {}
+fn mk<T>() -> T { todo!() }
+
+type NameMe<T> = impl Sized;
+type NameMe2<T> = impl Sized;
+
+#[define_opaque(NameMe, NameMe2)]
+fn leak<T>()
+where
+    T: Leak<Unpin = NameMe<T>, Send = NameMe2<T>>,
+{
+    require_unpin(define::<T, for<'a> fn(&'a ())>());
+    require_send(define::<T, for<'a> fn(&'a ())>());
+
+    // This is the actual logic for lubbing two closures
+    // with syntactically different `ty::Ty`s:
+
+    // lhs: Closure<C1=T, C2=for<'a1> fn(&'a1 ())>
+    let lhs = mk::<NameMe<T>>();
+    // lhs: Closure<C1=T, C2=for<'a2> fn(&'a2 ())>
+    let rhs = mk::<NameMe2<T>>();
+
+    macro_rules! lub {
+        ($lhs:expr, $rhs:expr) => {
+            if true { $lhs } else { $rhs }
+        };
+    }
+
+    // Lubbed to either:
+    // - `Closure<C1=T, C2=for<'a> 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() {}