Skip to content

Commit

Permalink
Rollup merge of rust-lang#62585 - pnkfelix:issue-60431-make-struct-ta…
Browse files Browse the repository at this point in the history
…il-normalize-when-possible, r=eddyb

Make struct_tail normalize when possible

As noted in commit message: this replaces the existing methods to extract the struct tail(s) with new entry points that make the handling of normalization explicit.

Most of the places that call `struct_tail` are during codegen, post type-checking, and therefore they can get away with using `tcx.normalize_erasing_regions` (this is the entry point `struct_tail_erasing_lifetimes`)

For other cases that may arise, one can use the core method, which is parameterized over the normalization `Ty -> Ty` closure (`struct_tail_with_normalize`).

Or one can use the trivial entry point that does not normalization (`struct_tail_without_normalization`)

----

I spent a little while trying to make a test that exposed the bug via `impl Trait` rather than a projection, but I failed to find something that tripped up the current nightly `rustc`.
 * I have *not* spent any time trying to make tests that trip up the other places where `struct_tail` was previously being called. While I do think the task of making such tests could be worthwhile, I am simply running out of time. (Its also possible that the layout code is always the first point called, and thus it may be pointless to try to come up with such tests.)

I also spent a little time discussing with @eddyb where this code should live. They suggested moving `struct_tail` and its sibling `struct_lockstep_tails` to the `LayoutCx`.  But in the interest of time, I have left that refactoring (which may be questionable at this point) to a follow-up task.

----

Fix rust-lang#60431
  • Loading branch information
Centril committed Jul 13, 2019
2 parents 5e1891c + 3c8279a commit 833dada
Show file tree
Hide file tree
Showing 11 changed files with 151 additions and 22 deletions.
6 changes: 3 additions & 3 deletions src/librustc/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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(_, _) => {
Expand Down
102 changes: 94 additions & 8 deletions src/librustc/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,46 @@ 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.
///
/// 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>)
-> 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 `<T as Trait>::Assoc` and `impl Trait`); pass the identity
/// function to indicate no normalization should take place.
///
/// 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>)
-> Ty<'tcx>
{
loop {
match ty.sty {
ty::Adt(def, substs) => {
Expand All @@ -281,6 +317,15 @@ impl<'tcx> TyCtxt<'tcx> {
}
}

ty::Projection(_) | ty::Opaque(..) => {
let normalized = normalize(ty);
if ty == normalized {
return ty;
} else {
ty = normalized;
}
}

_ => {
break;
}
Expand All @@ -294,10 +339,35 @@ impl<'tcx> TyCtxt<'tcx> {
/// structure definitions.
/// For `(Foo<Foo<T>>, Foo<dyn Trait>)`, the result will be `(Foo<T>, 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>) {
///
/// 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>,
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<T>>, Foo<dyn Trait>)`, the result will be `(Foo<T>, Trait)`,
/// whereas struct_tail produces `T`, and `Trait`, respectively.
///
/// 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>,
normalize: impl Fn(Ty<'tcx>) -> Ty<'tcx>)
-> (Ty<'tcx>, Ty<'tcx>)
{
let (mut a, mut b) = (source, target);
loop {
match (&a.sty, &b.sty) {
Expand All @@ -319,6 +389,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,
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_codegen_ssa/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ pub fn unsized_info<'tcx, Cx: CodegenMethods<'tcx>>(
target: Ty<'tcx>,
old_info: Option<Cx::Value>,
) -> 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()))
Expand Down
5 changes: 3 additions & 2 deletions src/librustc_codegen_ssa/traits/type_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/interpret/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
dty: Ty<'tcx>,
) -> InterpResult<'tcx> {
// A<Struct> -> A<Trait> 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(_)) => {
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
7 changes: 4 additions & 3 deletions src/librustc_mir/monomorphize/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_typeck/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
35 changes: 35 additions & 0 deletions src/test/ui/layout/issue-60431-unsized-tail-behind-projection.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// rust-lang/rust#60431: This is a scenario where to determine the size of
// `&Ref<Obstack>`, we need to know the concrete type of the last field in
// `Ref<Obstack>` (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<Obstack>` is a
// dynamically-sized type (DST).

// run-pass

use std::mem;

pub trait Arena {
type Dyn : ?Sized;
}

pub struct DynRef {
_dummy: [()],
}

pub struct Ref<A: Arena> {
_value: u8,
_dyn_arena: A::Dyn,
}

pub struct Obstack;

impl Arena for Obstack {
type Dyn = DynRef;
}

fn main() {
assert_eq!(mem::size_of::<&Ref<Obstack>>(), mem::size_of::<&[()]>());
}

0 comments on commit 833dada

Please sign in to comment.