Tracking Issue for Interoperability With C++ Destruction Order #100344
Assignees
Labels
C-tracking-issue
Category: An issue tracking the progress of sth. like the implementation of an RFC
S-tracking-impl-incomplete
Status: The implementation is incomplete.
T-lang
Relevant to the language team, which will review and decide on the PR/issue.
Comments
pnkfelix
added
T-lang
joshtriplett
added
the
S-tracking-impl-incomplete
|
Assigning to @cramertj as the current lang team liaison. |
matthiaskrgr
pushed a commit
to matthiaskrgr/rust
that referenced
this issue
Nov 16, 2022
This attribute is equivalent to `#[lang = "manually_drop"]`, and the behavior of both are extended to allow the type to define `Drop`, in which case, that will still be run. Only the field drop glue is suppressed. This PR should essentially fully implement rust-lang#100344. Some additional notes: `#[manually_drop]` means "do not destroy the fields automatically during destruction". `ManuallyDrop`, is (or could be) "just" `#[manually_drop] struct ManuallyDrop<T>(T);`. The intent is, per the MCP, to allow customization of the drop order, without making the interface for initializing or accessing the fields of the struct clumsier than a normal Rust type. It also -- and people did seem excited about this part -- lifts `ManuallyDrop` from being a special language supported feature to just another user of `#[manually_drop]`. There is the question of how this interacts with "insignificant" drop. I had trouble understanding the comments, but insignificant drop appears to just be a static analysis tool, and not something that changes behavior. (For example, it's used to detect if a language change will reorder drops in a meaningful way -- meaning, reorder the significant drops, not the insignificant ones.) Since it's unlikely to be used for `#[manually_drop]` types, I don't think it matters a whole lot. And where a destructor is defined, it would seem to make sense for `#[manually_drop]` types to match exactly the behavior of `union`, since they both have the shared property that field drop glue is suppressed. I looked for all locations that queried for `is_manually_drop` in any form, and found two difficult cases which are hardcoded for `ManuallyDrop` in particular. The first is a clippy lint for redundant clones. I don't understand why it special-cases `ManuallyDrop`, and it's almost certainly wrong for it to special-case `#[manually_drop]` types in general. However, without understanding the original rationale, I have trouble figuring the right fix. Perhaps it should simply be deleted altogether. The second is unions -- specifically, the logic for disabling `DerefMut`. `my_union.x.y = z` will automatically dereference `my_union.x` if it implements `DerefMut`, unless it is `ManuallyDrop`, in which case it will not. This is because `ManuallyDrop` is a pointer back to its content, and so this will automatically call `drop` on a probably uninitialized field, and is unsafe. This is true of `ManuallyDrop`, but not necessarily any other `#[manually_drop]` type. I believe the correct fix would, instead, be a way to mark and detect types which are a smart pointer whose pointee is within `self`. See, for example, this playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=76fb22a6214ce453538fc18ec35a839d But that needs to wait for a separate RFC. For now, we apply exactly the same restriction for `ManuallyDrop` and for any other `#[manually_drop]` type, even though it may be confusing. 1. Delete `#[lang = "manually_drop"]`. I'm not sure if anything special needs to be done here other than replacing it with `#[manually_drop]` -- is there a compatibility guarantee that must be upheld? 2. (Optional) Fix the redundant clone check to correctly handle `#[manually_drop]` structs that aren't `ManuallyDrop`. 3. When there is more experience with the feature (e.g. in Crubit) create a full RFC based on the MCP, and go through the remainder of the stabilization process. (Also, do things like generalize the union error messages to not specifically mention `ManuallyDrop`, but also mention `#[manually_drop]`. For as long as the feature is unstable, the error messages would be confusing if they referenced it...)
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Nov 16, 2022
Add the `#[manually_drop]` attribute. This attribute is equivalent to `#[lang = "manually_drop"]`, and the behavior of both are extended to allow the type to define `Drop`, in which case, that will still be run. Only the field drop glue is suppressed. This PR should essentially fully implement rust-lang#100344, and is gated behind the feature flag `manually_drop_attr`. Some additional notes: ### The meaning of `#[manually_drop]` `#[manually_drop]` means "do not destroy the fields automatically during destruction". `ManuallyDrop`, could "just", morally speaking, be `#[manually_drop] struct ManuallyDrop<T>(T);`. The intent is, per the MCP, to allow customization of the drop order, without making the interface for initializing or accessing the fields of the struct clumsier than a normal Rust type. It also -- and people did seem excited about this part -- lifts `ManuallyDrop` from being a special language supported feature to just another user of `#[manually_drop]`. ### Insignificant Drop There is the question of how this interacts with "insignificant" drop. I had trouble understanding the comments, but insignificant drop appears to just be a static analysis tool, and not something that changes behavior. (For example, it's used to detect if a language change will reorder drops in a meaningful way -- meaning, reorder the significant drops, not the insignificant ones.) Since it's unlikely to be used for `#[manually_drop]` types, I don't think it matters a whole lot. And where a destructor is defined, it would seem to make sense for `#[manually_drop]` types to match exactly the behavior of `union`, since they both have the shared property that field drop glue is suppressed. ### Users that expect `adt.is_manually_drop()` <-> "is `std::mem::ManuallyDrop`" I looked for all locations that queried for `is_manually_drop` in any form, and found two difficult cases which are hardcoded for `ManuallyDrop` in particular. The first is a clippy lint for redundant clones. I don't understand why it special-cases `ManuallyDrop`, and it's almost certainly wrong for it to special-case `#[manually_drop]` types in general. However, without understanding the original rationale, I have trouble figuring the right fix. Perhaps it should simply be deleted altogether. The second is unions -- specifically, the logic for disabling `DerefMut`. `my_union.x.y = z` will automatically dereference `my_union.x` if it implements `DerefMut`, unless it is `ManuallyDrop`, in which case it will not. This is because `ManuallyDrop` is a pointer back to its content, and so this will automatically call `drop` on a probably uninitialized field, and is unsafe. This is true of `ManuallyDrop`, but not necessarily any other `#[manually_drop]` type. I believe the correct fix would, instead, be a way to mark and detect types which are a smart pointer whose pointee is within `self`. See, for example, this playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=76fb22a6214ce453538fc18ec35a839d But that needs to wait for a separate RFC. For now, we apply exactly the same restriction for `ManuallyDrop` and for any other `#[manually_drop]` type, even though it may be confusing. ## To-do in future PRs 1. Delete `#[lang = "manually_drop"]`. I'm not sure if anything special needs to be done here other than replacing it with `#[manually_drop]` -- is there a compatibility guarantee that must be upheld? 2. (Optional) Fix the redundant clone check to correctly handle `#[manually_drop]` structs that aren't `ManuallyDrop`. 3. When there is more experience with the feature (e.g. in Crubit) create a full RFC based on the MCP, and go through the remainder of the stabilization process. (Also, do things like generalize the union error messages to not specifically mention `ManuallyDrop`, but also mention `#[manually_drop]`. For as long as the feature is unstable, the error messages would be confusing if they referenced it...)
|
Hmm, is this the best resource for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a tracking issue for the MCP "Interoperability With C++ Destruction Order" (rust-lang/lang-team#135).
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
instructions?)
Unresolved Questions
drop_order(from_end)anddrop_order(from_start)) or if even more flexible mechanisms (manually list out order for each field) is warranted here.#[manually_drop]suppress destructuring?Implementation history
The text was updated successfully, but these errors were encountered: