From 810f309ff30fe7a75917f9e5359074dc991b4590 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 30 May 2020 23:46:21 +0200 Subject: [PATCH 01/12] MIR sanity check: validate types on assignment --- src/librustc_mir/transform/validate.rs | 54 ++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 625f40cd79206..3d48a2387a88e 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -7,7 +7,7 @@ use rustc_middle::{ BasicBlock, Body, Location, Operand, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, }, - ty::{self, ParamEnv, TyCtxt}, + ty::{self, fold::BottomUpFolder, ParamEnv, Ty, TyCtxt, TypeFoldable}, }; #[derive(Copy, Clone, Debug)] @@ -83,6 +83,40 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { } } +/// Check if src can be assigned into dest. +/// This is not precise, it will accept some incorrect assignments. +fn mir_assign_valid_types<'tcx>(tcx: TyCtxt<'tcx>, src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { + if src == dest { + // Equal types, all is good. + return true; + } + + // Type-changing assignments can happen for (at least) two reasons: + // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. + // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types + // with their late-bound lifetimes are still around and can lead to type differences. + // Normalize both of them away. + // FIXME: Share this code with `interpret/eval_context.rs`. + let normalize = |ty: Ty<'tcx>| { + ty.fold_with(&mut BottomUpFolder { + tcx, + // Normalize all references to immutable. + ty_op: |ty| match ty.kind { + ty::Ref(_, pointee, _) => tcx.mk_imm_ref(tcx.lifetimes.re_erased, pointee), + _ => ty, + }, + // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): + // lifetimes in invariant positions could matter (e.g. through associated types). + // But that just means we miss some potential incompatible types, it will not + // lead to wrong errors. + lt_op: |_| tcx.lifetimes.re_erased, + // Leave consts unchanged. + ct_op: |ct| ct, + }) + }; + normalize(src) == normalize(dest) +} + impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) { // `Operand::Copy` is only supposed to be used with `Copy` types. @@ -99,9 +133,23 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { - // The sides of an assignment must not alias. Currently this just checks whether the places - // are identical. if let StatementKind::Assign(box (dest, rvalue)) = &statement.kind { + // LHS and RHS of the assignment must have the same type. + let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; + let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); + if !mir_assign_valid_types(self.tcx, right_ty, left_ty) { + self.fail( + location, + format!( + "encountered `Assign` statement with incompatible types:\n\ + left-hand side has type: {}\n\ + right-hand side has type: {}", + left_ty, right_ty, + ), + ); + } + // The sides of an assignment must not alias. Currently this just checks whether the places + // are identical. match rvalue { Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { if dest == src { From 50d7deac4de3bfde44a634ff4dabf3115f694c79 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 May 2020 09:54:25 +0200 Subject: [PATCH 02/12] prepare visit_statement for checking more kinds of statements --- src/librustc_mir/transform/validate.rs | 53 ++++++++++++++------------ 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 3d48a2387a88e..051ce9e6b1ef8 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -133,34 +133,37 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { - if let StatementKind::Assign(box (dest, rvalue)) = &statement.kind { - // LHS and RHS of the assignment must have the same type. - let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; - let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); - if !mir_assign_valid_types(self.tcx, right_ty, left_ty) { - self.fail( - location, - format!( - "encountered `Assign` statement with incompatible types:\n\ - left-hand side has type: {}\n\ - right-hand side has type: {}", - left_ty, right_ty, - ), - ); - } - // The sides of an assignment must not alias. Currently this just checks whether the places - // are identical. - match rvalue { - Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { - if dest == src { - self.fail( - location, - "encountered `Assign` statement with overlapping memory", - ); + match &statement.kind { + StatementKind::Assign(box (dest, rvalue)) => { + // LHS and RHS of the assignment must have the same type. + let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; + let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); + if !mir_assign_valid_types(self.tcx, right_ty, left_ty) { + self.fail( + location, + format!( + "encountered `Assign` statement with incompatible types:\n\ + left-hand side has type: {}\n\ + right-hand side has type: {}", + left_ty, right_ty, + ), + ); + } + // The sides of an assignment must not alias. Currently this just checks whether the places + // are identical. + match rvalue { + Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) => { + if dest == src { + self.fail( + location, + "encountered `Assign` statement with overlapping memory", + ); + } } + _ => {} } - _ => {} } + _ => {} } } From 93e3552d040edfa67cdedfe2fe4a44fe0c4ead59 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 May 2020 15:02:33 +0200 Subject: [PATCH 03/12] also normalize constants when comparing types --- src/librustc_mir/interpret/eval_context.rs | 1 + src/librustc_mir/transform/validate.rs | 68 +++++++++++----------- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 22f4691c22b3d..1a6ed41ba47e5 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -239,6 +239,7 @@ pub(super) fn mir_assign_valid_types<'tcx>( // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types // with their late-bound lifetimes are still around and can lead to type differences. // Normalize both of them away. + // Also see the related but slightly different pre-monomorphization method in `transform/validate.rs`. let normalize = |ty: Ty<'tcx>| { ty.fold_with(&mut BottomUpFolder { tcx, diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 051ce9e6b1ef8..14c67c2372c9f 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -81,40 +81,42 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { self.fail(location, format!("encountered jump to invalid basic block {:?}", bb)) } } -} -/// Check if src can be assigned into dest. -/// This is not precise, it will accept some incorrect assignments. -fn mir_assign_valid_types<'tcx>(tcx: TyCtxt<'tcx>, src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { - if src == dest { - // Equal types, all is good. - return true; - } + /// Check if src can be assigned into dest. + /// This is not precise, it will accept some incorrect assignments. + fn mir_assign_valid_types(&self, src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { + if src == dest { + // Equal types, all is good. + return true; + } - // Type-changing assignments can happen for (at least) two reasons: - // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. - // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types - // with their late-bound lifetimes are still around and can lead to type differences. - // Normalize both of them away. - // FIXME: Share this code with `interpret/eval_context.rs`. - let normalize = |ty: Ty<'tcx>| { - ty.fold_with(&mut BottomUpFolder { - tcx, - // Normalize all references to immutable. - ty_op: |ty| match ty.kind { - ty::Ref(_, pointee, _) => tcx.mk_imm_ref(tcx.lifetimes.re_erased, pointee), - _ => ty, - }, - // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): - // lifetimes in invariant positions could matter (e.g. through associated types). - // But that just means we miss some potential incompatible types, it will not - // lead to wrong errors. - lt_op: |_| tcx.lifetimes.re_erased, - // Leave consts unchanged. - ct_op: |ct| ct, - }) - }; - normalize(src) == normalize(dest) + // Type-changing assignments can happen for (at least) two reasons: + // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. + // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types + // with their late-bound lifetimes are still around and can lead to type differences. + // Normalize both of them away. + // Also see the related but slightly different post-monomorphization method in `interpret/eval_context.rs`. + let normalize = |ty: Ty<'tcx>| { + ty.fold_with(&mut BottomUpFolder { + tcx: self.tcx, + // Normalize all references to immutable. + ty_op: |ty| match ty.kind { + ty::Ref(_, pointee, _) => { + self.tcx.mk_imm_ref(self.tcx.lifetimes.re_erased, pointee) + } + _ => ty, + }, + // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): + // lifetimes in invariant positions could matter (e.g. through associated types). + // But that just means we miss some potential incompatible types, it will not + // lead to wrong errors. + lt_op: |_| self.tcx.lifetimes.re_erased, + // Evaluate consts. + ct_op: |ct| ct.eval(self.tcx, self.param_env), + }) + }; + normalize(src) == normalize(dest) + } } impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { @@ -138,7 +140,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { // LHS and RHS of the assignment must have the same type. let left_ty = dest.ty(&self.body.local_decls, self.tcx).ty; let right_ty = rvalue.ty(&self.body.local_decls, self.tcx); - if !mir_assign_valid_types(self.tcx, right_ty, left_ty) { + if !self.mir_assign_valid_types(right_ty, left_ty) { self.fail( location, format!( From 9576e307a7b8ac0c812fac927d247761662e7d1a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 1 Jun 2020 21:04:11 +0200 Subject: [PATCH 04/12] also normalize_erasing_regions --- src/librustc_mir/transform/validate.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 14c67c2372c9f..40189ca5c128c 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -89,6 +89,13 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // Equal types, all is good. return true; } + // Normalize projections and things like that. + let src = self.tcx.normalize_erasing_regions(self.param_env, src); + let dest = self.tcx.normalize_erasing_regions(self.param_env, dest); + // It's worth checking equality again. + if src == dest { + return true; + } // Type-changing assignments can happen for (at least) two reasons: // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. From 8200771aa2cbd393a5beca819ac2462cf35e8d15 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 6 Jun 2020 11:45:19 +0200 Subject: [PATCH 05/12] reveal_all when sanity-checking MIR assignment types --- src/librustc_mir/transform/validate.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 40189ca5c128c..b2fbb48eefea5 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -90,8 +90,11 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { return true; } // Normalize projections and things like that. - let src = self.tcx.normalize_erasing_regions(self.param_env, src); - let dest = self.tcx.normalize_erasing_regions(self.param_env, dest); + // FIXME: We need to reveal_all, as some optimizations change types in ways + // that requires unfolding opaque types. + let param_env = self.param_env.with_reveal_all(); + let src = self.tcx.normalize_erasing_regions(param_env, src); + let dest = self.tcx.normalize_erasing_regions(param_env, dest); // It's worth checking equality again. if src == dest { return true; @@ -119,7 +122,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // lead to wrong errors. lt_op: |_| self.tcx.lifetimes.re_erased, // Evaluate consts. - ct_op: |ct| ct.eval(self.tcx, self.param_env), + ct_op: |ct| ct.eval(self.tcx, param_env), }) }; normalize(src) == normalize(dest) From 978470f711b3be3350a46d386a424e1dfb1ea148 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 21 Jun 2020 10:04:12 +0200 Subject: [PATCH 06/12] no need to normalize mutability any more --- src/librustc_mir/transform/validate.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index b2fbb48eefea5..81bdcc849e49c 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -100,22 +100,16 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { return true; } - // Type-changing assignments can happen for (at least) two reasons: - // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. - // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types - // with their late-bound lifetimes are still around and can lead to type differences. - // Normalize both of them away. - // Also see the related but slightly different post-monomorphization method in `interpret/eval_context.rs`. + // Type-changing assignments can happen when subtyping is used. While + // all normal lifetimes are erased, higher-ranked types with their + // late-bound lifetimes are still around and can lead to type + // differences. Normalize both of them away. + // Also see the related but slightly different post-monomorphization + // method in `interpret/eval_context.rs`. let normalize = |ty: Ty<'tcx>| { ty.fold_with(&mut BottomUpFolder { tcx: self.tcx, - // Normalize all references to immutable. - ty_op: |ty| match ty.kind { - ty::Ref(_, pointee, _) => { - self.tcx.mk_imm_ref(self.tcx.lifetimes.re_erased, pointee) - } - _ => ty, - }, + ty_op: |ty| ty, // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): // lifetimes in invariant positions could matter (e.g. through associated types). // But that just means we miss some potential incompatible types, it will not From 91f73fbca488973169b4f4b927323f712ad3d776 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 21 Jun 2020 13:49:53 +0200 Subject: [PATCH 07/12] use a TypeRelation to compare the types --- src/librustc_mir/transform/validate.rs | 98 +++++++++++++++++++++----- 1 file changed, 80 insertions(+), 18 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 81bdcc849e49c..d0293131b263a 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -7,7 +7,11 @@ use rustc_middle::{ BasicBlock, Body, Location, Operand, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, }, - ty::{self, fold::BottomUpFolder, ParamEnv, Ty, TyCtxt, TypeFoldable}, + ty::{ + self, + relate::{Relate, RelateResult, TypeRelation}, + ParamEnv, Ty, TyCtxt, + }, }; #[derive(Copy, Clone, Debug)] @@ -103,23 +107,81 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // Type-changing assignments can happen when subtyping is used. While // all normal lifetimes are erased, higher-ranked types with their // late-bound lifetimes are still around and can lead to type - // differences. Normalize both of them away. - // Also see the related but slightly different post-monomorphization - // method in `interpret/eval_context.rs`. - let normalize = |ty: Ty<'tcx>| { - ty.fold_with(&mut BottomUpFolder { - tcx: self.tcx, - ty_op: |ty| ty, - // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): - // lifetimes in invariant positions could matter (e.g. through associated types). - // But that just means we miss some potential incompatible types, it will not - // lead to wrong errors. - lt_op: |_| self.tcx.lifetimes.re_erased, - // Evaluate consts. - ct_op: |ct| ct.eval(self.tcx, param_env), - }) - }; - normalize(src) == normalize(dest) + // differences. So we compare ignoring lifetimes. + struct LifetimeIgnoreRelation<'tcx> { + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + } + + impl TypeRelation<'tcx> for LifetimeIgnoreRelation<'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn param_env(&self) -> ty::ParamEnv<'tcx> { + self.param_env + } + + fn tag(&self) -> &'static str { + "librustc_mir::transform::validate" + } + + fn a_is_expected(&self) -> bool { + true + } + + fn relate_with_variance>( + &mut self, + _: ty::Variance, + a: &T, + b: &T, + ) -> RelateResult<'tcx, T> { + // Ignore variance, require types to be exactly the same. + self.relate(a, b) + } + + fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> { + if a == b { + // Short-circuit. + return Ok(a); + } + ty::relate::super_relate_tys(self, a, b) + } + + fn regions( + &mut self, + a: ty::Region<'tcx>, + _b: ty::Region<'tcx>, + ) -> RelateResult<'tcx, ty::Region<'tcx>> { + // Ignore regions. + Ok(a) + } + + fn consts( + &mut self, + a: &'tcx ty::Const<'tcx>, + b: &'tcx ty::Const<'tcx>, + ) -> RelateResult<'tcx, &'tcx ty::Const<'tcx>> { + ty::relate::super_relate_consts(self, a, b) + } + + fn binders( + &mut self, + a: &ty::Binder, + b: &ty::Binder, + ) -> RelateResult<'tcx, ty::Binder> + where + T: Relate<'tcx>, + { + self.relate(a.skip_binder(), b.skip_binder())?; + Ok(a.clone()) + } + } + + // Instantiate and run relation. + let mut relator: LifetimeIgnoreRelation<'tcx> = + LifetimeIgnoreRelation { tcx: self.tcx, param_env }; + relator.relate(&src, &dest).is_ok() } } From 7754322bcc2852814592c47c3b76c53fefe95a4f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Jun 2020 09:00:40 +0200 Subject: [PATCH 08/12] fix typo Co-authored-by: Bastian Kauschke --- src/librustc_mir/transform/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index d0293131b263a..803954d258fa0 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -95,7 +95,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { } // Normalize projections and things like that. // FIXME: We need to reveal_all, as some optimizations change types in ways - // that requires unfolding opaque types. + // that require unfolding opaque types. let param_env = self.param_env.with_reveal_all(); let src = self.tcx.normalize_erasing_regions(param_env, src); let dest = self.tcx.normalize_erasing_regions(param_env, dest); From 7f8fe6a9851aaf493c8657fe7e98145539d466dd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Jun 2020 09:17:33 +0200 Subject: [PATCH 09/12] also use relator in interpreter assignment sanity check --- src/librustc_mir/interpret/eval_context.rs | 41 ++---- src/librustc_mir/interpret/operand.rs | 4 +- src/librustc_mir/interpret/place.rs | 5 +- src/librustc_mir/transform/validate.rs | 162 +++++++++++---------- 4 files changed, 109 insertions(+), 103 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 1a6ed41ba47e5..b673738cec580 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -15,7 +15,7 @@ use rustc_middle::mir::interpret::{ }; use rustc_middle::ty::layout::{self, TyAndLayout}; use rustc_middle::ty::{ - self, fold::BottomUpFolder, query::TyCtxtAt, subst::SubstsRef, Ty, TyCtxt, TypeFoldable, + self, query::TyCtxtAt, subst::SubstsRef, ParamEnv, Ty, TyCtxt, TypeFoldable, }; use rustc_span::{source_map::DUMMY_SP, Span}; use rustc_target::abi::{Align, HasDataLayout, LayoutOf, Size, TargetDataLayout}; @@ -24,6 +24,7 @@ use super::{ Immediate, MPlaceTy, Machine, MemPlace, MemPlaceMeta, Memory, OpTy, Operand, Place, PlaceTy, ScalarMaybeUninit, StackPopJump, }; +use crate::transform::validate::equal_up_to_regions; use crate::util::storage::AlwaysLiveLocals; pub struct InterpCx<'mir, 'tcx, M: Machine<'mir, 'tcx>> { @@ -220,6 +221,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> LayoutOf for InterpCx<'mir, 'tcx, /// This test should be symmetric, as it is primarily about layout compatibility. pub(super) fn mir_assign_valid_types<'tcx>( tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, src: TyAndLayout<'tcx>, dest: TyAndLayout<'tcx>, ) -> bool { @@ -234,29 +236,15 @@ pub(super) fn mir_assign_valid_types<'tcx>( return false; } - // Type-changing assignments can happen for (at least) two reasons: - // 1. `&mut T` -> `&T` gets optimized from a reborrow to a mere assignment. - // 2. Subtyping is used. While all normal lifetimes are erased, higher-ranked types - // with their late-bound lifetimes are still around and can lead to type differences. - // Normalize both of them away. - // Also see the related but slightly different pre-monomorphization method in `transform/validate.rs`. - let normalize = |ty: Ty<'tcx>| { - ty.fold_with(&mut BottomUpFolder { - tcx, - // Normalize all references to immutable. - ty_op: |ty| match ty.kind { - ty::Ref(_, pointee, _) => tcx.mk_imm_ref(tcx.lifetimes.re_erased, pointee), - _ => ty, - }, - // We just erase all late-bound lifetimes, but this is not fully correct (FIXME): - // lifetimes in invariant positions could matter (e.g. through associated types). - // We rely on the fact that layout was confirmed to be equal above. - lt_op: |_| tcx.lifetimes.re_erased, - // Leave consts unchanged. - ct_op: |ct| ct, - }) - }; - normalize(src.ty) == normalize(dest.ty) + // Type-changing assignments can happen when subtyping is used. While + // all normal lifetimes are erased, higher-ranked types with their + // late-bound lifetimes are still around and can lead to type + // differences. So we compare ignoring lifetimes. + // + // Note that this is not fully correct (FIXME): + // lifetimes in invariant positions could matter (e.g. through associated types). + // We rely on the fact that layout was confirmed to be equal above. + equal_up_to_regions(tcx, param_env, src.ty, dest.ty) } /// Use the already known layout if given (but sanity check in debug mode), @@ -264,6 +252,7 @@ pub(super) fn mir_assign_valid_types<'tcx>( #[cfg_attr(not(debug_assertions), inline(always))] pub(super) fn from_known_layout<'tcx>( tcx: TyCtxtAt<'tcx>, + param_env: ParamEnv<'tcx>, known_layout: Option>, compute: impl FnOnce() -> InterpResult<'tcx, TyAndLayout<'tcx>>, ) -> InterpResult<'tcx, TyAndLayout<'tcx>> { @@ -272,7 +261,7 @@ pub(super) fn from_known_layout<'tcx>( Some(known_layout) => { if cfg!(debug_assertions) { let check_layout = compute()?; - if !mir_assign_valid_types(tcx.tcx, check_layout, known_layout) { + if !mir_assign_valid_types(tcx.tcx, param_env, check_layout, known_layout) { span_bug!( tcx.span, "expected type differs from actual type.\nexpected: {:?}\nactual: {:?}", @@ -476,7 +465,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // have to support that case (mostly by skipping all caching). match frame.locals.get(local).and_then(|state| state.layout.get()) { None => { - let layout = from_known_layout(self.tcx, layout, || { + let layout = from_known_layout(self.tcx, self.param_env, layout, || { let local_ty = frame.body.local_decls[local].ty; let local_ty = self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty); diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 35e433c4bd5cd..27fa9b2c17c57 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -472,6 +472,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // Sanity-check the type we ended up with. debug_assert!(mir_assign_valid_types( *self.tcx, + self.param_env, self.layout_of(self.subst_from_current_frame_and_normalize_erasing_regions( place.ty(&self.frame().body.local_decls, *self.tcx).ty ))?, @@ -554,7 +555,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // documentation). let val_val = M::adjust_global_const(self, val_val)?; // Other cases need layout. - let layout = from_known_layout(self.tcx, layout, || self.layout_of(val.ty))?; + let layout = + from_known_layout(self.tcx, self.param_env, layout, || self.layout_of(val.ty))?; let op = match val_val { ConstValue::ByRef { alloc, offset } => { let id = self.tcx.create_memory_alloc(alloc); diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 396aec0a8f89f..98a1cea97e220 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -652,6 +652,7 @@ where // Sanity-check the type we ended up with. debug_assert!(mir_assign_valid_types( *self.tcx, + self.param_env, self.layout_of(self.subst_from_current_frame_and_normalize_erasing_regions( place.ty(&self.frame().body.local_decls, *self.tcx).ty ))?, @@ -855,7 +856,7 @@ where ) -> InterpResult<'tcx> { // We do NOT compare the types for equality, because well-typed code can // actually "transmute" `&mut T` to `&T` in an assignment without a cast. - if !mir_assign_valid_types(*self.tcx, src.layout, dest.layout) { + if !mir_assign_valid_types(*self.tcx, self.param_env, src.layout, dest.layout) { span_bug!( self.cur_span(), "type mismatch when copying!\nsrc: {:?},\ndest: {:?}", @@ -912,7 +913,7 @@ where src: OpTy<'tcx, M::PointerTag>, dest: PlaceTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { - if mir_assign_valid_types(*self.tcx, src.layout, dest.layout) { + if mir_assign_valid_types(*self.tcx, self.param_env, src.layout, dest.layout) { // Fast path: Just use normal `copy_op` return self.copy_op(src, dest); } diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 803954d258fa0..5f0edd64c47e1 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -32,6 +32,93 @@ impl<'tcx> MirPass<'tcx> for Validator { } } +/// Returns whether the two types are equal up to lifetimes. +/// All lifetimes, including higher-ranked ones, get ignored for this comparison. +/// (This is unlike the `erasing_regions` methods, which keep higher-ranked lifetimes for soundness reasons.) +/// +/// The point of this function is to approximate "equal up to subtyping". However, +/// the approximation is incorrect as variance is ignored. +pub fn equal_up_to_regions( + tcx: TyCtxt<'tcx>, + param_env: ParamEnv<'tcx>, + src: Ty<'tcx>, + dest: Ty<'tcx>, +) -> bool { + struct LifetimeIgnoreRelation<'tcx> { + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + } + + impl TypeRelation<'tcx> for LifetimeIgnoreRelation<'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn param_env(&self) -> ty::ParamEnv<'tcx> { + self.param_env + } + + fn tag(&self) -> &'static str { + "librustc_mir::transform::validate" + } + + fn a_is_expected(&self) -> bool { + true + } + + fn relate_with_variance>( + &mut self, + _: ty::Variance, + a: &T, + b: &T, + ) -> RelateResult<'tcx, T> { + // Ignore variance, require types to be exactly the same. + self.relate(a, b) + } + + fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> { + if a == b { + // Short-circuit. + return Ok(a); + } + ty::relate::super_relate_tys(self, a, b) + } + + fn regions( + &mut self, + a: ty::Region<'tcx>, + _b: ty::Region<'tcx>, + ) -> RelateResult<'tcx, ty::Region<'tcx>> { + // Ignore regions. + Ok(a) + } + + fn consts( + &mut self, + a: &'tcx ty::Const<'tcx>, + b: &'tcx ty::Const<'tcx>, + ) -> RelateResult<'tcx, &'tcx ty::Const<'tcx>> { + ty::relate::super_relate_consts(self, a, b) + } + + fn binders( + &mut self, + a: &ty::Binder, + b: &ty::Binder, + ) -> RelateResult<'tcx, ty::Binder> + where + T: Relate<'tcx>, + { + self.relate(a.skip_binder(), b.skip_binder())?; + Ok(a.clone()) + } + } + + // Instantiate and run relation. + let mut relator: LifetimeIgnoreRelation<'tcx> = LifetimeIgnoreRelation { tcx: tcx, param_env }; + relator.relate(&src, &dest).is_ok() +} + struct TypeChecker<'a, 'tcx> { when: &'a str, source: MirSource<'tcx>, @@ -108,80 +195,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // all normal lifetimes are erased, higher-ranked types with their // late-bound lifetimes are still around and can lead to type // differences. So we compare ignoring lifetimes. - struct LifetimeIgnoreRelation<'tcx> { - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - } - - impl TypeRelation<'tcx> for LifetimeIgnoreRelation<'tcx> { - fn tcx(&self) -> TyCtxt<'tcx> { - self.tcx - } - - fn param_env(&self) -> ty::ParamEnv<'tcx> { - self.param_env - } - - fn tag(&self) -> &'static str { - "librustc_mir::transform::validate" - } - - fn a_is_expected(&self) -> bool { - true - } - - fn relate_with_variance>( - &mut self, - _: ty::Variance, - a: &T, - b: &T, - ) -> RelateResult<'tcx, T> { - // Ignore variance, require types to be exactly the same. - self.relate(a, b) - } - - fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> { - if a == b { - // Short-circuit. - return Ok(a); - } - ty::relate::super_relate_tys(self, a, b) - } - - fn regions( - &mut self, - a: ty::Region<'tcx>, - _b: ty::Region<'tcx>, - ) -> RelateResult<'tcx, ty::Region<'tcx>> { - // Ignore regions. - Ok(a) - } - - fn consts( - &mut self, - a: &'tcx ty::Const<'tcx>, - b: &'tcx ty::Const<'tcx>, - ) -> RelateResult<'tcx, &'tcx ty::Const<'tcx>> { - ty::relate::super_relate_consts(self, a, b) - } - - fn binders( - &mut self, - a: &ty::Binder, - b: &ty::Binder, - ) -> RelateResult<'tcx, ty::Binder> - where - T: Relate<'tcx>, - { - self.relate(a.skip_binder(), b.skip_binder())?; - Ok(a.clone()) - } - } - - // Instantiate and run relation. - let mut relator: LifetimeIgnoreRelation<'tcx> = - LifetimeIgnoreRelation { tcx: self.tcx, param_env }; - relator.relate(&src, &dest).is_ok() + equal_up_to_regions(self.tcx, param_env, src, dest) } } From 5e5ae8b0875ce70b1a729bef442e753bb3d2502f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Jun 2020 10:26:29 +0200 Subject: [PATCH 10/12] expand a comment --- src/librustc_mir/interpret/eval_context.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index b673738cec580..56a9355650e22 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -226,7 +226,9 @@ pub(super) fn mir_assign_valid_types<'tcx>( dest: TyAndLayout<'tcx>, ) -> bool { if src.ty == dest.ty { - // Equal types, all is good. + // Equal types, all is good. Layout will also be equal. + // (Enum variants would be an exception here as they have the type of the enum but different layout. + // However, those are never the type of an assignment.) return true; } if src.layout != dest.layout { From a593728fc753f85df575b54aadc64c3fd2c06d11 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 22 Jun 2020 11:07:39 +0200 Subject: [PATCH 11/12] make layout check a mere sanity check --- src/librustc_mir/interpret/eval_context.rs | 25 ++++++---------------- src/librustc_mir/transform/validate.rs | 10 +++++---- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 56a9355650e22..25860c43add41 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -225,28 +225,17 @@ pub(super) fn mir_assign_valid_types<'tcx>( src: TyAndLayout<'tcx>, dest: TyAndLayout<'tcx>, ) -> bool { - if src.ty == dest.ty { - // Equal types, all is good. Layout will also be equal. - // (Enum variants would be an exception here as they have the type of the enum but different layout. - // However, those are never the type of an assignment.) - return true; - } - if src.layout != dest.layout { - // Layout differs, definitely not equal. - // We do this here because Miri would *do the wrong thing* if we allowed layout-changing - // assignments. - return false; - } - // Type-changing assignments can happen when subtyping is used. While // all normal lifetimes are erased, higher-ranked types with their // late-bound lifetimes are still around and can lead to type // differences. So we compare ignoring lifetimes. - // - // Note that this is not fully correct (FIXME): - // lifetimes in invariant positions could matter (e.g. through associated types). - // We rely on the fact that layout was confirmed to be equal above. - equal_up_to_regions(tcx, param_env, src.ty, dest.ty) + if equal_up_to_regions(tcx, param_env, src.ty, dest.ty) { + // Make sure the layout is equal, too -- just to be safe. Miri really needs layout equality. + assert_eq!(src.layout, dest.layout); + true + } else { + false + } } /// Use the already known layout if given (but sanity check in debug mode), diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 5f0edd64c47e1..a293751d5b276 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -44,6 +44,11 @@ pub fn equal_up_to_regions( src: Ty<'tcx>, dest: Ty<'tcx>, ) -> bool { + // Fast path. + if src == dest { + return true; + } + struct LifetimeIgnoreRelation<'tcx> { tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, @@ -176,6 +181,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { /// Check if src can be assigned into dest. /// This is not precise, it will accept some incorrect assignments. fn mir_assign_valid_types(&self, src: Ty<'tcx>, dest: Ty<'tcx>) -> bool { + // Fast path before we normalize. if src == dest { // Equal types, all is good. return true; @@ -186,10 +192,6 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { let param_env = self.param_env.with_reveal_all(); let src = self.tcx.normalize_erasing_regions(param_env, src); let dest = self.tcx.normalize_erasing_regions(param_env, dest); - // It's worth checking equality again. - if src == dest { - return true; - } // Type-changing assignments can happen when subtyping is used. While // all normal lifetimes are erased, higher-ranked types with their From 35911eeb93bae9585b6881bb3d6620df3e6a38ff Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 24 Jun 2020 09:03:01 +0200 Subject: [PATCH 12/12] reduce sanity check in debug mode --- src/librustc_mir/interpret/eval_context.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 25860c43add41..ec792d5b22414 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -230,8 +230,14 @@ pub(super) fn mir_assign_valid_types<'tcx>( // late-bound lifetimes are still around and can lead to type // differences. So we compare ignoring lifetimes. if equal_up_to_regions(tcx, param_env, src.ty, dest.ty) { - // Make sure the layout is equal, too -- just to be safe. Miri really needs layout equality. - assert_eq!(src.layout, dest.layout); + // 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 + // enum downcast is involved (as enum variants carry the type of the + // enum), but those should never occur in assignments. + if cfg!(debug_assertions) || src.ty != dest.ty { + assert_eq!(src.layout, dest.layout); + } true } else { false