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

Allow dropping dyn Trait principal #126660

Closed
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
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_cranelift/src/unsize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ pub(crate) fn unsized_info<'tcx>(
{
let old_info =
old_info.expect("unsized_info: missing old info for trait upcasting coercion");
if data_a.principal_def_id() == data_b.principal_def_id() {
let b_principal_def_id = data_b.principal_def_id();
if data_a.principal_def_id() == b_principal_def_id || b_principal_def_id.is_none() {
// A NOP cast that doesn't actually change anything, should be allowed even with invalid vtables.
return old_info;
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ pub fn unsized_info<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
{
let old_info =
old_info.expect("unsized_info: missing old info for trait upcasting coercion");
if data_a.principal_def_id() == data_b.principal_def_id() {
let b_principal_def_id = data_b.principal_def_id();
if data_a.principal_def_id() == b_principal_def_id || b_principal_def_id.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if you did not do the is_none check here? Does supertrait_vtable_slot return a bogus vtable entry or ICE or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

Below, if supertrait_vtable_slot returns None we also just keep the old pointer. So is this here just an optimization to avoid calling supertrait_vtable_slot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this here just an optimization to avoid calling supertrait_vtable_slot?

@compiler-errors wrote this code, but I believe that is correct

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I was asking this while writing #127856, and that PR now has a bunch of assertions double-checking my understanding of the relevant invariants here.

// A NOP cast that doesn't actually change anything, should be allowed even with invalid vtables.
return old_info;
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_next_trait_solver/src/solve/trait_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,8 @@ where
let mut responses = vec![];
// If the principal def ids match (or are both none), then we're not doing
// trait upcasting. We're just removing auto traits (or shortening the lifetime).
if a_data.principal_def_id() == b_data.principal_def_id() {
let b_principal_def_id = b_data.principal_def_id();
if a_data.principal_def_id() == b_principal_def_id || b_principal_def_id.is_none() {
responses.extend(self.consider_builtin_upcast_to_principal(
goal,
CandidateSource::BuiltinImpl(BuiltinImplSource::Misc),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// #2 (region bounds).
let principal_def_id_a = a_data.principal_def_id();
let principal_def_id_b = b_data.principal_def_id();
if principal_def_id_a == principal_def_id_b {
if principal_def_id_a == principal_def_id_b || principal_def_id_b.is_none() {
// We may upcast to auto traits that are either explicitly listed in
// the object type's bounds, or implied by the principal trait ref's
// supertraits.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// We already checked the compatibility of auto traits within `assemble_candidates_for_unsizing`.
let iter = data_a
.principal()
.filter(|_| {
// optionally drop the principal, if we're unsizing to no principal
data_b.principal().is_some()
})
.map(|b| b.map_bound(ty::ExistentialPredicate::Trait))
.into_iter()
.chain(
Expand Down
51 changes: 51 additions & 0 deletions src/tools/miri/tests/pass/dyn-upcast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ fn main() {
struct_();
replace_vptr();
vtable_nop_cast();
drop_principal();
}

fn vtable_nop_cast() {
Expand Down Expand Up @@ -430,3 +431,53 @@ fn replace_vptr() {
let s = S(42);
invoke_outer(&s);
}

fn drop_principal() {
Copy link
Member

@RalfJung RalfJung Jul 16, 2024

Choose a reason for hiding this comment

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

The equivalent code with transmute causes Miri to report UB. So something is very odd here that makes this pass.

Is it maybe that these get translated to actual casts, and they get a new vtable? But Box can't be in a cast so I don't think so.
EDIT: Box can be in an unsizing cast so I think this is what happens.

Anyway if you think the transmute version should be sound then please also add that as a Miri test.

Copy link
Contributor Author

@Jules-Bertholet Jules-Bertholet Jul 16, 2024

Choose a reason for hiding this comment

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

Anyway if you think the transmute version should be sound

I don’t see any reason to commit to that at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should commit the version regardless, to make it intentional if we change it later 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I don’t see any reason to commit to that at this time.

After playing around with this more, I agree. I realized that there are already trait-changing casts which are NOPs at runtime, namely when the supertrait vtable is a prefix of the subtrait vtable. We compile these casts to a NOP but we don't want to allow people to do this as a transmute; the fact that this is a NOP is an implementation detail. The same applies to this case: semantically, Debug + Send to Send is still a meaningful cast, even if we compile it to a NOP.

We do currently make Debug + Send to Debug a non-meaningful cast; vtables in Miri only keep track of the principal trait. Maybe that is something we should reconsider, to be fully consistent.

use std::{alloc::Layout, any::Any};

const fn yeet_principal(x: Box<dyn Any + Send>) -> Box<dyn Send> {
x
}

trait Bar: Send + Sync {}

impl<T: Send + Sync> Bar for T {}

const fn yeet_principal_2(x: Box<dyn Bar>) -> Box<dyn Send> {
x
}

struct CallMe<F: FnOnce()>(Option<F>);

impl<F: FnOnce()> CallMe<F> {
fn new(f: F) -> Self {
CallMe(Some(f))
}
}

impl<F: FnOnce()> Drop for CallMe<F> {
fn drop(&mut self) {
(self.0.take().unwrap())();
}
}

fn goodbye() {
println!("goodbye");
}

let x = Box::new(CallMe::new(goodbye)) as Box<dyn Any + Send>;
let x_layout = Layout::for_value(&*x);
let y = yeet_principal(x);
let y_layout = Layout::for_value(&*y);
assert_eq!(x_layout, y_layout);
println!("before");
drop(y);

let x = Box::new(CallMe::new(goodbye)) as Box<dyn Bar>;
let x_layout = Layout::for_value(&*x);
let y = yeet_principal_2(x);
let y_layout = Layout::for_value(&*y);
assert_eq!(x_layout, y_layout);
println!("before");
drop(y);
}
4 changes: 4 additions & 0 deletions src/tools/miri/tests/pass/dyn-upcast.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
before
goodbye
before
goodbye
14 changes: 0 additions & 14 deletions tests/ui/impl-trait/unsized_coercion5.next.stderr

This file was deleted.

18 changes: 3 additions & 15 deletions tests/ui/impl-trait/unsized_coercion5.old.stderr
Original file line number Diff line number Diff line change
@@ -1,24 +1,12 @@
error[E0308]: mismatched types
--> $DIR/unsized_coercion5.rs:16:32
|
LL | let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
| ------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected trait `Send`, found trait `Trait + Send`
| |
| expected due to this
|
= note: expected struct `Box<dyn Send>`
found struct `Box<dyn Trait + Send>`

error[E0277]: the size for values of type `impl Trait + ?Sized` cannot be known at compilation time
--> $DIR/unsized_coercion5.rs:16:32
--> $DIR/unsized_coercion5.rs:17:32
|
LL | let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
| ^ doesn't have a size known at compile-time
|
= help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
= note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`

error: aborting due to 2 previous errors
error: aborting due to 1 previous error

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
For more information about this error, try `rustc --explain E0277`.
2 changes: 1 addition & 1 deletion tests/ui/impl-trait/unsized_coercion5.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

//@ revisions: next old
//@[next] compile-flags: -Znext-solver
//@[next] check-pass

#![feature(trait_upcasting)]

Expand All @@ -15,7 +16,6 @@ fn hello() -> Box<impl Trait + ?Sized> {
let x = hello();
let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
//[old]~^ ERROR: the size for values of type `impl Trait + ?Sized` cannot be know
//~^^ ERROR: mismatched types
}
Box::new(1u32)
}
Expand Down
68 changes: 68 additions & 0 deletions tests/ui/traits/dyn-drop-principal.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
//@ run-pass
//@ check-run-results

use std::{alloc::Layout, any::Any};

const fn yeet_principal(x: Box<dyn Any + Send>) -> Box<dyn Send> {
x
}

trait Bar: Send + Sync {}

impl<T: Send + Sync> Bar for T {}

const fn yeet_principal_2(x: Box<dyn Bar>) -> Box<dyn Send> {
x
}

struct CallMe<F: FnOnce()>(Option<F>);

impl<F: FnOnce()> CallMe<F> {
fn new(f: F) -> Self {
CallMe(Some(f))
}
}

impl<F: FnOnce()> Drop for CallMe<F> {
fn drop(&mut self) {
(self.0.take().unwrap())();
}
}

fn goodbye() {
println!("goodbye");
}

fn main() {
let x = Box::new(CallMe::new(goodbye)) as Box<dyn Any + Send>;
let x_layout = Layout::for_value(&*x);
let y = yeet_principal(x);
let y_layout = Layout::for_value(&*y);
assert_eq!(x_layout, y_layout);
println!("before");
drop(y);

let x = Box::new(CallMe::new(goodbye)) as Box<dyn Bar>;
let x_layout = Layout::for_value(&*x);
let y = yeet_principal_2(x);
let y_layout = Layout::for_value(&*y);
assert_eq!(x_layout, y_layout);
println!("before");
drop(y);
}

// Test that upcast works in `const`

const fn yeet_principal_3(x: &(dyn Any + Send + Sync)) -> &(dyn Send + Sync) {
x
}

#[used]
pub static FOO: &(dyn Send + Sync) = yeet_principal_3(&false);

const fn yeet_principal_4(x: &dyn Bar) -> &(dyn Send + Sync) {
x
}

#[used]
pub static BAR: &(dyn Send + Sync) = yeet_principal_4(&false);
4 changes: 4 additions & 0 deletions tests/ui/traits/dyn-drop-principal.run.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
before
goodbye
before
goodbye
Loading