Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax a debug assertion for dyn principal *equality* in codegen #130863

Merged
merged 2 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3448,6 +3448,7 @@ dependencies = [
"rustc_span",
"rustc_symbol_mangling",
"rustc_target",
"rustc_trait_selection",
"rustc_type_ir",
"serde_json",
"smallvec",
Expand Down
17 changes: 3 additions & 14 deletions compiler/rustc_codegen_cranelift/src/unsize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<A>: Unsize<dyn Tr<B>>`
// 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;
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Expand Down
59 changes: 53 additions & 6 deletions compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ 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::at::ToTrace;
use rustc_trait_selection::infer::{BoundRegionConversionTime, TyCtxtInferExt};
use rustc_trait_selection::traits::{ObligationCause, ObligationCtxt};
use tracing::{debug, info};

use crate::assert_module_sources::CguReuse;
Expand Down Expand Up @@ -101,6 +104,54 @@ 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<A>: Unsize<dyn Tr<B>>`
/// 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>,
source_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
target_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
) -> bool {
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);
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,
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me regardless, but

is adding ocx.eq_trace necessary or can we simply use ocx.eq here given that we don't care about diagnostics at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExistentialTraitRef does not implement ToTrace. Wanted to avoid that since it's almost always wrong compared to using the binders for the trace, and the new trait solver has relate_no_trace which avoids that.

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

/// Retrieves the information we are losing (making dynamic) in an unsizing
/// adjustment.
///
Expand Down Expand Up @@ -133,12 +184,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}"
);

Expand Down
17 changes: 16 additions & 1 deletion compiler/rustc_trait_selection/src/traits/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -133,6 +134,20 @@ where
.map(|infer_ok| self.register_infer_ok_obligations(infer_ok))
}

pub fn eq_trace<T: Relate<TyCtxt<'tcx>>>(
&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<T: ToTrace<'tcx>>(
&self,
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/codegen/sub-principals-in-codegen.rs
Original file line number Diff line number Diff line change
@@ -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;
}
Loading