From 8f171c49ceaf16b1a81125ad9788c9dae70d2111 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 11 Jul 2019 13:27:41 +0200 Subject: [PATCH 1/3] Replace `struct_tail` and `struct_lockstep_tails` with variants handling normalization. The old struct tail functions did not deal with `::A` and `impl Trait`, at least not explicitly. (We didn't notice this bug before because it is only exposed when the tail (post deep normalization) is not `Sized`, so it was a rare case to deal with.) For post type-checking (i.e. during codegen), there is now `struct_tail_erasing_lifetimes` and `struct_lockstep_tails_erasing_lifetimes`, which each take an additional `ParamEnv` argument to drive normalization. For pre type-checking cases where normalization is not needed, there is `struct_tail_without_normalization`. (Currently, the only instance of this is `Expectation::rvalue_hint`.) All of these new entrypoints work by calling out to common helper routines. The helpers are parameterized over a closure that handles the normalization. --- src/librustc/ty/layout.rs | 6 +- src/librustc/ty/util.rs | 97 ++++++++++++++++++++-- src/librustc_codegen_ssa/base.rs | 3 +- src/librustc_codegen_ssa/traits/type_.rs | 5 +- src/librustc_mir/interpret/cast.rs | 3 +- src/librustc_mir/interpret/intern.rs | 4 +- src/librustc_mir/interpret/validity.rs | 3 +- src/librustc_mir/monomorphize/collector.rs | 7 +- src/librustc_typeck/check/mod.rs | 2 +- src/librustc_typeck/check/wfcheck.rs | 3 +- 10 files changed, 111 insertions(+), 22 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 4af26e19b37..4ed52a1e966 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -543,7 +543,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { return Ok(tcx.intern_layout(LayoutDetails::scalar(self, data_ptr))); } - let unsized_part = tcx.struct_tail(pointee); + let unsized_part = tcx.struct_tail_erasing_lifetimes(pointee, param_env); let metadata = match unsized_part.sty { ty::Foreign(..) => { return Ok(tcx.intern_layout(LayoutDetails::scalar(self, data_ptr))); @@ -1664,7 +1664,7 @@ impl<'tcx> SizeSkeleton<'tcx> { ty::Ref(_, pointee, _) | ty::RawPtr(ty::TypeAndMut { ty: pointee, .. }) => { let non_zero = !ty.is_unsafe_ptr(); - let tail = tcx.struct_tail(pointee); + let tail = tcx.struct_tail_erasing_lifetimes(pointee, param_env); match tail.sty { ty::Param(_) | ty::Projection(_) => { debug_assert!(tail.has_param_types() || tail.has_self_ty()); @@ -2015,7 +2015,7 @@ where })); } - match tcx.struct_tail(pointee).sty { + match tcx.struct_tail_erasing_lifetimes(pointee, cx.param_env()).sty { ty::Slice(_) | ty::Str => tcx.types.usize, ty::Dynamic(_, _) => { diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index a3b99f143d0..871aeb78af5 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -258,10 +258,42 @@ impl<'tcx> TyCtxt<'tcx> { false } - /// Returns the deeply last field of nested structures, or the same type, - /// if not a structure at all. Corresponds to the only possible unsized - /// field, and its type can be used to determine unsizing strategy. - pub fn struct_tail(self, mut ty: Ty<'tcx>) -> Ty<'tcx> { + /// Attempts to returns the deeply last field of nested structures, but + /// does not apply any normalization in its search. Returns the same type + /// if input `ty` is not a structure at all. + pub fn struct_tail_without_normalization(self, ty: Ty<'tcx>) -> Ty<'tcx> + { + let tcx = self; + tcx.struct_tail_with_normalize(ty, |ty| ty) + } + + /// Returns the deeply last field of nested structures, or the same type if + /// not a structure at all. Corresponds to the only possible unsized field, + /// and its type can be used to determine unsizing strategy. + pub fn struct_tail_erasing_lifetimes(self, + ty: Ty<'tcx>, + param_env: ty::ParamEnv<'tcx>) + -> Ty<'tcx> + { + let tcx = self; + tcx.struct_tail_with_normalize(ty, |ty| tcx.normalize_erasing_regions(param_env, ty)) + } + + /// Returns the deeply last field of nested structures, or the same type if + /// not a structure at all. Corresponds to the only possible unsized field, + /// and its type can be used to determine unsizing strategy. + /// + /// This is parameterized over the normalization strategy (i.e. how to + /// handle `::Assoc` and `impl Trait`); pass the identity + /// function to indicate no normalization should take place. + /// + /// See also `struct_tail_erasing_lifetimes`, which is what callers running + /// after type checking should use. + pub fn struct_tail_with_normalize(self, + mut ty: Ty<'tcx>, + normalize: impl Fn(Ty<'tcx>) -> Ty<'tcx>) + -> Ty<'tcx> + { loop { match ty.sty { ty::Adt(def, substs) => { @@ -282,6 +314,15 @@ impl<'tcx> TyCtxt<'tcx> { } } + ty::Projection(_) | ty::Opaque(..) => { + let normalized = normalize(ty); + if ty == normalized { + return ty; + } else { + ty = normalized; + } + } + _ => { break; } @@ -295,10 +336,34 @@ impl<'tcx> TyCtxt<'tcx> { /// structure definitions. /// For `(Foo>, Foo)`, the result will be `(Foo, Trait)`, /// whereas struct_tail produces `T`, and `Trait`, respectively. - pub fn struct_lockstep_tails(self, - source: Ty<'tcx>, - target: Ty<'tcx>) - -> (Ty<'tcx>, Ty<'tcx>) { + /// + /// Must only be called after type-checking is complete; otherwise + /// normalization attempt may cause compiler bugs. + pub fn struct_lockstep_tails_erasing_lifetimes(self, + source: Ty<'tcx>, + target: Ty<'tcx>, + param_env: ty::ParamEnv<'tcx>) + -> (Ty<'tcx>, Ty<'tcx>) + { + let tcx = self; + tcx.struct_lockstep_tails_with_normalize( + source, target, |ty| tcx.normalize_erasing_regions(param_env, ty)) + } + + /// Same as applying struct_tail on `source` and `target`, but only + /// keeps going as long as the two types are instances of the same + /// structure definitions. + /// For `(Foo>, Foo)`, the result will be `(Foo, Trait)`, + /// whereas struct_tail produces `T`, and `Trait`, respectively. + /// + /// See also struct_lockstep_tails_erasing_lifetimes, which + /// is what callers running after type checking should use. + pub fn struct_lockstep_tails_with_normalize(self, + source: Ty<'tcx>, + target: Ty<'tcx>, + normalize: impl Fn(Ty<'tcx>) -> Ty<'tcx>) + -> (Ty<'tcx>, Ty<'tcx>) + { let (mut a, mut b) = (source, target); loop { match (&a.sty, &b.sty) { @@ -320,6 +385,22 @@ impl<'tcx> TyCtxt<'tcx> { break; } }, + (ty::Projection(_), _) | (ty::Opaque(..), _) | + (_, ty::Projection(_)) | (_, ty::Opaque(..)) => { + // If either side is a projection, attempt to + // progress via normalization. (Should be safe to + // apply to both sides as normalization is + // idempotent.) + let a_norm = normalize(a); + let b_norm = normalize(b); + if a == a_norm && b == b_norm { + break; + } else { + a = a_norm; + b = b_norm; + } + } + _ => break, } } diff --git a/src/librustc_codegen_ssa/base.rs b/src/librustc_codegen_ssa/base.rs index ca7e17ec97a..00471095f2f 100644 --- a/src/librustc_codegen_ssa/base.rs +++ b/src/librustc_codegen_ssa/base.rs @@ -128,7 +128,8 @@ pub fn unsized_info<'tcx, Cx: CodegenMethods<'tcx>>( target: Ty<'tcx>, old_info: Option, ) -> Cx::Value { - let (source, target) = cx.tcx().struct_lockstep_tails(source, target); + let (source, target) = + cx.tcx().struct_lockstep_tails_erasing_lifetimes(source, target, cx.param_env()); match (&source.sty, &target.sty) { (&ty::Array(_, len), &ty::Slice(_)) => { cx.const_usize(len.unwrap_usize(cx.tcx())) diff --git a/src/librustc_codegen_ssa/traits/type_.rs b/src/librustc_codegen_ssa/traits/type_.rs index aa38d8d5184..13f72e23819 100644 --- a/src/librustc_codegen_ssa/traits/type_.rs +++ b/src/librustc_codegen_ssa/traits/type_.rs @@ -77,11 +77,12 @@ pub trait DerivedTypeMethods<'tcx>: BaseTypeMethods<'tcx> + MiscMethods<'tcx> { } fn type_has_metadata(&self, ty: Ty<'tcx>) -> bool { - if ty.is_sized(self.tcx().at(DUMMY_SP), ty::ParamEnv::reveal_all()) { + let param_env = ty::ParamEnv::reveal_all(); + if ty.is_sized(self.tcx().at(DUMMY_SP), param_env) { return false; } - let tail = self.tcx().struct_tail(ty); + let tail = self.tcx().struct_tail_erasing_lifetimes(ty, param_env); match tail.sty { ty::Foreign(..) => false, ty::Str | ty::Slice(..) | ty::Dynamic(..) => true, diff --git a/src/librustc_mir/interpret/cast.rs b/src/librustc_mir/interpret/cast.rs index 3ef525979f8..980697360eb 100644 --- a/src/librustc_mir/interpret/cast.rs +++ b/src/librustc_mir/interpret/cast.rs @@ -270,7 +270,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { dty: Ty<'tcx>, ) -> InterpResult<'tcx> { // A -> A conversion - let (src_pointee_ty, dest_pointee_ty) = self.tcx.struct_lockstep_tails(sty, dty); + let (src_pointee_ty, dest_pointee_ty) = + self.tcx.struct_lockstep_tails_erasing_lifetimes(sty, dty, self.param_env); match (&src_pointee_ty.sty, &dest_pointee_ty.sty) { (&ty::Array(_, length), &ty::Slice(_)) => { diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index f0d64e217a2..bcd36ac547c 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -146,7 +146,9 @@ for let value = self.ecx.read_immediate(mplace.into())?; // Handle trait object vtables if let Ok(meta) = value.to_meta() { - if let ty::Dynamic(..) = self.ecx.tcx.struct_tail(referenced_ty).sty { + if let ty::Dynamic(..) = + self.ecx.tcx.struct_tail_erasing_lifetimes(referenced_ty, self.param_env).sty + { if let Ok(vtable) = meta.unwrap().to_ptr() { // explitly choose `Immutable` here, since vtables are immutable, even // if the reference of the fat pointer is mutable diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 34892f5b8ca..423fd9036bf 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -361,7 +361,8 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> "uninitialized data in fat pointer metadata", self.path); let layout = self.ecx.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?; if layout.is_unsized() { - let tail = self.ecx.tcx.struct_tail(layout.ty); + let tail = self.ecx.tcx.struct_tail_erasing_lifetimes(layout.ty, + self.ecx.param_env); match tail.sty { ty::Dynamic(..) => { let vtable = meta.unwrap(); diff --git a/src/librustc_mir/monomorphize/collector.rs b/src/librustc_mir/monomorphize/collector.rs index bcc6ad4eac2..6e9390f7750 100644 --- a/src/librustc_mir/monomorphize/collector.rs +++ b/src/librustc_mir/monomorphize/collector.rs @@ -851,12 +851,13 @@ fn find_vtable_types_for_unsizing<'tcx>( target_ty: Ty<'tcx>, ) -> (Ty<'tcx>, Ty<'tcx>) { let ptr_vtable = |inner_source: Ty<'tcx>, inner_target: Ty<'tcx>| { + let param_env = ty::ParamEnv::reveal_all(); let type_has_metadata = |ty: Ty<'tcx>| -> bool { use syntax_pos::DUMMY_SP; - if ty.is_sized(tcx.at(DUMMY_SP), ty::ParamEnv::reveal_all()) { + if ty.is_sized(tcx.at(DUMMY_SP), param_env) { return false; } - let tail = tcx.struct_tail(ty); + let tail = tcx.struct_tail_erasing_lifetimes(ty, param_env); match tail.sty { ty::Foreign(..) => false, ty::Str | ty::Slice(..) | ty::Dynamic(..) => true, @@ -866,7 +867,7 @@ fn find_vtable_types_for_unsizing<'tcx>( if type_has_metadata(inner_source) { (inner_source, inner_target) } else { - tcx.struct_lockstep_tails(inner_source, inner_target) + tcx.struct_lockstep_tails_erasing_lifetimes(inner_source, inner_target, param_env) } }; diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 4acba6d38fa..c35136ad6df 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -315,7 +315,7 @@ impl<'a, 'tcx> Expectation<'tcx> { /// See the test case `test/run-pass/coerce-expect-unsized.rs` and #20169 /// for examples of where this comes up,. fn rvalue_hint(fcx: &FnCtxt<'a, 'tcx>, ty: Ty<'tcx>) -> Expectation<'tcx> { - match fcx.tcx.struct_tail(ty).sty { + match fcx.tcx.struct_tail_without_normalization(ty).sty { ty::Slice(_) | ty::Str | ty::Dynamic(..) => { ExpectRvalueLikeUnsized(ty) } diff --git a/src/librustc_typeck/check/wfcheck.rs b/src/librustc_typeck/check/wfcheck.rs index a41f4ec91a4..68e5e7d4fd2 100644 --- a/src/librustc_typeck/check/wfcheck.rs +++ b/src/librustc_typeck/check/wfcheck.rs @@ -366,7 +366,8 @@ fn check_item_type( let mut forbid_unsized = true; if allow_foreign_ty { - if let ty::Foreign(_) = fcx.tcx.struct_tail(item_ty).sty { + let tail = fcx.tcx.struct_tail_erasing_lifetimes(item_ty, fcx.param_env); + if let ty::Foreign(_) = tail.sty { forbid_unsized = false; } } From e4b8af5d607b465f11615abefb5bbae20486887e Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 11 Jul 2019 13:54:54 +0200 Subject: [PATCH 2/3] Regression test for issue 60431. --- ...ue-60431-unsized-tail-behind-projection.rs | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 src/test/ui/layout/issue-60431-unsized-tail-behind-projection.rs diff --git a/src/test/ui/layout/issue-60431-unsized-tail-behind-projection.rs b/src/test/ui/layout/issue-60431-unsized-tail-behind-projection.rs new file mode 100644 index 00000000000..65845d2c9fe --- /dev/null +++ b/src/test/ui/layout/issue-60431-unsized-tail-behind-projection.rs @@ -0,0 +1,35 @@ +// rust-lang/rust#60431: This is a scenario where to determine the size of +// `&Ref`, we need to know the concrete type of the last field in +// `Ref` (i.e. its "struct tail"), and determining that concrete type +// requires normalizing `Obstack::Dyn`. +// +// The old "struct tail" computation did not perform such normalization, and so +// the compiler would ICE when trying to figure out if `Ref` is a +// dynamically-sized type (DST). + +// run-pass + +use std::mem; + +pub trait Arena { + type Dyn : ?Sized; +} + +pub struct DynRef { + _dummy: [()], +} + +pub struct Ref { + _value: u8, + _dyn_arena: A::Dyn, +} + +pub struct Obstack; + +impl Arena for Obstack { + type Dyn = DynRef; +} + +fn main() { + assert_eq!(mem::size_of::<&Ref>(), mem::size_of::<&[()]>()); +} From 3c8279a2246c8bbb63f7d6aeac144b3a8adbe755 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 12 Jul 2019 11:34:23 +0200 Subject: [PATCH 3/3] Update docs to reflect review feedback. --- src/librustc/ty/util.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index 871aeb78af5..635845a65c2 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -270,6 +270,10 @@ impl<'tcx> TyCtxt<'tcx> { /// Returns the deeply last field of nested structures, or the same type if /// not a structure at all. Corresponds to the only possible unsized field, /// and its type can be used to determine unsizing strategy. + /// + /// Should only be called if `ty` has no inference variables and does not + /// need its lifetimes preserved (e.g. as part of codegen); otherwise + /// normalization attempt may cause compiler bugs. pub fn struct_tail_erasing_lifetimes(self, ty: Ty<'tcx>, param_env: ty::ParamEnv<'tcx>) @@ -287,8 +291,8 @@ impl<'tcx> TyCtxt<'tcx> { /// handle `::Assoc` and `impl Trait`); pass the identity /// function to indicate no normalization should take place. /// - /// See also `struct_tail_erasing_lifetimes`, which is what callers running - /// after type checking should use. + /// See also `struct_tail_erasing_lifetimes`, which is suitable for use + /// during codegen. pub fn struct_tail_with_normalize(self, mut ty: Ty<'tcx>, normalize: impl Fn(Ty<'tcx>) -> Ty<'tcx>) @@ -337,7 +341,8 @@ impl<'tcx> TyCtxt<'tcx> { /// For `(Foo>, Foo)`, the result will be `(Foo, Trait)`, /// whereas struct_tail produces `T`, and `Trait`, respectively. /// - /// Must only be called after type-checking is complete; otherwise + /// Should only be called if the types have no inference variables and do + /// not need their lifetimes preserved (e.g. as part of codegen); otherwise /// normalization attempt may cause compiler bugs. pub fn struct_lockstep_tails_erasing_lifetimes(self, source: Ty<'tcx>, @@ -356,8 +361,8 @@ impl<'tcx> TyCtxt<'tcx> { /// For `(Foo>, Foo)`, the result will be `(Foo, Trait)`, /// whereas struct_tail produces `T`, and `Trait`, respectively. /// - /// See also struct_lockstep_tails_erasing_lifetimes, which - /// is what callers running after type checking should use. + /// See also `struct_lockstep_tails_erasing_lifetimes`, which is suitable for use + /// during codegen. pub fn struct_lockstep_tails_with_normalize(self, source: Ty<'tcx>, target: Ty<'tcx>,