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

Trait upcasting coercion (part4) #88010

Closed
wants to merge 2 commits into from
Closed
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
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub enum UnsafetyViolationDetails {
MutationOfLayoutConstrainedField,
BorrowOfLayoutConstrainedField,
CallToFunctionWith,
TraitUpcastingCoercionOfRawPointer,
}

impl UnsafetyViolationDetails {
Expand Down Expand Up @@ -102,6 +103,10 @@ impl UnsafetyViolationDetails {
"call to function with `#[target_feature]`",
"can only be called if the required target features are available",
),
TraitUpcastingCoercionOfRawPointer => (
"trait upcasting coercion from raw pointer type",
"trait upcasting coercion depends on the validity of the pointer metadata",
),
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion compiler/rustc_mir/src/monomorphize/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,12 @@ fn find_vtable_types_for_unsizing<'tcx>(
assert_eq!(source_adt_def, target_adt_def);

let CustomCoerceUnsized::Struct(coerce_index) =
monomorphize::custom_coerce_unsize_info(tcx, source_ty, target_ty);
crate::transform::custom_coerce_unsize_info(
tcx,
source_ty,
target_ty,
ty::ParamEnv::reveal_all(),
);

let source_fields = &source_adt_def.non_enum_variant().fields;
let target_fields = &target_adt_def.non_enum_variant().fields;
Expand Down
29 changes: 0 additions & 29 deletions compiler/rustc_mir/src/monomorphize/mod.rs
Original file line number Diff line number Diff line change
@@ -1,33 +1,4 @@
use rustc_middle::traits;
use rustc_middle::ty::adjustment::CustomCoerceUnsized;
use rustc_middle::ty::{self, Ty, TyCtxt};

use rustc_hir::lang_items::LangItem;

pub mod collector;
pub mod partitioning;
pub mod polymorphize;
pub mod util;

fn custom_coerce_unsize_info<'tcx>(
tcx: TyCtxt<'tcx>,
source_ty: Ty<'tcx>,
target_ty: Ty<'tcx>,
) -> CustomCoerceUnsized {
let def_id = tcx.require_lang_item(LangItem::CoerceUnsized, None);

let trait_ref = ty::Binder::dummy(ty::TraitRef {
def_id,
substs: tcx.mk_substs_trait(source_ty, &[target_ty.into()]),
});

match tcx.codegen_fulfill_obligation((ty::ParamEnv::reveal_all(), trait_ref)) {
Ok(traits::ImplSource::UserDefined(traits::ImplSourceUserDefinedData {
impl_def_id,
..
})) => tcx.coerce_unsized_info(impl_def_id).custom_kind.unwrap(),
impl_source => {
bug!("invalid `CoerceUnsized` impl_source: {:?}", impl_source);
}
}
}
70 changes: 69 additions & 1 deletion compiler/rustc_mir/src/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_hir::Node;
use rustc_middle::mir::visit::{MutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_session::lint::builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::Level;

Expand Down Expand Up @@ -132,6 +132,24 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
self.register_violations(&violations, &unsafe_blocks);
}
},
Rvalue::Cast(kind, source, mir_cast_ty) => {
if let CastKind::Pointer(ty::adjustment::PointerCast::Unsize) = kind {
let mir_source_ty = match *source {
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to use Operand::ty here, right?

Copy link
Member Author

@crlf0710 crlf0710 Aug 13, 2021

Choose a reason for hiding this comment

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

I'm not totally sure, since this is earlier than codegen phase, if i'm not misunderstanding it. I'll take a look.

Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with codegen. I think you basically inlined Operand::ty here.

Operand::Copy(ref place) | Operand::Move(ref place) => {
place.as_ref().ty(self.body, self.tcx).ty
}
Operand::Constant(ref constant) => constant.ty(),
};

if self.is_trait_upcasting_coercion_from_raw_pointer(mir_source_ty, mir_cast_ty)
{
self.require_unsafe(
UnsafetyViolationKind::General,
UnsafetyViolationDetails::TraitUpcastingCoercionOfRawPointer,
)
}
}
}
_ => {}
}
self.super_rvalue(rvalue, location);
Expand Down Expand Up @@ -370,6 +388,56 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
)
}
}

fn is_trait_upcasting_coercion_pointee(&self, source: Ty<'tcx>, target: Ty<'tcx>) -> bool {
match (source.kind(), target.kind()) {
(&ty::Dynamic(data_a, ..), &ty::Dynamic(data_b, ..)) => {
data_a.principal_def_id() != data_b.principal_def_id()
Copy link
Member

Choose a reason for hiding this comment

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

Why is it okay to allow casts where data_a.principal_def_id() == data_b.principal_def_id()? Please add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok!

}
_ => false,
}
}

fn is_trait_upcasting_coercion_from_raw_pointer(
&self,
source: Ty<'tcx>,
target: Ty<'tcx>,
) -> bool {
match (source.kind(), target.kind()) {
(
&ty::Ref(_, _a, _),
&ty::Ref(_, _b, _) | &ty::RawPtr(ty::TypeAndMut { ty: _b, .. }),
) => false,
(
&ty::RawPtr(ty::TypeAndMut { ty: a, .. }),
&ty::RawPtr(ty::TypeAndMut { ty: b, .. }),
) => {
if self.is_trait_upcasting_coercion_pointee(a, b) {
true
} else {
false
}
}
(&ty::Adt(def_a, substs_a), &ty::Adt(def_b, substs_b)) => {
let custom_coerce_unsized = crate::transform::custom_coerce_unsize_info(
self.tcx,
source,
target,
self.param_env,
);
match custom_coerce_unsized {
ty::adjustment::CustomCoerceUnsized::Struct(field_idx) => {
assert_eq!(def_a, def_b);
let field = def_a.non_enum_variant().fields.get(field_idx).unwrap();
let field_ty_a = field.ty(self.tcx, substs_a);
let field_ty_b = field.ty(self.tcx, substs_b);
self.is_trait_upcasting_coercion_from_raw_pointer(field_ty_a, field_ty_b)
}
}
}
_ => bug!("is_trait_upcasting_coercion_from_raw_pointer: called on bad types"),
}
}
}

pub(crate) fn provide(providers: &mut Providers) {
Expand Down
29 changes: 28 additions & 1 deletion compiler/rustc_mir/src/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@ use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
use rustc_index::vec::IndexVec;
use rustc_middle::mir::visit::Visitor as _;
use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted};
use rustc_middle::traits;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt, TypeFoldable};
use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable};
use rustc_span::{Span, Symbol};
use rustc_hir::lang_items::LangItem;

use std::borrow::Cow;

pub mod abort_unwinding_calls;
Expand Down Expand Up @@ -625,3 +628,27 @@ fn promoted_mir<'tcx>(

tcx.arena.alloc(promoted)
}

pub(crate) fn custom_coerce_unsize_info<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this function would be better put somewhere in rustc_mir/src/util?

Copy link
Member Author

Choose a reason for hiding this comment

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

should i create a new module in util for this?

Copy link
Member

Choose a reason for hiding this comment

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

If none of the others seem to fit, yeah.

tcx: TyCtxt<'tcx>,
source_ty: Ty<'tcx>,
target_ty: Ty<'tcx>,
param_env: ty::ParamEnv<'tcx>,
) -> ty::adjustment::CustomCoerceUnsized {
let def_id = tcx.require_lang_item(LangItem::CoerceUnsized, None);

let trait_ref = ty::Binder::dummy(ty::TraitRef {
def_id,
substs: tcx.mk_substs_trait(source_ty, &[target_ty.into()]),
});

match tcx.codegen_fulfill_obligation((param_env, trait_ref)) {
Ok(traits::ImplSource::UserDefined(traits::ImplSourceUserDefinedData {
impl_def_id,
..
})) => tcx.coerce_unsized_info(impl_def_id).custom_kind.unwrap(),
impl_source => {
bug!("invalid `CoerceUnsized` impl_source: {:?}", impl_source);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#![feature(trait_upcasting)]
#![allow(incomplete_features)]

trait A {
fn foo_a(&self) {}
}

trait B {
fn foo_b(&self) {}
}

trait C: A + B {
fn foo_c(&self) {}
}

struct S;

impl A for S {}

impl B for S {}

impl C for S {}

fn unsafe_coercion(v: * const dyn C) {
let g: * const dyn A = v;
//~^ ERROR trait upcasting coercion from
}


fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0133]: trait upcasting coercion from raw pointer type is unsafe and requires unsafe function or block
--> $DIR/unsafe-trait-upcasting-coercion.rs:25:28
|
LL | let g: * const dyn A = v;
| ^ trait upcasting coercion from raw pointer type
|
= note: trait upcasting coercion depends on the validity of the pointer metadata

error: aborting due to previous error

For more information about this error, try `rustc --explain E0133`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#![feature(trait_upcasting)]
#![allow(incomplete_features)]

trait A {
fn foo_a(&self) {}
}

trait B {
fn foo_b(&self) {}
}

trait C: A + B {
fn foo_c(&self) {}
}

struct S;

impl A for S {}

impl B for S {}

impl C for S {}

fn invalid_coercion(v: * const Box<dyn C>) {
let g: * const Box<dyn A> = v;
//~^ ERROR mismatched types
}

fn main() {}
Copy link
Member

Choose a reason for hiding this comment

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

Why are these two separate files?

Please also add some casts that are okay.

Does this hit the last match arm in is_trait_upcasting_coercion_from_raw_pointer?

Copy link
Member Author

@crlf0710 crlf0710 Aug 13, 2021

Choose a reason for hiding this comment

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

This is a separate file because the "mismatched types" error will prevent unsafety checker from running at all, it seems. I don't really know whether error recovery is possible at all in this case. This test case is just the status quo. I'll leave comments, when the unsafety checking implementation is ready.

Please also add some casts that are okay.

I will, thanks!

Does this hit the last match arm in is_trait_upcasting_coercion_from_raw_pointer?

No it doesn't, the unsafety checker is not yet running in this case.

Copy link
Member

Choose a reason for hiding this comment

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

No it doesn't, the unsafety checker is not yet running in this case.

Okay, then the other test should definitely do something to hit that arm.

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0308]: mismatched types
--> $DIR/unsafe-trait-upcasting-coercion2.rs:25:33
|
LL | let g: * const Box<dyn A> = v;
| ------------------ ^ expected trait `A`, found trait `C`
| |
| expected due to this
|
= note: expected raw pointer `*const Box<dyn A>`
found raw pointer `*const Box<(dyn C + 'static)>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.