From eb75d20a55e7c6416592a6e9d4d2e7e21e08ce14 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 28 Sep 2024 14:16:05 -0400 Subject: [PATCH 1/2] Relax a debug assertion in codegen --- Cargo.lock | 1 + .../rustc_codegen_cranelift/src/unsize.rs | 17 ++------ compiler/rustc_codegen_ssa/Cargo.toml | 1 + compiler/rustc_codegen_ssa/src/base.rs | 42 ++++++++++++++++--- tests/ui/codegen/sub-principals-in-codegen.rs | 8 ++++ 5 files changed, 49 insertions(+), 20 deletions(-) create mode 100644 tests/ui/codegen/sub-principals-in-codegen.rs diff --git a/Cargo.lock b/Cargo.lock index 582b5a763e6f7..7ea8c30008816 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3448,6 +3448,7 @@ dependencies = [ "rustc_span", "rustc_symbol_mangling", "rustc_target", + "rustc_trait_selection", "rustc_type_ir", "serde_json", "smallvec", diff --git a/compiler/rustc_codegen_cranelift/src/unsize.rs b/compiler/rustc_codegen_cranelift/src/unsize.rs index 339628053a953..5c297ebfadbfd 100644 --- a/compiler/rustc_codegen_cranelift/src/unsize.rs +++ b/compiler/rustc_codegen_cranelift/src/unsize.rs @@ -2,6 +2,7 @@ //! //! [`PointerCoercion::Unsize`]: `rustc_middle::ty::adjustment::PointerCoercion::Unsize` +use rustc_codegen_ssa::base::validate_trivial_unsize; use rustc_middle::ty::print::{with_no_trimmed_paths, with_no_visible_paths}; use crate::base::codegen_panic_nounwind; @@ -34,20 +35,8 @@ pub(crate) fn unsized_info<'tcx>( let old_info = old_info.expect("unsized_info: missing old info for trait upcasting coercion"); if data_a.principal_def_id() == data_b.principal_def_id() { - // Codegen takes advantage of the additional assumption, where if the - // principal trait def id of what's being casted doesn't change, - // then we don't need to adjust the vtable at all. This - // corresponds to the fact that `dyn Tr: Unsize>` - // requires that `A = B`; we don't allow *upcasting* objects - // between the same trait with different args. If we, for - // some reason, were to relax the `Unsize` trait, it could become - // unsound, so let's assert here that the trait refs are *equal*. - // - // We can use `assert_eq` because the binders should have been anonymized, - // and because higher-ranked equality now requires the binders are equal. - debug_assert_eq!( - data_a.principal(), - data_b.principal(), + debug_assert!( + validate_trivial_unsize(fx.tcx, data_a, data_b), "NOP unsize vtable changed principal trait ref: {data_a} -> {data_b}" ); return old_info; diff --git a/compiler/rustc_codegen_ssa/Cargo.toml b/compiler/rustc_codegen_ssa/Cargo.toml index 81590674ec7c1..346f07dfa0a62 100644 --- a/compiler/rustc_codegen_ssa/Cargo.toml +++ b/compiler/rustc_codegen_ssa/Cargo.toml @@ -34,6 +34,7 @@ rustc_session = { path = "../rustc_session" } rustc_span = { path = "../rustc_span" } rustc_symbol_mangling = { path = "../rustc_symbol_mangling" } rustc_target = { path = "../rustc_target" } +rustc_trait_selection = { path = "../rustc_trait_selection" } rustc_type_ir = { path = "../rustc_type_ir" } serde_json = "1.0.59" smallvec = { version = "1.8.1", features = ["union", "may_dangle"] } diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 5c67600e4eec1..1d39125498335 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -27,6 +27,8 @@ use rustc_session::config::{self, CrateType, EntryFnType, OptLevel, OutputType}; use rustc_span::symbol::sym; use rustc_span::{DUMMY_SP, Symbol}; use rustc_target::abi::FIRST_VARIANT; +use rustc_trait_selection::infer::TyCtxtInferExt; +use rustc_trait_selection::traits::{ObligationCause, ObligationCtxt}; use tracing::{debug, info}; use crate::assert_module_sources::CguReuse; @@ -101,6 +103,38 @@ pub fn compare_simd_types<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( bx.sext(cmp, ret_ty) } +/// Codegen takes advantage of the additional assumption, where if the +/// principal trait def id of what's being casted doesn't change, +/// then we don't need to adjust the vtable at all. This +/// corresponds to the fact that `dyn Tr: Unsize>` +/// requires that `A = B`; we don't allow *upcasting* objects +/// between the same trait with different args. If we, for +/// some reason, were to relax the `Unsize` trait, it could become +/// unsound, so let's validate here that the trait refs are subtypes. +pub fn validate_trivial_unsize<'tcx>( + tcx: TyCtxt<'tcx>, + data_a: &'tcx ty::List>, + data_b: &'tcx ty::List>, +) -> bool { + match (data_a.principal(), data_b.principal()) { + (Some(principal_a), Some(principal_b)) => { + let infcx = tcx.infer_ctxt().build(); + let ocx = ObligationCtxt::new(&infcx); + let Ok(()) = ocx.sub( + &ObligationCause::dummy(), + ty::ParamEnv::reveal_all(), + principal_a, + principal_b, + ) else { + return false; + }; + ocx.select_all_or_error().is_empty() + } + (None, None) => true, + _ => false, + } +} + /// Retrieves the information we are losing (making dynamic) in an unsizing /// adjustment. /// @@ -133,12 +167,8 @@ fn unsized_info<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( // between the same trait with different args. If we, for // some reason, were to relax the `Unsize` trait, it could become // unsound, so let's assert here that the trait refs are *equal*. - // - // We can use `assert_eq` because the binders should have been anonymized, - // and because higher-ranked equality now requires the binders are equal. - debug_assert_eq!( - data_a.principal(), - data_b.principal(), + debug_assert!( + validate_trivial_unsize(cx.tcx(), data_a, data_b), "NOP unsize vtable changed principal trait ref: {data_a} -> {data_b}" ); diff --git a/tests/ui/codegen/sub-principals-in-codegen.rs b/tests/ui/codegen/sub-principals-in-codegen.rs new file mode 100644 index 0000000000000..178c10da5968f --- /dev/null +++ b/tests/ui/codegen/sub-principals-in-codegen.rs @@ -0,0 +1,8 @@ +//@ build-pass + +// Regression test for an overly aggressive assertion in #130855. + +fn main() { + let subtype: &(dyn for<'a> Fn(&'a i32) -> &'a i32) = &|x| x; + let supertype: &(dyn Fn(&'static i32) -> &'static i32) = subtype; +} From cbb5047d35d76cd34ccf238220ccc70316f6ab77 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 30 Sep 2024 12:42:29 -0400 Subject: [PATCH 2/2] Relate binders explicitly, do a leak check too --- compiler/rustc_codegen_ssa/src/base.rs | 45 +++++++++++++------ .../src/traits/engine.rs | 17 ++++++- 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 1d39125498335..d91c0f0790d9d 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -27,7 +27,8 @@ use rustc_session::config::{self, CrateType, EntryFnType, OptLevel, OutputType}; use rustc_span::symbol::sym; use rustc_span::{DUMMY_SP, Symbol}; use rustc_target::abi::FIRST_VARIANT; -use rustc_trait_selection::infer::TyCtxtInferExt; +use rustc_trait_selection::infer::at::ToTrace; +use rustc_trait_selection::infer::{BoundRegionConversionTime, TyCtxtInferExt}; use rustc_trait_selection::traits::{ObligationCause, ObligationCtxt}; use tracing::{debug, info}; @@ -113,22 +114,38 @@ pub fn compare_simd_types<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( /// unsound, so let's validate here that the trait refs are subtypes. pub fn validate_trivial_unsize<'tcx>( tcx: TyCtxt<'tcx>, - data_a: &'tcx ty::List>, - data_b: &'tcx ty::List>, + source_data: &'tcx ty::List>, + target_data: &'tcx ty::List>, ) -> bool { - match (data_a.principal(), data_b.principal()) { - (Some(principal_a), Some(principal_b)) => { + match (source_data.principal(), target_data.principal()) { + (Some(hr_source_principal), Some(hr_target_principal)) => { let infcx = tcx.infer_ctxt().build(); + let universe = infcx.universe(); let ocx = ObligationCtxt::new(&infcx); - let Ok(()) = ocx.sub( - &ObligationCause::dummy(), - ty::ParamEnv::reveal_all(), - principal_a, - principal_b, - ) else { - return false; - }; - ocx.select_all_or_error().is_empty() + infcx.enter_forall(hr_target_principal, |target_principal| { + let source_principal = infcx.instantiate_binder_with_fresh_vars( + DUMMY_SP, + BoundRegionConversionTime::HigherRankedType, + hr_source_principal, + ); + let Ok(()) = ocx.eq_trace( + &ObligationCause::dummy(), + ty::ParamEnv::reveal_all(), + ToTrace::to_trace( + &ObligationCause::dummy(), + hr_target_principal, + hr_source_principal, + ), + target_principal, + source_principal, + ) else { + return false; + }; + if !ocx.select_all_or_error().is_empty() { + return false; + } + infcx.leak_check(universe, None).is_ok() + }) } (None, None) => true, _ => false, diff --git a/compiler/rustc_trait_selection/src/traits/engine.rs b/compiler/rustc_trait_selection/src/traits/engine.rs index de1d4ef15aced..d562692c1a865 100644 --- a/compiler/rustc_trait_selection/src/traits/engine.rs +++ b/compiler/rustc_trait_selection/src/traits/engine.rs @@ -9,12 +9,13 @@ use rustc_infer::infer::canonical::{ Canonical, CanonicalQueryResponse, CanonicalVarValues, QueryResponse, }; use rustc_infer::infer::outlives::env::OutlivesEnvironment; -use rustc_infer::infer::{DefineOpaqueTypes, InferCtxt, InferOk, RegionResolutionError}; +use rustc_infer::infer::{DefineOpaqueTypes, InferCtxt, InferOk, RegionResolutionError, TypeTrace}; use rustc_macros::extension; use rustc_middle::arena::ArenaAllocatable; use rustc_middle::traits::query::NoSolution; use rustc_middle::ty::error::TypeError; use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, Upcast, Variance}; +use rustc_type_ir::relate::Relate; use super::{FromSolverError, FulfillmentContext, ScrubbedTraitError, TraitEngine}; use crate::error_reporting::InferCtxtErrorExt; @@ -133,6 +134,20 @@ where .map(|infer_ok| self.register_infer_ok_obligations(infer_ok)) } + pub fn eq_trace>>( + &self, + cause: &ObligationCause<'tcx>, + param_env: ty::ParamEnv<'tcx>, + trace: TypeTrace<'tcx>, + expected: T, + actual: T, + ) -> Result<(), TypeError<'tcx>> { + self.infcx + .at(cause, param_env) + .eq_trace(DefineOpaqueTypes::Yes, trace, expected, actual) + .map(|infer_ok| self.register_infer_ok_obligations(infer_ok)) + } + /// Checks whether `expected` is a subtype of `actual`: `expected <: actual`. pub fn sub>( &self,