From 2f95ecfdd63c2ec27067d0cf3e8eb1155b872af7 Mon Sep 17 00:00:00 2001 From: Boxy Uwu Date: Wed, 3 Dec 2025 14:53:03 +0000 Subject: [PATCH] reviews --- compiler/rustc_hir_typeck/src/coercion.rs | 89 +++++++++++-------- .../hr_alias_normalization_leaking_vars.rs | 2 + ...hr_alias_normalization_leaking_vars.stderr | 12 +-- tests/ui/coercion/leak_check_fndef_lub.rs | 4 +- .../leak_check_fndef_lub_deadcode_breakage.rs | 4 +- ...k_check_fndef_lub_deadcode_breakage.stderr | 2 +- .../lub_closures_before_fnptr_coercion.rs | 70 --------------- 7 files changed, 61 insertions(+), 122 deletions(-) delete mode 100644 tests/ui/coercion/lub_closures_before_fnptr_coercion.rs diff --git a/compiler/rustc_hir_typeck/src/coercion.rs b/compiler/rustc_hir_typeck/src/coercion.rs index 6daeb917014c..99dbb1958cf6 100644 --- a/compiler/rustc_hir_typeck/src/coercion.rs +++ b/compiler/rustc_hir_typeck/src/coercion.rs @@ -113,9 +113,15 @@ fn success<'tcx>( Ok(InferOk { value: (adj, target), obligations }) } -enum LeakCheck { +/// Whether to force a leak check to occur in `Coerce::unify_raw`. +/// Note that leak checks may still occur evn with `ForceLeakCheck::No`. +/// +/// FIXME: We may want to change type relations to always leak-check +/// after exiting a binder, at which point we will always do so and +/// no longer need to handle this explicitly +enum ForceLeakCheck { Yes, - Default, + No, } impl<'f, 'tcx> Coerce<'f, 'tcx> { @@ -132,7 +138,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { &self, a: Ty<'tcx>, b: Ty<'tcx>, - leak_check: LeakCheck, + leak_check: ForceLeakCheck, ) -> InferResult<'tcx, Ty<'tcx>> { debug!("unify(a: {:?}, b: {:?}, use_lub: {})", a, b, self.use_lub); self.commit_if_ok(|snapshot| { @@ -176,10 +182,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { // In order to actually ensure that equating the binders *does* // result in equal binders, and that the lhs is actually a supertype // of the rhs, we must perform a leak check here. - // - // FIXME: Type relations should handle leak checks - // themselves whenever a binder is entered. - if matches!(leak_check, LeakCheck::Yes) { + if matches!(leak_check, ForceLeakCheck::Yes) { self.leak_check(outer_universe, Some(snapshot))?; } @@ -188,7 +191,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { } /// Unify two types (using sub or lub). - fn unify(&self, a: Ty<'tcx>, b: Ty<'tcx>, leak_check: LeakCheck) -> CoerceResult<'tcx> { + fn unify(&self, a: Ty<'tcx>, b: Ty<'tcx>, leak_check: ForceLeakCheck) -> CoerceResult<'tcx> { self.unify_raw(a, b, leak_check) .and_then(|InferOk { value: ty, obligations }| success(vec![], ty, obligations)) } @@ -200,7 +203,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { b: Ty<'tcx>, adjustments: impl IntoIterator>, final_adjustment: Adjust, - leak_check: LeakCheck, + leak_check: ForceLeakCheck, ) -> CoerceResult<'tcx> { self.unify_raw(a, b, leak_check).and_then(|InferOk { value: ty, obligations }| { success( @@ -231,7 +234,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { ); } else { // Otherwise the only coercion we can do is unification. - return self.unify(a, b, LeakCheck::Default); + return self.unify(a, b, ForceLeakCheck::No); } } @@ -265,7 +268,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { return self.coerce_to_raw_ptr(a, b, b_mutbl); } ty::Ref(r_b, _, mutbl_b) => { - return self.coerce_to_ref(a, b, mutbl_b, r_b); + return self.coerce_to_ref(a, b, r_b, mutbl_b); } ty::Adt(pin, _) if self.tcx.features().pin_ergonomics() @@ -301,7 +304,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { } _ => { // Otherwise, just use unification rules. - self.unify(a, b, LeakCheck::Default) + self.unify(a, b, ForceLeakCheck::No) } } } @@ -325,12 +328,19 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { )); }; - let target_ty = self.use_lub.then(|| self.next_ty_var(self.cause.span)).unwrap_or(b); - - push_coerce_obligation(a, target_ty); - if self.use_lub { + let target_ty = if self.use_lub { + // When computing the lub, we create a new target + // and coerce both `a` and `b` to it. + let target_ty = self.next_ty_var(self.cause.span); + push_coerce_obligation(a, target_ty); push_coerce_obligation(b, target_ty); - } + target_ty + } else { + // When subtyping, we don't need to create a new target + // as we only coerce `a` to `b`. + push_coerce_obligation(a, b); + b + }; debug!( "coerce_from_inference_variable: two inference variables, target_ty={:?}, obligations={:?}", @@ -340,7 +350,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { } else { // One unresolved type variable: just apply subtyping, we may be able // to do something useful. - self.unify(a, b, LeakCheck::Default) + self.unify(a, b, ForceLeakCheck::No) } } @@ -355,8 +365,8 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { &self, a: Ty<'tcx>, b: Ty<'tcx>, - mutbl_b: hir::Mutability, r_b: ty::Region<'tcx>, + mutbl_b: hir::Mutability, ) -> CoerceResult<'tcx> { debug!("coerce_to_ref(a={:?}, b={:?})", a, b); debug_assert!(self.shallow_resolve(a) == a); @@ -367,7 +377,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { coerce_mutbls(mutbl, mutbl_b)?; (r_a, ty::TypeAndMut { ty, mutbl }) } - _ => return self.unify(a, b, LeakCheck::Default), + _ => return self.unify(a, b, ForceLeakCheck::No), }; // Look at each step in the `Deref` chain and check if @@ -418,7 +428,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { // the `Target` type with the pointee of `b`. This is necessary // to properly account for the differing variances of the pointees // of `&` vs `&mut` references. - match self.unify_raw(autorefd_deref_ty, b, LeakCheck::Default) { + match self.unify_raw(autorefd_deref_ty, b, ForceLeakCheck::No) { Ok(ok) => Some(ok), Err(err) => { if first_error.is_none() { @@ -473,10 +483,13 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { obligations.extend(o); obligations.extend(autoderef.into_obligations()); + assert!( + matches!(coerced_a.kind(), ty::Ref(..)), + "expected a ref type, got {:?}", + coerced_a + ); + // Now apply the autoref - let ty::Ref(..) = coerced_a.kind() else { - span_bug!(self.cause.span, "expected a ref type, got {:?}", coerced_a); - }; let mutbl = AutoBorrowMutability::new(mutbl_b, self.allow_two_phase); adjustments .push(Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(mutbl)), target: coerced_a }); @@ -615,7 +628,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { target, reborrow.into_iter().flat_map(|(deref, autoref)| [deref, autoref]), Adjust::Pointer(PointerCoercion::Unsize), - LeakCheck::Default, + ForceLeakCheck::No, )?; // Create an obligation for `Source: CoerceUnsized`. @@ -830,7 +843,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { // To complete the reborrow, we need to make sure we can unify the inner types, and if so we // add the adjustments. - self.unify_and(a, b, [], Adjust::ReborrowPin(mut_b), LeakCheck::Default) + self.unify_and(a, b, [], Adjust::ReborrowPin(mut_b), ForceLeakCheck::No) } fn coerce_from_fn_pointer( @@ -846,9 +859,9 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { ty::FnPtr(_, b_hdr) if a_sig.safety().is_safe() && b_hdr.safety.is_unsafe() => { let a = self.tcx.safe_to_unsafe_fn_ty(a_sig); let adjust = Adjust::Pointer(PointerCoercion::UnsafeFnPointer); - self.unify_and(a, b, [], adjust, LeakCheck::Yes) + self.unify_and(a, b, [], adjust, ForceLeakCheck::Yes) } - _ => self.unify(a, b, LeakCheck::Yes), + _ => self.unify(a, b, ForceLeakCheck::Yes), } } @@ -861,20 +874,18 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { ty::FnPtr(_, b_hdr) => { let a_sig = self.sig_for_fn_def_coercion(a, Some(b_hdr.safety))?; - // FIXME: we shouldn't be normalizing here as coercion is inside of - // a probe. This can probably cause ICEs. let InferOk { value: a_sig, mut obligations } = self.at(&self.cause, self.param_env).normalize(a_sig); let a = Ty::new_fn_ptr(self.tcx, a_sig); let adjust = Adjust::Pointer(PointerCoercion::ReifyFnPointer(b_hdr.safety)); let InferOk { value, obligations: o2 } = - self.unify_and(a, b, [], adjust, LeakCheck::Yes)?; + self.unify_and(a, b, [], adjust, ForceLeakCheck::Yes)?; obligations.extend(o2); Ok(InferOk { value, obligations }) } - _ => self.unify(a, b, LeakCheck::Default), + _ => self.unify(a, b, ForceLeakCheck::No), } } @@ -893,9 +904,9 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { debug!("coerce_closure_to_fn(a={:?}, b={:?}, pty={:?})", a, b, pointer_ty); let adjust = Adjust::Pointer(PointerCoercion::ClosureFnPointer(safety)); - self.unify_and(pointer_ty, b, [], adjust, LeakCheck::Default) + self.unify_and(pointer_ty, b, [], adjust, ForceLeakCheck::No) } - _ => self.unify(a, b, LeakCheck::Default), + _ => self.unify(a, b, ForceLeakCheck::No), } } @@ -912,7 +923,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { let (is_ref, mt_a) = match *a.kind() { ty::Ref(_, ty, mutbl) => (true, ty::TypeAndMut { ty, mutbl }), ty::RawPtr(ty, mutbl) => (false, ty::TypeAndMut { ty, mutbl }), - _ => return self.unify(a, b, LeakCheck::Default), + _ => return self.unify(a, b, ForceLeakCheck::No), }; coerce_mutbls(mt_a.mutbl, mutbl_b)?; @@ -927,7 +938,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { b, [Adjustment { kind: Adjust::Deref(None), target: mt_a.ty }], Adjust::Borrow(AutoBorrow::RawPtr(mutbl_b)), - LeakCheck::Default, + ForceLeakCheck::No, ) } else if mt_a.mutbl != mutbl_b { self.unify_and( @@ -935,10 +946,10 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { b, [], Adjust::Pointer(PointerCoercion::MutToConstPointer), - LeakCheck::Default, + ForceLeakCheck::No, ) } else { - self.unify(a_raw, b, LeakCheck::Default) + self.unify(a_raw, b, ForceLeakCheck::No) } } } @@ -1037,7 +1048,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // We don't ever need two-phase here since we throw out the result of the coercion. let coerce = Coerce::new(self, cause, AllowTwoPhase::No, true); coerce.autoderef(DUMMY_SP, expr_ty).find_map(|(ty, steps)| { - self.probe(|_| coerce.unify_raw(ty, target, LeakCheck::Default)).ok().map(|_| steps) + self.probe(|_| coerce.unify_raw(ty, target, ForceLeakCheck::No)).ok().map(|_| steps) }) } diff --git a/tests/ui/coercion/hr_alias_normalization_leaking_vars.rs b/tests/ui/coercion/hr_alias_normalization_leaking_vars.rs index 43678fd876fb..71e8fdfcfa78 100644 --- a/tests/ui/coercion/hr_alias_normalization_leaking_vars.rs +++ b/tests/ui/coercion/hr_alias_normalization_leaking_vars.rs @@ -1,3 +1,5 @@ +// Regression test for #132765 +// // We have two function parameters with types: // - `&?0` // - `Box fn(>::Item)>` diff --git a/tests/ui/coercion/hr_alias_normalization_leaking_vars.stderr b/tests/ui/coercion/hr_alias_normalization_leaking_vars.stderr index 143cc6b29735..54da352c6503 100644 --- a/tests/ui/coercion/hr_alias_normalization_leaking_vars.stderr +++ b/tests/ui/coercion/hr_alias_normalization_leaking_vars.stderr @@ -1,5 +1,5 @@ error[E0277]: the trait bound `(): LendingIterator` is not satisfied - --> $DIR/hr_alias_normalization_leaking_vars.rs:32:31 + --> $DIR/hr_alias_normalization_leaking_vars.rs:34:31 | LL | LendingIterator::for_each(&(), f); | ------------------------- ^^^ the trait `LendingIterator` is not implemented for `()` @@ -7,13 +7,13 @@ LL | LendingIterator::for_each(&(), f); | required by a bound introduced by this call | help: this trait has no implementations, consider adding one - --> $DIR/hr_alias_normalization_leaking_vars.rs:24:1 + --> $DIR/hr_alias_normalization_leaking_vars.rs:26:1 | LL | trait LendingIterator { | ^^^^^^^^^^^^^^^^^^^^^ error[E0308]: mismatched types - --> $DIR/hr_alias_normalization_leaking_vars.rs:32:36 + --> $DIR/hr_alias_normalization_leaking_vars.rs:34:36 | LL | LendingIterator::for_each(&(), f); | ------------------------- ^ expected `Box`, found fn item @@ -23,19 +23,19 @@ LL | LendingIterator::for_each(&(), f); = note: expected struct `Box fn(<() as LendingIterator>::Item<'a>)>` found fn item `fn(()) {f}` note: method defined here - --> $DIR/hr_alias_normalization_leaking_vars.rs:26:8 + --> $DIR/hr_alias_normalization_leaking_vars.rs:28:8 | LL | fn for_each(&self, _f: Box)>) {} | ^^^^^^^^ --------------------------- error[E0277]: the trait bound `(): LendingIterator` is not satisfied - --> $DIR/hr_alias_normalization_leaking_vars.rs:32:36 + --> $DIR/hr_alias_normalization_leaking_vars.rs:34:36 | LL | LendingIterator::for_each(&(), f); | ^ the trait `LendingIterator` is not implemented for `()` | help: this trait has no implementations, consider adding one - --> $DIR/hr_alias_normalization_leaking_vars.rs:24:1 + --> $DIR/hr_alias_normalization_leaking_vars.rs:26:1 | LL | trait LendingIterator { | ^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/coercion/leak_check_fndef_lub.rs b/tests/ui/coercion/leak_check_fndef_lub.rs index b13a62db20be..2385117f5e2f 100644 --- a/tests/ui/coercion/leak_check_fndef_lub.rs +++ b/tests/ui/coercion/leak_check_fndef_lub.rs @@ -9,9 +9,7 @@ fn fndef_lub_leak_check() { }; } - // 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. + // Unused parameters on FnDefs are considered invariant let lhs = foo:: fn(&'static (), &'a ())>; let rhs = foo:: fn(&'a (), &'static ())>; diff --git a/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs index e2743bf39ab6..0e79ca28654e 100644 --- a/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs +++ b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.rs @@ -9,9 +9,7 @@ fn fndef_lub_leak_check() { }; } - // 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. + // Unused parameters on FnDefs are considered invariant let lhs = foo:: fn(&'static (), &'a ())>; let rhs = foo:: fn(&'a (), &'static ())>; diff --git a/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr index 60e63ee93269..f7336d49e8ca 100644 --- a/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr +++ b/tests/ui/coercion/leak_check_fndef_lub_deadcode_breakage.stderr @@ -1,5 +1,5 @@ error[E0512]: cannot transmute between types of different sizes, or dependently-sized types - --> $DIR/leak_check_fndef_lub_deadcode_breakage.rs:27:14 + --> $DIR/leak_check_fndef_lub_deadcode_breakage.rs:25:14 | LL | unsafe { std::mem::transmute::<_, ()>(lubbed) } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/coercion/lub_closures_before_fnptr_coercion.rs b/tests/ui/coercion/lub_closures_before_fnptr_coercion.rs deleted file mode 100644 index b2c421ece570..000000000000 --- a/tests/ui/coercion/lub_closures_before_fnptr_coercion.rs +++ /dev/null @@ -1,70 +0,0 @@ -//@ 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() {}