-
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
Manually Drop #1860
Manually Drop #1860
Changes from 1 commit
9f0172f
b9674db
8927082
8ce4449
c19baf7
511fdb5
79a466c
66e9f74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
- Feature Name: manually_drop | ||
- Start Date: 2017-01-20 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Include the `ManuallyDrop` wrapper in `core::mem`. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Currently Rust does not specify the order in which the destructors are run. Furthermore, this order | ||
differs depending on context. RFC issue [#744](https://github.com/rust-lang/rfcs/issues/744) | ||
exposed the fact that the current, but unspecified behaviour is relied onto for code validity and | ||
that there’s at least a few instances of such code in the wild. | ||
|
||
While a move to stabilise and document the order of destructor evaluation would technically fix the | ||
problem describe above, there’s another important aspect to consider here – implicitness. Consider | ||
such code: | ||
|
||
```rust | ||
struct FruitBox { | ||
peach: Peach, | ||
banana: Banana, | ||
} | ||
``` | ||
|
||
Does this structure depend on `Peach`’s destructor being run before `Banana` for correctness? | ||
Perhaps its the other way around and it is `Banana`’s destructor that has to run first? In the | ||
common case structures do not have any such dependencies between fields, and therefore it is easy | ||
to overlook such a dependency while changing the code above to the snippet below (e.g. so the | ||
fields are sorted by name). | ||
|
||
```rust | ||
struct FruitBox { | ||
banana: Banana, | ||
peach: Peach, | ||
} | ||
``` | ||
|
||
For structures with dependencies between fields it is worthwhile to have ability to explicitly | ||
annotate the dependencies somehow. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
This RFC proposes adding following `struct` to the `core::mem` (and by extension the `std::mem`) | ||
module. `mem` module is a most suitable place for such `struct`, as the module already a place for | ||
functions very similar in purpose: `drop` and `forget`. | ||
|
||
```rust | ||
/// Inhibits compiler from automatically calling `T`’s destructor. | ||
#[unstable(feature = "manually_drop", reason = "recently added", issue = "0")] | ||
#[allow(missing_debug_implementations, unions_with_drop_fields)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should have |
||
pub union ManuallyDrop<T>{ value: T } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You write adding following |
||
|
||
impl<T> ManuallyDrop<T> { | ||
/// Wraps a value to be manually dropped. | ||
#[unstable(feature = "manually_drop", reason = "recently added", issue = "0")] | ||
pub fn new(value: T) -> ManuallyDrop<T> { | ||
ManuallyDrop { value: value } | ||
} | ||
|
||
/// Extracts the value from the ManuallyDrop container. | ||
#[unstable(feature = "manually_drop", reason = "recently added", issue = "0")] | ||
pub fn into_inner(self) -> T { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a static method like |
||
unsafe { | ||
self.value | ||
} | ||
} | ||
|
||
/// Extracts the value from the ManuallyDrop container. | ||
/// Could also be implemented as Deref. | ||
#[unstable(feature = "manually_drop", reason = "recently added", issue = "0")] | ||
pub fn as_ref(&self) -> &T { | ||
unsafe { | ||
&self.value | ||
} | ||
} | ||
|
||
/// Extracts the value from the ManuallyDrop container. | ||
/// Could also be implemented as a DerefMut. | ||
#[unstable(feature = "manually_drop", reason = "recently added", issue = "0")] | ||
pub fn as_mut(&mut self) -> &mut T { | ||
unsafe { | ||
&mut self.value | ||
} | ||
} | ||
|
||
/// Manually drops the contained value. | ||
/// | ||
/// # Unsafety | ||
/// | ||
/// This function runs the destructor of the contained value and thus makes any further action | ||
/// with the value within invalid. The fact that this function does not consume the wrapper | ||
/// does not statically prevent further reuse. | ||
#[unstable(feature = "manually_drop", reason = "recently added", issue = "0")] | ||
pub unsafe fn manually_drop(&mut self) { | ||
ptr::drop_in_place(&mut self.value) | ||
} | ||
} | ||
``` | ||
|
||
Let us apply this structure to the example from the motivation: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace this structure with this union |
||
|
||
```rust | ||
struct FruitBox { | ||
// Immediately clear there’s something non-trivial going on with these fields. | ||
peach: ManuallyDrop<Peach>, | ||
banana: ManuallyDrop<Banana>, | ||
} | ||
|
||
impl Drop for FruitBox { | ||
fn drop(&mut self) { | ||
unsafe { | ||
// Explicit ordering in which field destructors are run specified in the intuitive | ||
// location – the destructor of the structure containing the fields. | ||
// Moreover, one can now reorder fields within the struct however much they want. | ||
peach.manually_drop(); | ||
banana.manually_drop(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be updated to |
||
} | ||
} | ||
} | ||
``` | ||
|
||
It is proposed that such code would become idiomatic for structures where fields must be dropped in | ||
a particular order. | ||
|
||
# How We Teach This | ||
[how-we-teach-this]: #how-we-teach-this | ||
|
||
It is expected that the functions and wrapper added as a result of this RFC would be seldom | ||
necessary. | ||
|
||
In addition to the usual API documentation, this structure should be mentioned in | ||
reference/nomicon/elsewhere where drop ordering guarantees are described as a way to explicitly | ||
control the order in which the structure fields gets dropped. | ||
|
||
<!-- | ||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
No drawbacks known at the time. | ||
--> | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
* Stabilise some sort of drop order and make people to write code that’s hard to figure out at a | ||
glance; | ||
* Bikeshed colour; | ||
* Stabilise union and let people implement this themselves: | ||
* Precludes (or makes it much harder) from recommending this pattern as the idiomatic way to | ||
implement destructors with dependencies. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
* Best way to expose value inside the wrapper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd to call this a
struct
. Perhaps "type" instead?