search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.35k stars 12.59k forks source link

Tracking issue for dyn upcasting coercion #65991

Open alexreg opened 4 years ago

alexreg commented 4 years ago

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

Implementation history

nikomatsakis commented 2 years ago

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

dimpolo commented 2 years ago

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?

danielhenrymantilla commented 2 years ago

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 2 years ago

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

Mark-Simulacrum commented 2 years ago

cc https://github.com/rust-lang/rust/issues/89460, which is tracking a future-compat lint that seems related to this feature.

pnkfelix commented 2 years ago

@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 2 years ago

@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 2 years ago

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

apiraino commented 2 years ago

Signaling the above comment with a S-blocked label

@rustbot label s-blocked

dtolnay commented 2 years ago

@rustbot label -S-blocked

Niko's comment was "this is blocked on lang decisions". This issue was already labelled https://github.com/rust-lang/rust/labels/S-waiting-on-team https://github.com/rust-lang/rust/labels/T-lang which captures this — "awaiting decision from the relevant subteam". https://github.com/rust-lang/rust/labels/S-blocked means something different — "blocked on something else such as an RFC or other implementation work".

apiraino commented 2 years ago

@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 1 year ago

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 1 year ago

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 1 year ago

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 1 year ago

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

To that end, it would be useful

dhardy commented 1 year ago

:tada:

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:

nikomatsakis commented 1 year ago

@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:

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

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 1 year ago

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.

m-ou-se commented 1 year ago

@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 1 year ago

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.

crlf0710 commented 1 year ago

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

nikomatsakis commented 1 year ago

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

WaffleLapkin commented 1 year ago

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

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.

programmerjake commented 1 year ago

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)
}
nikomatsakis commented 1 year ago

@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).

crlf0710 commented 1 year ago

@WaffleLapkin thanks for the list! Just a note that the second bullet has been resolved via the fcp in https://github.com/rust-lang/rust/issues/101336 .

WaffleLapkin commented 1 year ago

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

nikomatsakis commented 1 year ago

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

WaffleLapkin commented 1 year ago

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

nikomatsakis commented 1 year ago

@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.

nikomatsakis commented 1 year ago

@rfcbot concern gathering data

I opened this issue https://github.com/rust-lang/rust/issues/112355 to track @WaffleLapkin's work on gathering data.

WaffleLapkin commented 1 year ago

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: https://github.com/rust-lang/rust/blob/cb882fa998571e8a7ec1c06bb5d9dd9bc3423629/compiler/rustc_trait_selection/src/traits/vtable.rs#L143-L144

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

nikomatsakis commented 1 year ago

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.

WaffleLapkin commented 1 year ago

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

nikomatsakis commented 1 year ago

@rfcbot resolve gathering data

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

pnkfelix commented 1 year ago

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 1 year ago

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

@rustbot label: -T-compiler

pnkfelix commented 1 year ago

Following up on: https://github.com/rust-lang/rust/issues/65991#issuecomment-1640732308

After a few consecutive weeks discussing of the data presented in https://github.com/rust-lang/rust/issues/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 1 year ago

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.

pnkfelix commented 1 year ago

(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.

lcnr commented 1 year ago

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. https://github.com/rust-lang/rust/pull/114036. I still think this feature is very much desirable and this is a cost we are willing to pay.

lcnr commented 1 year ago

To add to the above point, I would like us[^1] 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.

[^1]: mostly t-types as that is where I am most involved, but also more generally t-compiler/t-lang etc

crlf0710 commented 1 year ago

I originally posted a stablization report at https://github.com/rust-lang/rust/pull/101718#issuecomment-1244925635 , but let me paste it here for visibility:

crlf0710 commented 1 year ago

\<Initial draft of the stablization report here>

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

lcnr commented 1 year ago

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:

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.

RalfJung commented 1 year ago

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 https://github.com/rust-lang/rust/issues/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 https://github.com/rust-lang/unsafe-code-guidelines/issues/166.)

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

RalfJung commented 1 year ago

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

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

compiler-errors commented 1 year ago

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

scottmcm commented 1 year ago

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

pnkfelix commented 1 year ago

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