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.

Sign up for GitHub

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

Jump to bottom

remove Subtype projections #133258

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+67 −234
Open

remove Subtype projections #133258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de6d914
remove `Subtype` projections
lcnr Nov 20, 2024
4871588
fix comment
lcnr Nov 20, 2024
808e441
update crashes
lcnr Nov 20, 2024
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: 0 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3971,7 +3971,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
}
ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subslice { .. }
| ProjectionElem::Subtype(_)
| ProjectionElem::Index(_) => kind,
},
place_ty.projection_ty(tcx, elem),
Expand Down
5 changes: 1 addition & 4 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
ProjectionElem::Downcast(..) if opt.including_downcast => return None,
ProjectionElem::Downcast(..) => (),
ProjectionElem::OpaqueCast(..) => (),
ProjectionElem::Subtype(..) => (),
ProjectionElem::Field(field, _ty) => {
// FIXME(project-rfc_2229#36): print capture precisely here.
if let Some(field) = self.is_upvar_field_projection(PlaceRef {
Expand Down Expand Up @@ -324,9 +323,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
PlaceRef { local, projection: proj_base }.ty(self.body, self.infcx.tcx)
}
ProjectionElem::Downcast(..) => place.ty(self.body, self.infcx.tcx),
ProjectionElem::Subtype(ty) | ProjectionElem::OpaqueCast(ty) => {
PlaceTy::from_ty(*ty)
}
ProjectionElem::OpaqueCast(ty) => PlaceTy::from_ty(*ty),
ProjectionElem::Field(_, field_type) => PlaceTy::from_ty(*field_type),
},
};
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
[
..,
ProjectionElem::Index(_)
| ProjectionElem::Subtype(_)
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::OpaqueCast { .. }
| ProjectionElem::Subslice { .. }
Expand Down
7 changes: 1 addition & 6 deletions compiler/rustc_borrowck/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,11 +1712,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
},
// `OpaqueCast`: only transmutes the type, so no moves there.
// `Downcast` : only changes information about a `Place` without moving.
// `Subtype` : only transmutes the type, so no moves.
// So it's safe to skip these.
ProjectionElem::OpaqueCast(_)
| ProjectionElem::Subtype(_)
| ProjectionElem::Downcast(_, _) => (),
ProjectionElem::OpaqueCast(_) | ProjectionElem::Downcast(_, _) => (),
}

place_ty = place_ty.projection_ty(tcx, elem);
Expand Down Expand Up @@ -1940,7 +1937,6 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
for (place_base, elem) in place.iter_projections().rev() {
match elem {
ProjectionElem::Index(_/*operand*/) |
ProjectionElem::Subtype(_) |
ProjectionElem::OpaqueCast(_) |
ProjectionElem::ConstantIndex { .. } |
// assigning to P[i] requires P to be valid.
Expand Down Expand Up @@ -2332,7 +2328,6 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
| ProjectionElem::Index(..)
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subslice { .. }
| ProjectionElem::Subtype(..)
| ProjectionElem::OpaqueCast { .. }
| ProjectionElem::Downcast(..) => {
let upvar_field_projection = self.is_upvar_field_projection(place);
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_borrowck/src/places_conflict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ fn place_components_conflict<'tcx>(
| (ProjectionElem::ConstantIndex { .. }, _, _)
| (ProjectionElem::Subslice { .. }, _, _)
| (ProjectionElem::OpaqueCast { .. }, _, _)
| (ProjectionElem::Subtype(_), _, _)
| (ProjectionElem::Downcast { .. }, _, _) => {
// Recursive case. This can still be disjoint on a
// further iteration if this a shallow access and
Expand Down Expand Up @@ -509,7 +508,6 @@ fn place_projection_conflict<'tcx>(
| ProjectionElem::Field(..)
| ProjectionElem::Index(..)
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subtype(_)
| ProjectionElem::OpaqueCast { .. }
| ProjectionElem::Subslice { .. }
| ProjectionElem::Downcast(..),
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_borrowck/src/prefixes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ impl<'tcx> Iterator for Prefixes<'tcx> {
| ProjectionElem::Index(_) => {
cursor = cursor_base;
}
ProjectionElem::Subtype(..) => {
panic!("Subtype projection is not allowed before borrow check")
}
ProjectionElem::Deref => {
match self.kind {
PrefixSet::Shallow => {
Expand Down
6 changes: 0 additions & 6 deletions compiler/rustc_borrowck/src/type_check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,9 +698,6 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
}
PlaceTy::from_ty(fty)
}
ProjectionElem::Subtype(_) => {
bug!("ProjectionElem::Subtype shouldn't exist in borrowck")
}
ProjectionElem::OpaqueCast(ty) => {
let ty = self.sanitize_type(place, ty);
let ty = self.cx.normalize(ty, location);
Expand Down Expand Up @@ -2700,9 +2697,6 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
| ProjectionElem::Subslice { .. } => {
// other field access
}
ProjectionElem::Subtype(_) => {
bug!("ProjectionElem::Subtype shouldn't exist in borrowck")
}
}
}
}
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_codegen_cranelift/src/base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,6 @@ pub(crate) fn codegen_place<'tcx>(
cplace = cplace.place_deref(fx);
}
PlaceElem::OpaqueCast(ty) => bug!("encountered OpaqueCast({ty}) in codegen"),
PlaceElem::Subtype(ty) => cplace = cplace.place_transmute_type(fx, fx.monomorphize(ty)),
PlaceElem::Field(field, _ty) => {
cplace = cplace.place_field(fx, field);
}
Expand Down
10 changes: 0 additions & 10 deletions compiler/rustc_codegen_cranelift/src/value_and_place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,16 +708,6 @@ impl<'tcx> CPlace<'tcx> {
}
}

/// Used for `ProjectionElem::Subtype`, `ty` has to be monomorphized before
/// passed on.
pub(crate) fn place_transmute_type(
self,
fx: &mut FunctionCx<'_, '_, 'tcx>,
ty: Ty<'tcx>,
) -> CPlace<'tcx> {
CPlace { inner: self.inner, layout: fx.layout_of(ty) }
}

pub(crate) fn place_field(
self,
fx: &mut FunctionCx<'_, '_, 'tcx>,
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::ProjectionElem::OpaqueCast(ty) => {
bug!("encountered OpaqueCast({ty}) in codegen")
}
mir::ProjectionElem::Subtype(ty) => cg_base.project_type(bx, self.monomorphize(ty)),
mir::ProjectionElem::Index(index) => {
let index = &mir::Operand::Copy(mir::Place::from(index));
let index = self.codegen_operand(bx, index);
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_const_eval/src/check_consts/qualifs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ where
ProjectionElem::Index(index) if in_local(index) => return true,

ProjectionElem::Deref
| ProjectionElem::Subtype(_)
| ProjectionElem::Field(_, _)
| ProjectionElem::OpaqueCast(_)
| ProjectionElem::ConstantIndex { .. }
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_middle::query::TyCtxtAt;
use rustc_middle::ty::layout::{
self, FnAbiError, FnAbiOfHelpers, FnAbiRequest, LayoutError, LayoutOfHelpers, TyAndLayout,
};
use rustc_middle::ty::{self, GenericArgsRef, Ty, TyCtxt, TypeFoldable, TypingEnv, Variance};
use rustc_middle::ty::{self, GenericArgsRef, Ty, TyCtxt, TypeFoldable, TypingEnv};
use rustc_middle::{mir, span_bug};
use rustc_session::Limit;
use rustc_span::Span;
Expand Down Expand Up @@ -113,8 +113,7 @@ impl<'tcx, M: Machine<'tcx>> FnAbiOfHelpers<'tcx> for InterpCx<'tcx, M> {
}
}

/// Test if it is valid for a MIR assignment to assign `src`-typed place to `dest`-typed value.
/// This test should be symmetric, as it is primarily about layout compatibility.
/// Test if it is valid for a MIR assignment to store a `src`-typed value in a `dest`-typed place.
pub(super) fn mir_assign_valid_types<'tcx>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we check for subtyping here which is not symmetric, so either this comment is wrong or the impl is. I would be open to alternative change this to try subtyping in either direction 🤷 but given that this already uses src and dest and the behavior is preexisting, this comment is not relied on rn

Copy link
Member

@RalfJung RalfJung Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I originally made the check symmetric since I didn't want to think about which direction it has to be. 😂 So it might be worth checking that all users of this function use it correctly.

I also find the comment extremely confusing. When doing an assignment, dest is a place and src is a value! So what is going on here?
What I expect is a comment like this:

/// Test if it is valid for a MIR assignment to store a `src`-typed value in a `dest`-typed place.

But I am not sure if that matches the implementation, or the users...

Copy link
Member

@RalfJung RalfJung Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Apparently I wrote that comment. Unfortunately I can't explain what I meant...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it should be src-typed value and dest-typed place 😁

tcx: TyCtxt<'tcx>,
typing_env: TypingEnv<'tcx>,
Expand All @@ -125,7 +124,7 @@ pub(super) fn mir_assign_valid_types<'tcx>(
// all normal lifetimes are erased, higher-ranked types with their
// late-bound lifetimes are still around and can lead to type
// differences.
if util::relate_types(tcx, typing_env, Variance::Covariant, src.ty, dest.ty) {
if util::sub_types(tcx, typing_env, src.ty, dest.ty) {
// Make sure the layout is equal, too -- just to be safe. Miri really
// needs layout equality. For performance reason we skip this check when
// the types are equal. Equal types *can* have different layouts when
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_const_eval/src/interpret/projection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,6 @@ where
OpaqueCast(ty) => {
span_bug!(self.cur_span(), "OpaqueCast({ty}) encountered after borrowck")
}
// We don't want anything happening here, this is here as a dummy.
Subtype(_) => base.transmute(base.layout(), self)?,
Field(field, _) => self.project_field(base, field.index())?,
Downcast(_, variant) => self.project_downcast(base, variant)?,
Deref => self.deref_pointer(&base.to_op(self)?)?.into(),
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_const_eval/src/util/compare_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ pub fn sub_types<'tcx>(

/// Returns whether `src` is a subtype of `dest`, i.e. `src <: dest`.
///
/// When validating assignments, the variance should be `Covariant`. When checking
/// during `MirPhase` >= `MirPhase::Runtime(RuntimePhase::Initial)` variance should be `Invariant`
/// because we want to check for type equality.
/// When validating assignments, the variance should be `Covariant` and
/// `sub_types` should be used instead.
pub fn relate_types<'tcx>(
tcx: TyCtxt<'tcx>,
typing_env: TypingEnv<'tcx>,
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_middle/src/mir/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,7 +1270,6 @@ fn pre_fmt_projection(projection: &[PlaceElem<'_>], fmt: &mut Formatter<'_>) ->
for &elem in projection.iter().rev() {
match elem {
ProjectionElem::OpaqueCast(_)
| ProjectionElem::Subtype(_)
| ProjectionElem::Downcast(_, _)
| ProjectionElem::Field(_, _) => {
write!(fmt, "(")?;
Expand All @@ -1293,9 +1292,6 @@ fn post_fmt_projection(projection: &[PlaceElem<'_>], fmt: &mut Formatter<'_>) ->
ProjectionElem::OpaqueCast(ty) => {
write!(fmt, " as {ty})")?;
}
ProjectionElem::Subtype(ty) => {
write!(fmt, " as subtype {ty})")?;
}
ProjectionElem::Downcast(Some(name), _index) => {
write!(fmt, " as {name})")?;
}
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_middle/src/mir/statement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ impl<V, T> ProjectionElem<V, T> {
Self::Field(_, _)
| Self::Index(_)
| Self::OpaqueCast(_)
| Self::Subtype(_)
| Self::ConstantIndex { .. }
| Self::Subslice { .. }
| Self::Downcast(_, _) => false,
Expand All @@ -73,7 +72,6 @@ impl<V, T> ProjectionElem<V, T> {
Self::Deref | Self::Index(_) => false,
Self::Field(_, _)
| Self::OpaqueCast(_)
| Self::Subtype(_)
| Self::ConstantIndex { .. }
| Self::Subslice { .. }
| Self::Downcast(_, _) => true,
Expand All @@ -99,7 +97,6 @@ impl<V, T> ProjectionElem<V, T> {
| Self::Field(_, _) => true,
Self::ConstantIndex { from_end: true, .. }
| Self::Index(_)
| Self::Subtype(_)
| Self::OpaqueCast(_)
| Self::Subslice { .. } => false,
}
Expand Down
12 changes: 0 additions & 12 deletions compiler/rustc_middle/src/mir/syntax.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,18 +1182,6 @@ pub enum ProjectionElem<V, T> {
/// Like an explicit cast from an opaque type to a concrete type, but without
/// requiring an intermediate variable.
OpaqueCast(T),

/// A `Subtype(T)` projection is applied to any `StatementKind::Assign` where
/// type of lvalue doesn't match the type of rvalue, the primary goal is making subtyping
/// explicit during optimizations and codegen.
///
/// This projection doesn't impact the runtime behavior of the program except for potentially changing
/// some type metadata of the interpreter or codegen backend.
///
/// This goal is achieved with mir_transform pass `Subtyper`, which runs right after
/// borrowchecker, as we only care about subtyping that can affect trait selection and
/// `TypeId`.
Subtype(T),
}

/// Alias for projections as they appear in places, where the base is a place
Expand Down
9 changes: 2 additions & 7 deletions compiler/rustc_middle/src/mir/tcx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl<'tcx> PlaceTy<'tcx> {
tcx: TyCtxt<'tcx>,
elem: &ProjectionElem<V, T>,
mut handle_field: impl FnMut(&Self, FieldIdx, T) -> Ty<'tcx>,
mut handle_opaque_cast_and_subtype: impl FnMut(&Self, T) -> Ty<'tcx>,
mut handle_opaque_cast: impl FnMut(&Self, T) -> Ty<'tcx>,
) -> PlaceTy<'tcx>
where
V: ::std::fmt::Debug,
Expand Down Expand Up @@ -105,12 +105,7 @@ impl<'tcx> PlaceTy<'tcx> {
PlaceTy { ty: self.ty, variant_index: Some(index) }
}
ProjectionElem::Field(f, fty) => PlaceTy::from_ty(handle_field(&self, f, fty)),
ProjectionElem::OpaqueCast(ty) => {
PlaceTy::from_ty(handle_opaque_cast_and_subtype(&self, ty))
}
ProjectionElem::Subtype(ty) => {
PlaceTy::from_ty(handle_opaque_cast_and_subtype(&self, ty))
}
ProjectionElem::OpaqueCast(ty) => PlaceTy::from_ty(handle_opaque_cast(&self, ty)),
};
debug!("projection_ty self: {:?} elem: {:?} yields: {:?}", self, elem, answer);
answer
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_middle/src/mir/visit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,11 +1142,6 @@ macro_rules! visit_place_fns {
self.visit_ty(&mut new_ty, TyContext::Location(location));
if ty != new_ty { Some(PlaceElem::OpaqueCast(new_ty)) } else { None }
}
PlaceElem::Subtype(ty) => {
let mut new_ty = ty;
self.visit_ty(&mut new_ty, TyContext::Location(location));
if ty != new_ty { Some(PlaceElem::Subtype(new_ty)) } else { None }
}
PlaceElem::Deref
| PlaceElem::ConstantIndex { .. }
| PlaceElem::Subslice { .. }
Expand Down Expand Up @@ -1213,9 +1208,7 @@ macro_rules! visit_place_fns {
location: Location,
) {
match elem {
ProjectionElem::OpaqueCast(ty)
| ProjectionElem::Subtype(ty)
| ProjectionElem::Field(_, ty) => {
ProjectionElem::OpaqueCast(ty) | ProjectionElem::Field(_, ty) => {
self.visit_ty(ty, TyContext::Location(location));
}
ProjectionElem::Index(local) => {
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_mir_build/src/build/expr/as_place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ fn convert_to_hir_projections_and_truncate_for_capture(
variant = Some(*idx);
continue;
}
// These do not affect anything, they just make sure we know the right type.
ProjectionElem::OpaqueCast(_) | ProjectionElem::Subtype(..) => continue,
// This does not affect anything, it just makes sure we know the right type.
ProjectionElem::OpaqueCast(_) => continue,
ProjectionElem::Index(..)
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subslice { .. } => {
Expand Down Expand Up @@ -712,7 +712,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
ProjectionElem::Field(..)
| ProjectionElem::Downcast(..)
| ProjectionElem::OpaqueCast(..)
| ProjectionElem::Subtype(..)
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subslice { .. } => (),
}
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_mir_dataflow/src/move_paths/abs_domain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ impl<'tcx> Lift for PlaceElem<'tcx> {
ProjectionElem::ConstantIndex { offset, min_length, from_end }
}
ProjectionElem::Downcast(a, u) => ProjectionElem::Downcast(a, u),
ProjectionElem::Subtype(ty) => ProjectionElem::Subtype(ty.lift()),
}
}
}
5 changes: 1 addition & 4 deletions compiler/rustc_mir_dataflow/src/move_paths/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,8 @@ impl<'a, 'tcx, F: Fn(Ty<'tcx>) -> bool> MoveDataBuilder<'a, 'tcx, F> {
},
// `OpaqueCast`:Only transmutes the type, so no moves there.
// `Downcast` :Only changes information about a `Place` without moving.
// `Subtype` :Only transmutes the type, so moves.
// So it's safe to skip these.
ProjectionElem::OpaqueCast(_)
| ProjectionElem::Subtype(_)
| ProjectionElem::Downcast(_, _) => (),
ProjectionElem::OpaqueCast(_) | ProjectionElem::Downcast(_, _) => (),
}
let elem_ty = PlaceTy::from_ty(place_ty).projection_ty(tcx, elem).ty;
if !(self.filter)(elem_ty) {
Expand Down
Loading
Loading