Closed
Conversation
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
|
@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".) |
This comment has been minimized.
This comment has been minimized.
Centril
reviewed
May 17, 2019
src/librustc/ty/sty.rs
Outdated
Contributor
There was a problem hiding this comment.
I thought we had done away with "principal"? (don't change it here... just... we should fix this sometime?)
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
|
Do you have some links to the related discussion, issues, RFCs? Am I right in assuming that the current unfinished implementation will allow casting |
This comment has been minimized.
This comment has been minimized.
oli-obk
reviewed
May 17, 2019
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
I presume you meant 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)? |
Contributor
Yep; just need to find the time somehow =) |
Contributor
|
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.
Ah yes I remember, I mostly thought we were talking about deduplicating the vtable generation :D
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? |
oli-obk
reviewed
May 17, 2019
src/librustc/traits/select.rs
Outdated
Contributor
There was a problem hiding this comment.
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?)
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
That sounds good; the current discussion has been centered in https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/object.20upcasting. |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
I'm probably missing something obvious... could you elaborate?
Hah, yep, that was just a natural side-topic stemming from eddyb and I chatting about how to go about implementing this feature. :-)
Will do shortly. |
Contributor
The code you commented out does this relating. I presume the result of the equality check not ending up in the |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
Right, but shouldn't the subsequent bit beginning Edit: maybe it's as simple as keeping that code and changing |
oli-obk
reviewed
May 17, 2019
Contributor
The vtable of The vtable of The vtable of or implementation defined as (note the order of So you can only ever upcast to either The vtable of The vtable of The vtable of or implementation defined as (note the order of So you can only ever upcast to either In the zulip thread there was some discussion about doing something like for Though the memory regression is probably very small, it may still have a bad impact on embedded devs. |
Contributor
Author
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? |
Contributor
|
Ah, you're right. I misread the loop in 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). |
Contributor
Author
|
@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. |
Contributor
|
I think it would be fine to just check by how much libstd and maybe some other large crates like |
Contributor
Author
|
Sounds reasonable. |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@alexreg if you can fix the tests and |
Contributor
Author
|
@Dylan-DPC Yep, will have a look at that tomorrow! :-) |
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Collaborator
|
☔ The latest upstream changes (presumably #70994) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
Author
|
Good news: the PR is rebased and passing CI now! I'm going to try to take out some of the refactoring first and land that in a separate PR, as requested above, and then land this PR after. |
Collaborator
|
☔ The latest upstream changes (presumably #71231) made this pull request unmergeable. Please resolve the merge conflicts. |
Collaborator
|
☔ The latest upstream changes (presumably #71268) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from Triage: @alexreg Hi, please resolve the conflicts. Thanks |
Contributor
Author
|
@joelpalmer Yep, resolve them locally... going to factor out some of these changes to a separate PR soon. |
Contributor
|
Closing as "too big, needs to be broken up". |
Contributor
|
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. |
Contributor
|
I'm interested in reviving this - which parts would need to be split out? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allows casting objects of type
dyn Footodyn BarwheneverFoo: Bar.r? @eddyb
CC @nikomatsakis