rust-lang/rust

Tracking issue for dyn upcasting coercion

alexreg opened this issue · 84 comments

alexreg commented

This is a tracking issue for RFC3324. Corresponding MCP is here.
The feature gate for the issue is #![feature(trait_upcasting)].

STATUS UPDATE

We are in the process of stabilizing.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Previous discussions

Unresolved Questions

  • Currently CoerceUnsized trait cannot express this case: a smart pointers that wrap a raw pointer and don't guarantee via a custom invariant that it is valid. Maybe a separate CoerceUnsizedUnsafe trait is needed. (see #88010 (comment)).
  • Should we make upcasting opt-in in some form to limit vtable size by default? The current inclination of the lang-team is "no", but it would be useful to gather data on how much supporting upcasting contributors to overall binary size.
  • Before stabilizing it we should check that libs-api is ok with upcasting for all dyn-allowed traits in the library, since those we can't change. (addressed by #65991 (comment))
  • Should we add an opt-out mechanism, and extend library stabilization checklist with "do we want to opt-out for now"?

Implementation history

nikomatsakis commented

This is currently blocked on resolving the safety conditions for upcasting. The following updated document describes my current thinking, looking for feedback:

https://rust-lang.github.io/dyn-upcasting-coercion-initiative/design-discussions/upcast-safety-2.html

  • 👍9
dimpolo commented

I was playing around with this and discovered that the feature gate (and its incomplete_features warning) can be bypassed using #![feature(unsize)] instead.

This will upcast a &dyn Bar where Foo is a supertrait of Bar.

fn upcast<Dyn: ?Sized + Unsize<dyn Foo>>(bar: &Dyn) -> &dyn Foo { bar }

Playground

Would this mean the unsize feature should be incomplete as well?

  • 👍2
danielhenrymantilla commented

Came to comment the same observation that @dimpolo has made: I think the impl of Unsize<dyn '_ + Super> for dyn '_ + Sub ought, itself, to be feature-gated by trait_upcasting.

nikomatsakis commented

That makes sense, although really I just want to stabilize this.

  • 👍19
  • 🎉7
Mark-Simulacrum commented

cc #89460, which is tracking a future-compat lint that seems related to this feature.

pnkfelix commented

@joshtriplett can you confirm that when you added the S-waiting-on-team label, that was a reference to T-lang, and not T-compiler?

pnkfelix commented

@rustbot label: I-lang-nominated

I am seeking an answer to the question, requoted here:

@joshtriplett can you confirm that when you added the S-waiting-on-team label, that was a reference to T-lang, and not T-compiler?

nikomatsakis commented

This is blocked on lang decisions, and @crlf0710 has a stabilization PR ready to go

  • 🎉5
apiraino commented

Signaling the above comment with a S-blocked label

@rustbot label s-blocked

dtolnay commented

@rustbot label -S-blocked

Niko's comment was "this is blocked on lang decisions". This issue was already labelled S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. which captures this — "awaiting decision from the relevant subteam". S-blocked Status: Blocked on something else such as an RFC or other implementation work. means something different — "blocked on something else such as an RFC or other implementation work".

apiraino commented

@dtolnay it was just a convenience when I scan every week issues waiting on team to find the oldest ones to mention for the T-compiler meeting.

but it's fine, I won't bikeshed labels meaning :)

CAD97 commented

Something to consider before stabilization: while trait A: B is upcastable, should trait A where Self: B be?

I want to propose that perhaps where Self: B should not be considered an upcastable supertrait bound. That these two forms are equivalent is AIUI only apparent in one other location of the language -- that where Self: bounds are elaborated (on trait items), but all other where bounds are not.

TBQH I'm unsure one way or the other, but this should be explicitly decided, and I don't think it has been.

jeffs commented

Something to consider before stabilization: while trait A: B is upcastable, should trait A where Self: B be?

Isn't the former syntactic sugar for the latter? From The Rust Reference:

The following is an example of declaring Shape to be a supertrait of Circle.

trait Shape { fn area(&self) -> f64; }
trait Circle : Shape { fn radius(&self) -> f64; }

And the following is the same example, except using where clauses.

trait Shape { fn area(&self) -> f64; }
trait Circle where Self: Shape { fn radius(&self) -> f64; }

Any semantic difference between these syntaxes would be, at the least, surprising.

CAD97 commented

Isn't the former syntactic sugar for the latter?

Yes, that is currently the case. where Self: Bound is also special because it gets elaborated as a supertrait bound, unlike say where Self::Assoc: Bound which doesn't get elaborated.

I'm not convinced either way that where Self: Bound should or shouldn't offer upcasting; I think either are a valid choice, I just feel that the choice should be deliberately made rather than an artifact of the two forms currently being unified.

nikomatsakis commented

The RFC has been merged rust-lang/rfcs#3324. I updated this issue with unresolved questions:

  • Should we make upcasting opt-in in some form to limit vtable size by default? The current inclination of the lang-team is "no", but it would be useful to gather data on how much supporting upcasting contributors to overall binary size.
  • Before stabilizing it we should check that libs-api is ok with upcasting for all dyn-allowed traits in the library, since those we can't change.
  • Should we add an opt-out mechanism, and extend library stabilization checklist with "do we want to opt-out for now"?

To that end, it would be useful

  • if someone wanted to try and gather data about size overhead as a result of this change; you could compare the vtable behavior from before/after the improvements landed, or otherwise examine output.
  • to prepare a list of libcore traits that are affected
dhardy commented

🎉

My two cents: Rust code can have too many annotations. @nikomatsakis's rationale Why not make upcasting opt-in at a trait level? adds enough arguments to convince me that opt-ins are probably the wrong approach (primarily in that libraries don't know how their traits are used, though certainly in some cases dyn upcast usage is more likely than in others). A further argument is that opt-in/opt-out syntax is more to teach, where dyn upcast itself isn't. For much the same reasons (plus it being a breaking change) opt-outs also don't make sense.

Possible alternatives:

  • LTO
  • Compile-time configuration: (a) disable dyn upcast by default everywhere (including in dependencies) and (b) include a list of exceptions where dyn upcast is enabled (just like Cargo.lock this would require frequent updates — maintainers needing the optimisation would pay the cost, while eventually tooling may be able to generate the exception list).
  • Just make Rust users pay the (tiny) cost. Is this premature optimisation?
  • 👍1
nikomatsakis commented

@scottmcm raised the point in a previous meeting that we should review the set of traits that are affected. @crlf0710 was awesome enough to do an analysis, results available here. There will be 11 stable API traits that are now dyn upcastable, but only one of them has multiple supertraits and hence would be committed to the larger vtable:

  • Error <-- has two supertraits, so increases vtable size
  • BufRead
  • BorrowMut
  • PartialOrd
  • IndexMut
  • Fn
  • FnMut
  • DerefMut
  • DoubleSidedIterator
  • ExactSizeIterator
  • FusedIterator

In addition there are 6 unstable traits that become upcastable (purple background), but none of them have multiple supertraits:

  • std::str::pattern::ReverseSearcher
  • std::str::pattern::DoubleEndedSearcher
  • std::ops::OneSidedRange
  • std::iter::traits::marker::TrustedLen
  • std::simd::SimdPartialOrd
  • std::simd::SimdOrd

21 traits are now upcastable to a "Sealed" supertrait (orange background), this is a recurring pattern (e.g., std::io::IsTerminal).

@rustbot labels +I-libs-api-nominated

I am nominating for libs API. Please take a look at that list and nominated for lang if it raises any immediate concerns.

(Context: the existing vtable layout already supports upcasting. if we stabilize dyn upcasting, we'll be committed to that. Vtables become larger because they must contain a reference to the supertrait vtable, which is 1 word, plus the size of the supertrait vtable if that was not independently necessary. This is necessary even if no upcasting occurs because the upcasting may occur in a downstream crate that is not yet visible to us.)

bjorn3 commented

Error should be 12 pointers (4 Error methods + 1 Display method + 1 Debug method + 2 * 3 header fields) big I believe and with upcasting forbidden it would be 9 pointers (4 Error methods + 1 Display method + 1 Debug method + 1 * 3 header fields). I don't think this is an issue.

21 traits are now upcastable to a "Sealed" supertrait (orange background), this is a recurring pattern (e.g., std::io::IsTerminal).

std::sealed::Sealed doesn't have any methods. As such it doesn't add to the vtable size but instead has it's vtable overlap with the start of the parent vtable AFAIK. For example IsTerminal's vtable is size, align, drop_in_place, is_terminal, and upcasting to Sealed will reuse this exact same vtable, but ignore is_terminal.

  • 👍1
m-ou-se commented

@nikomatsakis We discussed your list in the libs-api meeting, and this looks fine to us.

However, we do think it might be good to also take into consideration traits in the ecosystem outside of the standard library.

QuineDot commented

I happened to notice that the trait_upcasting feature gate can be bypassed by the coerce_unsized feature gate.

#![feature(coerce_unsized)]
// No feature(trait_upcasting)

fn upcast_me<'a, T>(x: &'a T) -> &'a dyn Any
where
    T: ?Sized,
    &'a T: CoerceUnsized<&'a dyn Any>,
{
    x as &dyn Any
}

//...
let _ = upcast_me::<dyn Trait>(&());

Not sure if it's a problem per se, but I thought I'd mention it in case the leakage was accidental.

  • ❤️1
crlf0710 commented

This is known and not a problem (unless either unsize or coerced_unsized feature got stablized before this, which is unlikely).

  • 👍1
nikomatsakis commented

@crlf0710 I totally forgot about this, but now that RFC #3324 was merged, are we ready to stabilize?

  • ❤️4
WaffleLapkin commented

Although I would love to see this stabilized, I'm not sure if all concerns has been addressed, in particular:

  • Exponential (?) blowup in size (raised @ rust-lang/rfcs#3324 (comment))
  • Whatever we are fine with making creating *const dyn Tr with invalid metadata invalid, to support safe coercions (resolved by #101336 (comment))
  • Unresolved questions from this issue:
    • Should we make upcasting opt-in in some form to limit vtable size by default? The current inclination of the lang-team is "no", but it would be useful to gather data on how much supporting upcasting contributors to overall binary size.
    • Should we add an opt-out mechanism, and extend library stabilization checklist with "do we want to opt-out for now"?
  • Whatever there are concerns with traits in the ecosystem outside of the standard library
    (raised @ #65991 (comment))
  • While trait A: B is upcastable, should trait A where Self: B be? (raised @ #65991 (comment))

Some of those may have already been addressed/decided, but I could find where, if they even were. Anyway this is something the stabilization report / lang team will need to address.

  • 👍2
programmerjake commented

well, afaict *mut dyn Trait with invalid metadata is unsound when combined with arbitrary_self_types, so imho that means invalid metadata should be documented as invalid anyway:
https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=fba1253fe76cfc4a573d6499aa9b56d4
the following compiles with no errors and calls using the metadata:

#![feature(arbitrary_self_types)]
pub trait Tr {
    fn f(self: *mut Self);
}

pub fn g(v: *mut dyn Tr) {
    Tr::f(v)
}
  • 👍1
nikomatsakis commented

@WaffleLapkin thanks for the list!

On the topic of size, I am still hoping for data about the size impact from supporting upcasting. All the machinery is on stable and has been for some time. I feel like there have been a lot of comments citing theoretical concerns but few with actual measurements -- but perhaps I've forgotten.

Other folks on lang team may feel differently, but from my POV the size concerns are not a blocker for stabilization, more of a motivation tool to create better tooling or other mechanisms to reduce binary size for those contexts where it matters (e.g., it's already fairly common practice to strip some kinds of data in some contexts).

  • 👍3
crlf0710 commented

@WaffleLapkin thanks for the list! Just a note that the second bullet has been resolved via the fcp in #101336 .

  • 👍1
WaffleLapkin commented

On the topic of size, I am still hoping for data about the size impact from supporting upcasting.

I think it should be possible to add a debug flag to the compiler, that prints VTable sizes with/without supporting upcasting and then run it on a set of crates somehow. I'll try to work in this direction.

@crlf0710 I've updated the list, thanks

  • 👍1
nikomatsakis commented

@WaffleLapkin I like that idea! But I think we need a volunteer to do it.

WaffleLapkin commented

@nikomatsakis I have ideas how to make that work, this is near the top of my todo list =)

nikomatsakis commented

@WaffleLapkin great! My take is that I am willing to wait for people to gather data, but I'm not willing to block if nobody is signed up saying they will do it.

  • ❤️4
nikomatsakis commented

@rfcbot concern gathering data

I opened this issue #112355 to track @WaffleLapkin's work on gathering data.

WaffleLapkin commented

While working on #112355 I think I found a bug. Given this program:

#![feature(rustc_attrs)]

#[rustc_dump_vtable]
pub trait What: Send + Sync {
    fn a(&self) {}
}

impl What for () {}

fn main() {
    (&() as &dyn What).a();
}

rustc outputs the following vtable layout:

 [
    MetadataDropInPlace,
    MetadataSize,
    MetadataAlign,
    TraitVPtr(<() as Sync>),
    Method(<() as What>::a),
]

Which is surprising, because TraitVPtr(<() as Sync>) is unnecessary — Send does not have any methods, so Sync vtable can be inlined...

I think we should be smarter here:

rust/compiler/rustc_trait_selection/src/traits/vtable.rs

Lines 143 to 144 in cb882fa

// Other than the left-most path, vptr should be emitted for each trait.
emit_vptr_on_new_entry = true;

(for the record, I'm pretty sure this happens in practice, I've found this with log::Log)

nikomatsakis commented

Discussed in the rust-lang/lang meeting. To all those listening, @WaffleLapkin has prepared a flag that can gather data on the size of vtables, but nobody has tried it. We encourage everyone listening on this issue who has concerns to try this out on their project and post results in #112355 (instructions can be found in that issue). We are going to revisit this next week and if we don't see any data, or if the data we see doesn't concern us, we'll begin moving forward with FCP.

On a related note, I filed #113840 for the potential bug you found, @WaffleLapkin.

  • 👍2
  • ❤️10
WaffleLapkin commented

While experimenting with vtables I also discovered #114007 (not necessarily important for this, but still potentially interesting)

  • 👍2
  • ❤️2
nikomatsakis commented

@rfcbot resolve gathering data

Based on what we see in #112355, I feel comfortable moving forward now (and potentially doing follow up work later).

  • 🎉8
  • ❤️3
pnkfelix commented

Just wanted to quickly mention: I noticed this paper, "Tighten Rust's Belt: Shrinking Embedded Rust Binaries", recently that explicitly calls out vtable sizes as a concern for minimizing the size of Rust binaries: https://sing.stanford.edu/site/publications/rust-lctes22.pdf

(That paper is discussing a different problem, namely that vtables include destructor info, in order to handle dropping Box<dyn Trait>, and the vtables include this data even for targets that have neither heap nor Box. Much like the proposal for dyn upcasting, I believe we could deal with this by adding a way for programs to opt-out of providing such vtables.)

pnkfelix commented

Temporarily removing T-compiler label to do a T-lang focused FCP.

@rustbot label: -T-compiler

pnkfelix commented

Following up on: #65991 (comment)

After a few consecutive weeks discussing of the data presented in #112355, T-lang believes this is ready to stabilize.

2023-08-01 hackmd

2023-08-08 hackmd

The basic conclusion is that we think the cost of this will be limited. Namely, sufficiently limited that we can, if necessary, invest in an opt-out mechanism at some future date; we do not need to block stabilization today on the addition of a hypothetical opt-in mechanism, because we think migrating to an opt-in mechanism will be overly disruptive to Rust developers.

@rfcbot fcp merge

rfcbot commented

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

  • 🎉3
pnkfelix commented

(readding T-compiler label now that I've started FCP merge.)

@rustbot label: +T-compiler

also, cc @rust-lang/compiler , if you have any concerns about this feature, make sure you get them reflected by a representative on T-lang.

  • 👍2
lcnr commented

also, cc @rust-lang/types this feature needs pretty much the most involved builtin impls we have. Unsize<dyn SuperTrait>: dyn Trait is non-trivial , see e.g. #114036. I still think this feature is very much desirable and this is a cost we are willing to pay.

lcnr commented

To add to the above point, I would like us1 to increase our requirements for stabilizing new features

#114036 dealt with an issue which is both quite subtle - the original approach in that PR added new ways to exploit #57893 - and the general issue of "projection bounds on trait objects have to be handled in some way", was afaict never mentioned anywhere during the stabilization process.

I have some ideas here, like requiring the stabilization report to again fully describe everything that is stabilized, referencing the implementation. I think starting the FCP without any summary of what is stabilized, or stating "implements RFC X", is bound to cause bigger issues going forward. But these ideas are my own and might not necessarily work out too well in practice. The types team will have an in-person meetup this year where I will try to talk about this and I am also interested in the opinions of others here via zulip or in-person.

Footnotes

  1. mostly t-types as that is where I am most involved, but also more generally t-compiler/t-lang etc ↩

  • 👍5
  • ❤️3
crlf0710 commented

I originally posted a stablization report at #101718 (comment) , but let me paste it here for visibility:

crlf0710 commented

<Initial draft of the stablization report here>

EDIT: I have moved it into a hackmd document at https://hackmd.io/@VzIJrhqJTM-xle1nEBAysw/S18EtXXn2/edit

  • 👍1
  • 🎉2
lcnr commented

Sorry for using this feature for the far more general topic of "I have issues with our stabilization process". My issues are not limited to this stabilization. They are not "mistakes" of whoever is pushing towards stabilization, but much rather issues with our existing process and expectations.

Using this stabilization report as an example of what I would ideally expect of features like this, based on my current - somewhat limited - knowledge about this feature:

  • there seems to be a finished FCP on a not yet closed issue #101336. I would expect the decision made in that FCP to be referenced directly in the stabilization report with a short summary of its impact. Should this issue be closed before finishing the stabilization?
  • afaik there has been significant discussion about the size of the vtables. T-lang decided that this size cost was acceptable: #65991 (comment). However, this decision and its rationale are pretty spread out and hard to follow. I would have liked a self-contained summary for that: linking to the collected data and summarizing the arguments used when making the decision
  • afaict enabling this feature is a breaking change for Deref impls from Trait to SuperTrait. There is a future compat lint for impls like this in #89460. I would like to see mention of this breakage and its impact (e.g. linking to crater run)
  • "We are stablizing a new kind of coercion", an explanation of these coercion rules in detail. This should have ideally caught the issues fixed by #114036. This should have made it apparent that we currently don't support dyn Trait + Send to dyn Send upcasting, cc #114679.

Some of these additional requirements may require in-depth knowledge in many different areas, more than what can be expected from a single contributor. E.g. to accurately describe the coercion rules you probably need a @rust-lang/types member to participate in the stabilization report. However, such expertise is required anyways to evaluate the stabilization. It feels a lot better to me to potentially delay the stabilization because of this, rather than hoping that an expert notices the stabilization and that - and how - it impacts their area of expertise.

  • 👍9
RalfJung commented

Agreed with @lcnr. For major features like this, I think it would be very reasonable for several people to collaborate on a stabilization report e.g. on hackmd, and spend some time going back and forth with different teams until everyone is happy with it. (I've recently done this for a t-opsem FCP and found it a very pleasant experience.)

there seems to be a finished FCP on a not yet closed issue #101336. I would expect the decision made in that FCP to be referenced directly in the stabilization report with a short summary of its impact. Should this issue be closed before finishing the stabilization?

Indeed I was about to mention, this commits us to safe dyn upcasting on raw pointers. In other words, the safety invariant for raw dyn pointers requires a valid vtable, and creating a dangling raw dyn pointer will forever be an unsafe operation. (The validity invariant is undecided still, that would determine whether creating a dangling raw dyn pointer is possible at all or whether a vtable is required at any time. This is tracked at rust-lang/unsafe-code-guidelines#166.)

I think this is a reasonable choice, but it is definitely worth calling out.

  • ❤️2
RalfJung commented

Uh, is it expected that this code using upcasting works on nightly without any feature gates...? It actually builds on stable. 😨

EDIT: Oh, I guess this doesn't upcast. 🤔 it... turns the Box itself into an Ayn, or so? Sorry for the scare.

compiler-errors commented

@RalfJung: That's not upcasting. That's unsizing Box<dyn FileDescriptor> to dyn Any.

  • 👍1
scottmcm commented

@rfcbot concern ensure-we-discuss-lcnr's-thoughts-in-triage

pnkfelix commented

Discussed in T-lang meeting. We believe this is still S-waiting-on-team, its just waiting on T-types instead of T-lang.

@rustbot label: +I-types-nominated +T-types

scottmcm commented

This was discussed in triage, so I'll remove the concern
@rfcbot resolve ensure-we-discuss-lcnr's-thoughts-in-triage

apiraino commented

Were all concerns raised by @WaffleLapkin in this comment ticked/discussed?

(apologies in advance if I missed something, I am slightly struggling to follow the technical details of the discussion)

tmandry commented

@apiraino I believe those should be addressed in some form in the stabilization report by the time it's published. I think T-types is now driving this to completion, but a number of other people have contributed to the draft (https://hackmd.io/QggP6SJVTa2jb7DjGq0vrQ?both).

  • 👍1
nikomatsakis commented

Hi folks, we talked about this in our @rust-lang/types meeting. We all agreed that we'd like to see a better stabilization report that covers the trait system implementation and, ideally, modeling for trait upcasts in a-mir-formality.

I'm up to mentor someone on the latter if anybody has time to commit, please reach out.

EDIT: We decided a write-up suffices, see below.

nikomatsakis commented

We talked about this in our rust-lang/types meetup. While I still very much want to see this covered in a-mir-formality, we agreed that we shouldn't block on that. We would be happy to move forward with an improved stabilization report that covered more technical details, included links to the various writeups and decisions we've made. The major piece of missing docs is covering the interaction with trait selection -- @compiler-errors has recently reimplemented this and indicated a willingness to write it up (perhaps for rustc-dev-guide). I believe that the rest is already written and we can compile a stabilization report that links to the various places.

  • ❤️4
  • 🚀15
nikomatsakis commented

@rfcbot reviewed

nikomatsakis commented

@rfcbot concern types-team-sign-off

I am adding a concern that @rust-lang/types wants to be happy with the impl details (and especially @lcnr) -- @compiler-errors covered the details of type system interactions in rust-lang/rustc-dev-guide#1817 though.

lcnr commented

I am happy with the impl details. I had some other concerns in #65991 (comment).

While #101336 has still not been closed, everything seems now documented in https://hackmd.io/QggP6SJVTa2jb7DjGq0vrQ, so 👍 from my end.

nikomatsakis commented

@rfcbot resolve types-team-sign-off

see add'l discussion here

traviscross commented

@rustbot labels +I-lang-nominated

This was discussed in a recent T-types meeting. As @lcnr stated above, the concerns about the stabilization report and documentation have been resolved, so this is now unblocked. @nikomatsakis has proposed that we re-highlight this issue for T-lang, so let's renominate.

  • 🎉4
GregoryConrad commented

Not sure this has been mentioned yet, but seems related: #32220 should be possible now that Rust has upcasting support (at least in nightly)?

Edit: it looks like yes, see rust-lang/rfcs#2035

nikomatsakis commented

We discussed @WaffleLapkin's list of unresolved concerns in the rust-lang triage meeting and had this text which I will add to the stabilization report:

  • Exponential (?) blowup in size (raised @ rust-lang/rfcs#3324 (comment))
    • Conclusion: We put out a request for data in #112355. The data gathered did not suggest major impact (e.g., nbdd0121 found <0.2% overhead in the size of text sections). Furthermore, the vtables in practice have included upcast support for a long time and so this is not making anything larger.
  • Whatever we are fine with making creating *const dyn Tr with invalid metadata invalid, to support safe coercions (resolved by #101336 (comment))
  • Should we make upcasting opt-in in some form to limit vtable size by default? The current inclination of the lang-team is "no", but it would be useful to gather data on how much supporting upcasting contributors to overall binary size.
    • Answer: no. This would create complexity for all Rust users with very little practical benefit (size impact, as measured above, is very little).
  • Should we add an opt-out mechanism, and extend library stabilization checklist with "do we want to opt-out for now"?
    • Answer: we are not adding a mechanism but we can do it later. We wouldn't be able to apply it to the standard library traits but we did an analysis (in the stabilization report) and found that we would not want that anyway.
  • Whatever there are concerns with traits in the ecosystem outside of the standard library
    (raised @ #65991 (comment))
    • Answer: No, based on our data, we are not concetrned.
  • While trait A: B is upcastable, should trait A where Self: B be? (raised @ #65991 (comment))
    • Answer: Yes, and it is, because all other places in Rust treat trait A: B as equivalent to trait A where Self: B.
  • 👍2
  • ❤️2
tmandry commented

@rfcbot reviewed

rfcbot commented

🔔 This is now entering its final comment period, as per the review above. 🔔

  • 🚀10
NobodyXu commented

If the upcasting feature isn't used, can it be optimized out by compiler?

WaffleLapkin commented

@NobodyXu if a type is not ever coerced to a trait object, then the compiler won't generate a vtable. Otherwise, if a type is coerced, then the compiler will not be able to change the layout of the vtable based on it's usage.

NobodyXu commented

Thanks, it makes sense since it's hard to do that.

  • 👍1
traviscross commented

@rustbot labels -I-lang-nominated

The proposal to do this is in FCP. Let's remove the nomination until and unless there is something more to do here.

Mergulsevdova commented

This is a tracking issue for RFC3324. Corresponding MCP is here.

The feature gate for the issue is #![feature(trait_upcasting)].

STATUS UPDATE

We are in the process of stabilizing.

About tracking issues

Tracking issues are used to record the overall progress of implementation.

They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.

A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.

Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Previous discussions

Unresolved Questions

  • Currently CoerceUnsized trait cannot express this case: a smart pointers that wrap a raw pointer and don't guarantee via a custom invariant that it is valid. Maybe a separate CoerceUnsizedUnsafe trait is needed. (see #88010 (comment)).

  • Should we make upcasting opt-in in some form to limit vtable size by default? The current inclination of the lang-team is "no", but it would be useful to gather data on how much supporting upcasting contributors to overall binary size.

  • Before stabilizing it we should check that libs-api is ok with upcasting for all dyn-allowed traits in the library, since those we can't change. (addressed by #65991 (comment))

  • Should we add an opt-out mechanism, and extend library stabilization checklist with "do we want to opt-out for now"?

Implementation history

rfcbot commented

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

  • 🎉35
Ekleog commented

I just tried using this feature to upcast to Any + Send + Sync, and it seems to me like this is currently not allowed:

trait DynSized: 'static + Any + Send + Sync + DeepSizeOf {}
impl<T: 'static + Any + Send + Sync + DeepSizeOf> DynSized for T {}

fn main() {
    let ptr = Arc::new(()) as Arc<dyn DynSized>;
    let ptr_any = ptr as Arc<dyn Any + Send + Sync>;
    // my goal:
    let res = Arc::<dyn Any + Send + Sync>::downcast::<()>(ptr);
}

(playground link)

With references, the situation does not seem better than with Arc: this does not compile either

fn main() {
    let ptr = &() as &'static dyn DynSized;
    let ptr_any = ptr as &'static (dyn Any + Send + Sync);
}

(playground link)

Is this expected? I can't find any reference to this limitation, neither explicitly in the RFC text nor in this tracking issue. The RFC text does mention not supporting upcasting to multiple traits, but I'd have expected lifetimes as well as auto traits to not be included in the limitation.

If it is expected that this feature is stabilized without this support, does anyone know where I should watch, to know of the news on that front?

  • 👍1
GregoryConrad commented

@Ekleog you can probably do a workaround like:

trait AnySendSync: Any + Send + Sync {}
impl<T: Any + Send + Sync> AnySendSync for T{}

Then upcast to AnySendSync instead of Any + Send + Sync. On my phone though so can’t verify if this actually works

Edit: you’ll also likely have to change your DynSized to be DynSized: AnySendSync + DeepSizeOf 👍

  • 👍1
Ekleog commented

Unfortunately, it looks like [I still cannot downcast, because I need to upcast Arc<dyn AnySendSync> to Arc<dyn Any + Send + Sync> in order to be able to downcast, and this still does not compile :/

  • 😕1
CAD97 commented

It's certainly not ideal due to requiring unsafe, but a downcast_ref to check followed by a Arc::from_raw(arc.as_raw().cast()) would at least be be a sound workaround.

yshui commented

@Ekleog how about this? this has an extra T: Sized restriction though, don't know if it can be better.

use std::any::Any;
use std::sync::Arc;

trait DeepSizeOf {} // from deepsize
impl DeepSizeOf for () {}

trait AnySendSync: Any + Send + Sync {
    fn explode(self: Arc<Self>) -> Arc<dyn Any + Send + Sync>;
}
impl<T: Any + Send + Sync> AnySendSync for T {
    fn explode(self: Arc<T>) -> Arc<dyn Any + Send + Sync> {
        self
    }
}
trait DynSized: 'static + AnySendSync + DeepSizeOf {}
impl<T: 'static + AnySendSync + DeepSizeOf> DynSized for T {}

fn main() {
    let ptr = Arc::new(()) as Arc<dyn DynSized>;
    let ptr_anysendsync = ptr as Arc<dyn AnySendSync>; // works fine
    let ptr_any = ptr_anysendsync.explode();
    let res = ptr_any.downcast::<()>();
}

i guess upcasting coercion exists to solve this kind of inefficiencies... but since upcasting to a group of traits isn't possible right now.

  • 👍1
RalfJung commented

Closed issues are not a good place for discussion, please open a new issue or discuss on Zulip or the forums.

upcasting to a group of traits isn't possible right now

Indeed that's the core here, you can only upcast to a single trait. Could be interesting to discuss extending this to "one trait plus any number of marker traits" but that's an extension which needs a new issue. :)

  • 👍1
Ekleog commented

Let's continue discussion on my upcasting issue on this separate issue opened by @compiler-errors :) Incidentally, it is currently being solved in this PR also by @compiler-errors (lots of thanks ❤️)

@dhardy As for the downcasting you're mentioning, I'm not sure what you're thinking of? But if/when you open a separate issue to discuss it, please tag me!

  • 🎉1
steffahn commented

I’ve come across some soundness issues with upcasting today. Readers of this thread might be interested ^^

  • 👀1
PoignardAzur commented

Can we have a status update with regards to the reverted stabilization? The current status update says "We are in the process of stabilizing" since January, with no mention of the recent soundness issues.

  • 👍5
WaffleLapkin commented

The stabilization is currently blocked on the fix of the unsoundness bug tracked in #120222. The fix is in #120248, but needs a bit more work. I've been occupied by the never type stuff which is urgent because some of the changes are edition based. I hope I'll have time soon to finish #120248.

This is also blocked on rust reference PR, which someone needs to take over: rust-lang/reference#1259 (comment).

  • 👍3
  • ❤️7
Kish29 commented

It looks like the blocking bug #120222 have been fixed out, and what's next for this stabilization?

  • 👍2
WaffleLapkin commented

@Kish29 the next steps are changing the reference (cc rust-lang/reference#1259) and doing a new stabilization PR :)

  • 👍3
  • 🎉13
noclue commented

I am newbie baffled by the absence of this basic syntax. I got this workaround if anyone is interested. It works all on stable Rust

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0d53066544591aadc71e22bc17f0bb2a

  • 👍1