-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Objects should be upcastable to supertraits #2765
Comments
Reproduced with 64963d6cbaea86e0d2a58f507e57a76da7512e3e. Nominating for milestone 5, production-ready |
The error as of 6c2dc3c is:
which is a bit weird, why shouldn't you be able to cast a trait object to the same trait object? |
IINM there's two ways you could interpret this: one is that it's the identity function (casting something to its own type), the other is that you're looking for an |
I would have assumed it's the identity function |
Updating title to reflect the larger issue (casting from |
accepted for feature-complete milestone |
This doesn't seem like the identity function to me, as the vtables can be subsets of each other in weird ways (e.g. with multiple supertraits). Doing this properly will require new codegen. |
A simpler testcase:
|
this need not block 1.0. Assigning P-high as it is important for some use cases. |
To be clear: this is not a subtyping rule but a coercion one. As @bblum says, it may require substituting a new vtable. |
This is on the 1.0 milestone currently, due to @pnkfelix's comment above, I'm removing the milestone assuming that it was an accident. |
I'm curious, how we could call supertrait method on subtrait object [1], without being able to upcast such object to supertrait? Does vtable of subtrait include all methods from supertrait? Or how does it work? |
Is there any way to work around this issue in current rust?
What is layout of trait objects? I think it should be possible to "upcast" subtrait object to supertrait with unsafe manipulation of vtables. |
For future references: https://github.com/rust-lang/rust/blob/master/src/librustc/driver/pretty.rs#L137 — here is workaround for this issue. |
Sub. |
My workaround: http://stackoverflow.com/a/28664881/403742 |
Triage: https://github.com/rust-lang/rust/issues/5665#issuecomment-31582946 seems to be a decent test-case, and this is still a non-scalar cast today. |
I've started looking at this. I figured I'd try to clear up the design first. Here's my understanding: BasicsTrait objects (spelled as Smart pointersSimilar conversions should probably be possible for ImplementationTrait objects consist of a vptr and a self-pointer. The vptr points to a vtable, which contains a static array of function addresses. Each method is associated with a statically known offset in that table. To call a method on a trait object, add the offset to the vptr and call the function whose address is stored in that slot, passing the self-pointer as the first argument. (The vtable also stores some metadata, which currently includes size, alignment, and the address of the destructor. The destructor is treated specially, presumably because any concrete type may implement Currently, there is one vtable for each impl of an object-safe trait. There is no particular relationship between different vtables for a type that implements multiple traits. Supertrait method addresses are, however, duplicated into the subtrait's vtable. To support upcasting, we need a way to get from the subtrait's vtable to the supertrait's vtable. I'm planning on storing supertrait vtables next to subtraits vtables, in a recursive pattern. This would let us use statically known offsets for upcasting, because the offsets are the same for every subtrait impl. The drawback is that superclass vtables would need to be duplicated in a 'diamond' inheritance situation. (Another consequence is that two trait objects of the same trait could have different vptrs even if they were created from the same concrete type, depending on what path they took up the inheritance chain. This shouldn't matter in practice, since we don't make any guarantees about trait object equivalence.) An alternative solution would be to store offsets next to each method list, one for each supertrait we can upcast to. This representation is more compact, but I expect it would be slower because of the dependent read. Questions
|
Looks like RFC 401 answered my first question: this is supposed to be an implicit coercion. See also: rust-lang/rust#18469, the tracking issue for RFC 401, and rust-lang/rust#18600, which deals with subtrait coercions and therefore may be a duplicate of this issue. RFC 982 (DST coercions) also looks to be related. Tracking issue is rust-lang/rust#18598. |
RFC pull request 250 (postponed) dealt with this, but alongside proposals for associated fields on traits, opt-in internal vtables, and other features. The upcasting section of that proposal is basically what I had in mind. |
@typelist sorry for not getting back to you sooner! I have some thoughts about how this should work, but I think the problem can also be subdivided. Likely we should start by writing up an RFC, perhaps just for a subset of the total problem. Let me just leave various notes and let's try to sort it out a bit. Multiple traits, multiple vtablesFirst of all, this is related to the topic of supporting trait objects of multiple traits. For example, I would like to support This is orthogonal from supertrait upcasting, just wanted to throw it out there. In particular, if you have I bring it up though because the implementation strategy here may want to be shared with other trait combinations. Originally, the way I had thought to support this was to have a trait object potentially have >1 vtable (i.e., one for Note that if we adopted the strategy of making one coallesced vtable, this will affect how supertraits are stored in vtables. If we said that Supertrait upcastingOK, so, with that out of the way, I think it makes sense to focus squarely on the case of supertrait upcasting where exactly one vtable is needed. We can then extend (orthogonally) to the idea of multiple vtables. So in that case I think the basic strategy you outlined makes sense. You already cited this, but I think that the strategy outlined by @kimundi and @eddyb in RFC 250 is basically correct. Interaction with the unsize coercionYou are also already onto this one, but yes this is clearly related to the idea of coercing a Because the vtable must be adjusted, it's clear that this has to be a coercion and not an extension of subtyping, since subtyping would require that there are no repesentation changes. Moreover, by attaching it to the existing unsizing coercion, we don't have to address questions about just when it triggers -- same time as unsizing triggers. Do we need an RFC to go forward here?It seems like it can't hurt, but I'd basically just extract the relevant text from RFC 250, which I think was essentially correct. If we're just sticking to supporting single trait object to other trait object, seems harmless enough, and I can't imagine many objections. I would be very happy to work with you and draft it together! |
Thanks for the response! I started drafting an RFC based on part of RFC 250. Comments or revisions welcome. (Should we keep communicating through this issue, or some other way?) For the multi-trait case, increasing the size of the trait object does seem like the path of least resistance. It certainly makes partial upcasts (and upcasts like |
@typelist this RFC looks great! I appreciate that you took the time to talk about the motivation, alternatives and downsides. (We can keep talking over the issue, or feel free to e-mail me or ping me on IRC. I try to keep up with GH notifications, but it can be challenging.) |
@typelist @nikomatsakis Any updates on this? I'd like to push this forward. I took a stat of vtable usage in rustc. Perhaps it is useful when predicting code-size/performance effects of upcasting support. https://gist.github.com/qnighy/20184bd34d8b4c70a302fac86c7bde91 |
If this was implemented, would this compile? If so, this would be extremely helpful. For example, with TypeScript interop, I could avoid all the ugly trait Statement {}
trait IfStatement: Statement {}
struct IfStatementImpl;
impl Statement for IfStatementImpl {}
impl IfStatement for IfStatementImpl {}
fn print_statement(_statement: &Statement){
println!("print_statement");
}
fn print_if_statement(_if_statement: &IfStatement){
println!("print_if_statement");
}
fn main() {
// How come this compiles?
let ref a = IfStatementImpl {};
print_statement(a);
print_if_statement(a);
// but this does not?
let ref b: IfStatement = IfStatementImpl {};
print_statement(b);
print_if_statement(b);
} |
@ctaggart I think 50% yes. |
@rust-lang/lang, IRC suggests that this is something that should be closed or moved to the RFCs repo. |
I think the https://internals.rust-lang.org/t/casting-families-for-fast-dynamic-trait-object-casts-and-multi-trait-objects/12195 should cover this case too. |
@rust-lang/lang talked about this issue today. Allowing for evolution of Rust syntax since pre-1.0, to the best we can tell, this is something that you can now do. Closing; please comment if there's still an issue here. |
do you mean "that you cannot do" 😕. also this issue doesn't seem to have any relationship with syntax. |
Because of the ancient syntax, it's unclear what's being asked for. The 4 people in the meeting who looked at this all looked at the example code and understood the request to be about casting a reference to a struct type into a reference to a trait type (a trait that the struct type implements). If the request is about turning a sub-trait reference into a super-trait reference that is indeed not currently possible. However, since the example code only has one trait and one struct, we did not think that the sub-trait/super-trait matter was involved here. |
As one of those 4 people, my first thought was the same as yours, @kennytm -- that from the issue header this was about casting one I guess the later examples (#2765 (comment)) are that, but the one in the OP currently isn't. I'm not sure what the general intent of issues in this repo is these days. So it doesn't really bother me whether it's open or closed... |
@Lokathor the subtrait/supertrait discussion starts from #2765 (comment). In any case the wording above is not clear enough to tell what is going on — because "this is something you can now do" so the issue (which does not mention changing syntax at all) is closed??? can't comprehend. (To clarify: I'm not saying we should re-open this issue, I'm saying that comment is hard to understand.) |
The original question was about casting a trait reference to the same trait or another trait that should be part of the typeat that point. |
I'm so confused. I'm coming here from rust-lang/rust#18600 (comment) which closes itself as a duplicate of this one. But the discussion here makes it sound like it isn't a duplicate? This issue is also referenced by rust-lang/rust#18469 |
@scottmcm wrote:
I think that was exactly the issue: the example in the OP looked like something that's now possible, and it wasn't obvious that this issue was asking for something else. |
Given the amount of time that has passed, and the amount of syntax evolution, could someone involved in the original issue please either point to a current issue that's requesting the same thing using current syntax, or file one? |
Kvark's earlier example still doesn't compile:
|
I actually made a post about this on reddit confused why this wasn't working a few days ago here. If you're looking to collect more examples of confusion, here's another one. |
As I noted above, one could develop this outside rustc+core using https://internals.rust-lang.org/t/casting-families-for-fast-dynamic-trait-object-casts-and-multi-trait-objects/12195 You'll need to manually insert compilation units that unify your casting families however. |
Side note that I was thinking that I would like to mentor someone through the implementation steps required here. I think that's the primary blocker. Perhaps I will file a lang team proposal around it first though. |
create explicit struct reading pack indexes use generics rather than trait objects as casting from &Trait to &SuperTrait is not implemented (rust-lang/rfcs#2765) and its very inconveninent to not be able to pass `&mut BufReadSeek` to a function that expects a `&mut BufRead`. Furthermore, the main reason we were using dynamic dispatch in the first place was just to make certain traits (Serialize & hence BitObj) object safe. I think Deserialize was rewritten to use dyn to be consistent with Serialize.
Current tracking issue for this is rust-lang/rust#65991 |
error: failed to find an implementation of trait @T for T
The text was updated successfully, but these errors were encountered: