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 #60900

Closed
wants to merge 14 commits into from
Closed

Trait upcasting #60900

wants to merge 14 commits into from

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented May 17, 2019

Allows casting objects of type dyn Foo to dyn Bar whenever Foo: Bar.

r? @eddyb

CC @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2019
@alexreg
Copy link
Contributor Author

alexreg commented May 17, 2019

@eddyb This isn't complete yet, but an initial review would be appreciated. Also, if you could elaborate slightly on what remains, that would be helpful. (At the moment I don't think upcast objects get the right vtable pointer still, i.e. it just points to the start of the overall vtable, not the "subtable".)

@rust-highfive

This comment has been minimized.

src/librustc/mir/interpret/pointer.rs Outdated Show resolved Hide resolved
src/librustc/mir/interpret/pointer.rs Outdated Show resolved Hide resolved
src/librustc/mir/interpret/pointer.rs Outdated Show resolved Hide resolved
src/librustc/traits/select.rs Outdated Show resolved Hide resolved
src/librustc/traits/select.rs Outdated Show resolved Hide resolved
///
/// A Rust trait object type consists (in addition to a lifetime bound)
/// of a set of trait bounds, which are separated into any number
/// of auto-trait bounds, and at most 1 non-auto-trait bound. The
/// of auto-trait bounds, and at most one non-auto-trait bound. The
/// non-auto-trait bound is called the "principal" of the trait
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we had done away with "principal"? (don't change it here... just... we should fix this sometime?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we kind of did, in my other PR (not yet merged), but it turns out there's a few more things. Easy to remove though.

src/librustc_codegen_ssa/meth.rs Show resolved Hide resolved
src/librustc_mir/interpret/traits.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/traits.rs Outdated Show resolved Hide resolved
src/librustc_mir/monomorphize/collector.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2019

Do you have some links to the related discussion, issues, RFCs?

Am I right in assuming that the current unfinished implementation will allow casting dyn Foo to Bar and Boo if Foo: Bar + Foo, even though one of these is definitely not possible because the vtables can't match? So the implementation strategy is solely for zero cost upcasting where the start of Foo's vtable is equal to the start of Bar's vtable?

@oli-obk

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@alexreg

This comment has been minimized.

@alexreg
Copy link
Contributor Author

alexreg commented May 17, 2019

Do you have some links to the related discussion, issues, RFCs?

Am I right in assuming that the current unfinished implementation will allow casting dyn Foo to Bar and Boo if Foo: Bar + Foo, even though one of these is definitely not possible because the vtables can't match? So the implementation strategy is solely for zero cost upcasting where the start of Foo's vtable is equal to the start of Bar's vtable?

I presume you meant Boo: Bar + Foo? This PR isn't allowing multi-trait objects though, so we don't need to worry about such cases unless one of Foo or Bar is an auto-trait, in which case the vtable isn't affected.

Anyway, for background, @nikomatsakis and I have discussed this feature informally for some time now, and we had a conversation on Zulip a few weeks ago, which @eddyb also participated in – I believe Eddy tagged you in a question right at the end and you replied, so you might have seen some of it? He explains the vtable layout in any case (which I may have gotten wrong in this PR, but hopefully not).

I believe the plan of action is to review & merge this PR under "experimentation in master", but write up an RFC simultaneously, if possible. I forget if anyone already volunteered for that (@Centril)?

@Centril
Copy link
Contributor

Centril commented May 17, 2019

I believe the plan of action is to review & merge this PR under "experimentation in master", but write up an RFC simultaneously, if possible. I forget if anyone already volunteered for that (@Centril)?

Yep; just need to find the time somehow =)

@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2019

I am not talking about multi-trait object, but about traits with multiple parents:

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

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

trait Foo: A + B {}

impl A for () {}
impl B for () {}
impl Foo for () {}


fn main() {
    let x: &Foo = &();
    let y: &A = x;
}

Right now this will be unsound with the implementation at hand.

tagged you in a question right at the end and you replied, so you might have seen some of it?

Ah yes I remember, I mostly thought we were talking about deduplicating the vtable generation :D

I believe the plan of action is to review & merge this PR under "experimentation in master", but write up an RFC simultaneously, if possible.

Ok, can you open a tracking issue with some info about what's being done so PRs can reference it and we have a sort of central point to talk about things and collect links?


/*
Copy link
Contributor

Choose a reason for hiding this comment

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

So basically right now all that needs doing is put a feature gate check around this code instead of uncommenting? (in order to get upcasting to parent traits if there is only a single parent trait?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, basically. I'll want to feature gate data_a.principal_def_id() == data_b.principal_def_id() in assemble_candidates_for_unsizing too, but that's probably about it.

@Centril
Copy link
Contributor

Centril commented May 17, 2019

Ok, can you open a tracking issue with some info about what's being done so PRs can reference it and we have a sort of central point to talk about things and collect links?

That sounds good; the current discussion has been centered in https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/object.20upcasting.

@alexreg

This comment has been minimized.

@alexreg
Copy link
Contributor Author

alexreg commented May 17, 2019

@oli-obk

I am not talking about multi-trait object, but about traits with multiple parents:

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

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

trait Foo: A + B {}

impl A for () {}
impl B for () {}
impl Foo for () {}


fn main() {
    let x: &Foo = &();
    let y: &A = x;
}

Right now this will be unsound with the implementation at hand.

I'm probably missing something obvious... could you elaborate?

tagged you in a question right at the end and you replied, so you might have seen some of it?

Ah yes I remember, I mostly thought we were talking about deduplicating the vtable generation :D

Hah, yep, that was just a natural side-topic stemming from eddyb and I chatting about how to go about implementing this feature. :-)

I believe the plan of action is to review & merge this PR under "experimentation in master", but write up an RFC simultaneously, if possible.

Ok, can you open a tracking issue with some info about what's being done so PRs can reference it and we have a sort of central point to talk about things and collect links?

Will do shortly.

@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2019

Any ideas?

The code you commented out does this relating. I presume the result of the equality check not ending up in the nested variable causes Trait + Send to be upcastable to neither Send nor Trait

@rust-highfive

This comment has been minimized.

@alexreg
Copy link
Contributor Author

alexreg commented May 17, 2019

The code you commented out does this relating. I presume the result of the equality check not ending up in the nested variable causes Trait + Send to be upcastable to neither Send nor Trait

Right, but shouldn't the subsequent bit beginning // Register an obligation for `dyn TraitA: TraitB`. cover for that?

Edit: maybe it's as simple as keeping that code and changing .eq to .lub...

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2019

I'm probably missing something obvious... could you elaborate?

The vtable of () as A looks like

* ()::drop
* 1 (align)
* 0 (size)
* <() as A>::a

The vtable of () as B looks like

* ()::drop
* 1 (align)
* 0 (size)
* <() as B>::b

The vtable of () as Foo looks like

* ()::drop
* 1 (align)
* 0 (size)
* <() as A>::a
* <() as B>::b

or implementation defined as

* ()::drop
* 1 (align)
* 0 (size)
* <() as B>::b
* <() as A>::a

(note the order of a and b)

So you can only ever upcast to either A or B when going from Foo, as only one of the vtables is fully available in Foo's vtable.> I'm probably missing something obvious... could you elaborate?

The vtable of () as A looks like

* ()::drop
* 1 (align)
* 0 (size)
* <() as A>::a

The vtable of () as B looks like

* ()::drop
* 1 (align)
* 0 (size)
* <() as B>::b

The vtable of () as Foo looks like

* ()::drop
* 1 (align)
* 0 (size)
* <() as A>::a
* <() as B>::b

or implementation defined as

* ()::drop
* 1 (align)
* 0 (size)
* <() as B>::b
* <() as A>::a

(note the order of a and b)

So you can only ever upcast to either A or B when going from Foo, as only one of the vtables is fully available in Foo's vtable.

In the zulip thread there was some discussion about doing something like

* ()::drop
* 1 (align)
* 0 (size)
* <() as B>::b
* ()::drop
* 1 (align)
* 0 (size)
* <() as A>::a

for Foo's vtable in order to have both bases to upcast to. I don't remember what the discussion on that was, and it also makes this feature not zero cost, so I don't see how to enable this feature without impacting stable users except by restricting it to upcasts for traits with a single base trait.

Though the memory regression is probably very small, it may still have a bad impact on embedded devs.

@alexreg
Copy link
Contributor Author

alexreg commented May 19, 2019

In the zulip thread there was some discussion about doing something like

Oh, I thought that's what I did implement already... I'm pretty sure it is, in fact. Can you explain why it isn't?

@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2019

Ah, you're right. I misread the loop in meth.rs.

I'm not sure how to measure the impact of this. This will have some effect on binary sizes I presume (since all vtables of traits with multiple parent traits will now be bigger by 24 bytes per parent trait beyond the first).

@alexreg
Copy link
Contributor Author

alexreg commented May 22, 2019

@oli-obk Yeah... I think we (@nikomatsakis, @eddyb, myself) just implicitly discounted that as not so important during our conversation. If it's really a concern, maybe there's some sort of crater-like test we can run? I do think it's a very reasonable and small "hit" to take in any case.

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2019

I think it would be fine to just check by how much libstd and maybe some other large crates like cargo change their binary size. If it's negligible then we can just ignore it.

@alexreg
Copy link
Contributor Author

alexreg commented May 26, 2019

Sounds reasonable.

@bors

This comment has been minimized.

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 5, 2020
@joelpalmer
Copy link

Ping from Triage: @alexreg Hi, please resolve the conflicts. Thanks

@alexreg
Copy link
Contributor Author

alexreg commented May 5, 2020

@joelpalmer Yep, resolve them locally... going to factor out some of these changes to a separate PR soon.

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2020
@nikomatsakis
Copy link
Contributor

Closing as "too big, needs to be broken up".

@Others
Copy link
Contributor

Others commented Aug 1, 2020

Did this ever go anywhere after this PR was closed? I have a use-case for this and I'm wondering if there has been any progress on trait upcasting, even on nightly.

@Aaron1011
Copy link
Member

I'm interested in reviving this - which parts would need to be split out?

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2021
…omatsakis

Trait upcasting coercion (part1)

This revives the first part of earlier PR rust-lang#60900 .

It's not very clear to me which parts of that pr was design decisions, so i decide to cut it into pieces and land them incrementally. This allows more eyes on the details.

This is the first part, it adds feature gates, adds feature gates tests, and implemented the unsize conversion part.
(I hope i have dealt with the `ExistentialTraitRef` values correctly...)

The next part will be implementing the pointer casting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-trait_upcasting `#![feature(trait_upcasting)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.