Implement Manually Drop #40559

nagisa · 2017-03-15T22:11:51Z

As the RFC has been from approx a week in FCP without any major comments, I’m taking the opportunity to submit the PR early.

rust-highfive · 2017-03-15T22:12:01Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2017-03-16T05:44:47Z

src/libcore/mem.rs

+impl<T: ::fmt::Debug> ::fmt::Debug for ManuallyDrop<T> {
+    fn fmt(&self, fmt: &mut ::fmt::Formatter) -> ::fmt::Result {
+        unsafe {
+            fmt.debug_tuple("ManuallyDrop").field(&self.value).finish()


I'd probably recommend just printing ManuallyDrop { .. } here (but retain the T: Debug bound) to be maximally conservative.

Why? The data inside ManuallyDrop is guaranteed to be valid, just like any other wrapper.

I kind of disagree. Since the value can be logically uninhabited, I think we either need to state up front that this will access the inner value and cannot be called after drop, or that it will not. While we could leave the door open to going from not printing to printing, that seems like something I would not be comfortable actually doing in practice.

@gereeter Isn't the entire point of this type that that is not the case?

@sfackler No - the point of this type is that it modifies the behavior on drop - instead of dropping the wrapped value, it just doesn't do anything. However, aside from behavior on drop, ManuallyDrop has no implications on accessing the data.

This is why ManuallyDrop implements Deref and DerefMut.

~Every usage of ManuallyDrop will involve calling ManuallyDrop::drop at some point. It's then up to the developer to ensure that the inner value is never accessed past that call. If Debug does not access that value, and then all of the sudden starts doing so, that seems bad.

I'm not so certain that most uses of ManuallyDrop will involve calling ManuallyDrop::drop. The one use I know of currently in the standard library, for example, never drops the contained value. Looking at the three commonly-downloaded users of nodrop, arrayvec doesn't drop the inside of the wrapper (since it only wants to drop some elements), quickersort just replaces the elements instead of dropping them, and generic-array doesn't drop the array inside NoDrop because it, like arrayvec, needs to only drop some of the elements.

That said, I completely agree that changing the behavior of Debug is not a good idea. I just think there isn't a good reason to not print the contained value.

ManuallyDrop::drop() is unsafe with a warning that it is the responsibility of the user not to access the field after it is dropped. Is there a reason to look at Debug as different from the other methods/traits provided by ManuallyDrop which expect it's contents to be valid?

The user has already been warned about use-after-drop, I'd consider that enough to allow the default Debug impl for ManuallyDrop to remain useful (and print the value).

est31 · 2017-03-16T18:20:21Z

Cross linking rust-lang/rfcs#1860

sfackler · 2017-03-16T18:26:04Z

src/libcore/mem.rs

+
+    /// Extract the value from the ManuallyDrop container.
+    #[unstable(feature = "manually_drop", issue = "0")]
+    pub fn into_inner(self) -> T {


This should be a static method.

This belongs to the RFC.

Yep, commented over there as well.

sfackler · 2017-03-16T18:27:38Z

src/libcore/mem.rs

+///     }
+/// }
+/// ```
+pub union ManuallyDrop<T>{ value: T }


Union fields are private by default, right?

sfackler · 2017-03-16T18:29:37Z

src/libcore/mem.rs

+impl<T: ::fmt::Debug> ::fmt::Debug for ManuallyDrop<T> {
+    fn fmt(&self, fmt: &mut ::fmt::Formatter) -> ::fmt::Result {
+        unsafe {
+            fmt.debug_tuple("ManuallyDrop").field(&self.value).finish()


I kind of disagree. Since the value can be logically uninhabited, I think we either need to state up front that this will access the inner value and cannot be called after drop, or that it will not. While we could leave the door open to going from not printing to printing, that seems like something I would not be comfortable actually doing in practice.

ghost · 2017-03-16T18:45:33Z

src/libcore/mem.rs

@@ -736,3 +736,99 @@ pub fn discriminant<T>(v: &T) -> Discriminant<T> {
    }
 }

+
+#[unstable(feature = "manually_drop", issue = "0")]


Nit: can you move these attributes below documentation? The established ordering convention is "docs, attributes, implementation".

scottmcm · 2017-03-17T06:40:54Z

Would it make sense to have another union field to "mark" the value as dropped? Something like:

pub union ManuallyDrop<T>{ value: T, dropped: () }
    pub unsafe fn drop(slot: &mut ManuallyDrop<T>) {
        ptr::drop_in_place(&mut slot.value)
        *slot = ManuallyDrop { dropped: () };
    }

Then a hypothetical "union sanitizer" could flag any future uses of .value as errors post-drop since it knows that .dropped is the only active fragment. It might also communicate stuff to the optimizer that could potentially be helpful, like "those sizeof(T) bytes are definitely dead now".

(But of course that depends on future events or uncertainties, so may well not be worth doing now.)

alexcrichton · 2017-03-20T22:34:02Z

@rust-lang/libs thoughts on the ManuallyDrop::into_inner question here?

It was brought up on the RFC that the likelihood of shadowing a legitimate method is very small because deref traits don't allow movement. @sfackler, however, brings up that trying to be clever here may mean it's just more difficult to understand in general.

I personally am sympathetic to what @sfackler is saying and would lean a bit towards a static method instead of an inherent method myself at this point.

eddyb · 2017-03-21T00:49:30Z

src/libcore/mem.rs

+///     }
+/// }
+/// ```
+pub union ManuallyDrop<T>{ value: T }


Missing space before {, and value should likely be on its own line (although I don't mind it like this).

eddyb · 2017-03-21T00:52:11Z

@scottmcm I believe that might actually be needed in some interpretations of union.
cc @petrochenkov

petrochenkov · 2017-03-21T15:14:47Z

Would it make sense to have another union field to "mark" the value as dropped?

Seems reasonable if this will help some future sanitizer. This is an implementation detail though and can be changed later.

I believe that might actually be needed in some interpretations of union.

ptr::drop_in_place is an unsafe action orthogonal to unions, so it can break union guarantees and bring unions into invalid state in the same way like, e.g. writes through unsafe pointers can break any guarantees. In this case ptr::drop_in_place does bring the union into invalid state - its active field value no longer contains a valid value.
*slot = ManuallyDrop { dropped: () }; returns the union into valid state again, but I don't see what practical benefits it gives without sanitizers, given that the accessor methods (deref, into_inner etc) still cannot be used.

bors · 2017-03-21T22:23:48Z

☔ The latest upstream changes (presumably #40601) made this pull request unmergeable. Please resolve the merge conflicts.

ghost · 2017-03-22T20:06:03Z

Good job getting rid of NoDrop from our sorting routines!

I see there is one more similar struct left, namely in src/librustc_data_structures/array_vec.rs - incidentally it's called ManuallyDrop, too. Perhaps we could also replace that one?

nagisa · 2017-03-22T21:38:23Z

Perhaps we could also replace that one?

Sure!

scottmcm · 2017-03-29T05:41:59Z

Another random thought: forget(foo) can now be spelled ManuallyDrop::new(foo). Thus,

Should std::mem::forget be implemented using this instead of the intrinsic?
Should ManuallyDrop be must_use to encourage forget when there's no path that might use it later?

nagisa · 2017-04-01T09:57:04Z

Intrinsic replaced.

The PR has been open for a while. I haven’t seen any opinions on into_inner so I’m inclined to just land this and (re-)consider the option during stabilisation.

alexcrichton · 2017-04-01T22:32:57Z

@nagisa want to switch to a static method instead of an inherent function? I'll r+ w/ that

alexcrichton · 2017-04-03T22:26:39Z

r=me, but I'd recommend squashing the commits as well

Looks like travis is also failing?

scottmcm · 2017-04-04T00:08:04Z

@nagisa looks like compile-fail/forget-init-unsafe.rs is using the intrinsic that 4f494c9 removed.

bors · 2017-04-07T01:32:58Z

☔ The latest upstream changes (presumably #41121) made this pull request unmergeable. Please resolve the merge conflicts.

less intrinsics = better life

alexcrichton · 2017-04-11T13:57:41Z

@bors: r+

bors · 2017-04-11T13:57:44Z

📌 Commit c337b99 has been approved by alexcrichton

bors · 2017-04-11T16:14:23Z

⌛ Testing commit c337b99 with merge 5ad9e49...

bors · 2017-04-11T16:44:36Z

💔 Test failed - status-appveyor

TimNN · 2017-04-11T16:49:20Z

@bors retry

Spurious failures on AppVeyor due to ar.exe errors #40546

alexcrichton · 2017-04-11T16:49:47Z

@bors: retry * ar.exe failure #40546

…

On Tue, Apr 11, 2017 at 11:45 AM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2882> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40559 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95KbmXsd4yUGPrfK0uvC0Uq7ma_S6ks5ru64SgaJpZM4MemKl> .

Implement Manually Drop As the RFC has been from approx a week in FCP without any major comments, I’m taking the opportunity to submit the PR early.

Rollup of 8 pull requests - Successful merges: #40377, #40559, #41173, #41202, #41204, #41209, #41216, #41231 - Failed merges:

bors · 2017-04-12T03:58:42Z

⌛ Testing commit c337b99 with merge da32752...

bors · 2017-04-12T04:01:55Z

☔ The latest upstream changes (presumably #41237) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb

…xcrichton Add mem::forget_unsized() for forgetting unsized values ~~Allows passing values of `T: ?Sized` types to `mem::drop` and `mem::forget`.~~ Adds `mem::forget_unsized()` that accepts `T: ?Sized`. I had to revert the PR that removed the `forget` intrinsic and replaced it with `ManuallyDrop`: rust-lang#40559 We can't use `ManuallyDrop::new()` here because it needs `T: Sized` and we don't have support for unsized return values yet (will we ever?). r? @eddyb

@eddyb

…xcrichton Add mem::forget_unsized() for forgetting unsized values ~~Allows passing values of `T: ?Sized` types to `mem::drop` and `mem::forget`.~~ Adds `mem::forget_unsized()` that accepts `T: ?Sized`. I had to revert the PR that removed the `forget` intrinsic and replaced it with `ManuallyDrop`: rust-lang#40559 We can't use `ManuallyDrop::new()` here because it needs `T: Sized` and we don't have support for unsized return values yet (will we ever?). r? @eddyb

rust-highfive assigned brson Mar 15, 2017

alexcrichton reviewed Mar 16, 2017

View reviewed changes

est31 mentioned this pull request Mar 16, 2017

Manually Drop rust-lang/rfcs#1860

Merged

sfackler reviewed Mar 16, 2017

View reviewed changes

ghost reviewed Mar 16, 2017

View reviewed changes

alexcrichton mentioned this pull request Mar 20, 2017

Tracking issue for ManuallyDrop #40673

Closed

eddyb reviewed Mar 21, 2017

View reviewed changes

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 21, 2017

nagisa force-pushed the manually-drop branch from d3a9ae9 to 13adceb Compare March 22, 2017 16:46

nagisa force-pushed the manually-drop branch from 1420f3d to 0efc33f Compare March 22, 2017 21:37

nagisa force-pushed the manually-drop branch from 0efc33f to f5a7f16 Compare April 1, 2017 09:55

nagisa force-pushed the manually-drop branch 2 times, most recently from 87a5096 to 4090a97 Compare April 2, 2017 10:26

nagisa force-pushed the manually-drop branch 2 times, most recently from 23b8527 to 99f995b Compare April 5, 2017 14:50

nagisa added 5 commits April 9, 2017 18:50

Implement Manually Drop

f6e5661

Move away from the ad-hoc NoDrop unions

3871312

Replace the forget intrinsic with ManuallyDrop

c94b3f1

less intrinsics = better life

into_inner to associated function

4863455

Fix test failures

c337b99

nagisa force-pushed the manually-drop branch from 99f995b to c337b99 Compare April 9, 2017 15:56

frewsxcv mentioned this pull request Apr 11, 2017

Rollup of 5 pull requests #41226

Closed

frewsxcv mentioned this pull request Apr 11, 2017

Rollup of 8 pull requests #41237

Merged

bors added a commit that referenced this pull request Apr 12, 2017

Auto merge of #41237 - frewsxcv:rollup, r=frewsxcv

da32752

Rollup of 8 pull requests - Successful merges: #40377, #40559, #41173, #41202, #41204, #41209, #41216, #41231 - Failed merges:

bors merged commit c337b99 into rust-lang:master Apr 12, 2017

scottmcm mentioned this pull request Dec 20, 2017

Only mark unions as uninhabited if all of their fields are uninhabited #46859

Merged

ghost mentioned this pull request Nov 8, 2018

Add mem::forget_unsized() for forgetting unsized values #55785

Merged

Implement Manually Drop #40559

Implement Manually Drop #40559

Conversation

nagisa commented Mar 15, 2017 • edited Loading

rust-highfive commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gereeter Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyps Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

est31 commented Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Mar 17, 2017

alexcrichton commented Mar 20, 2017

Choose a reason for hiding this comment

eddyb commented Mar 21, 2017

petrochenkov commented Mar 21, 2017

bors commented Mar 21, 2017

ghost commented Mar 22, 2017

nagisa commented Mar 22, 2017

scottmcm commented Mar 29, 2017

nagisa commented Apr 1, 2017 • edited Loading

alexcrichton commented Apr 1, 2017

alexcrichton commented Apr 3, 2017

scottmcm commented Apr 4, 2017

bors commented Apr 7, 2017

alexcrichton commented Apr 11, 2017

bors commented Apr 11, 2017

bors commented Apr 11, 2017

bors commented Apr 11, 2017

TimNN commented Apr 11, 2017

alexcrichton commented Apr 11, 2017 via email

bors commented Apr 12, 2017

bors commented Apr 12, 2017

nagisa commented Mar 15, 2017 •

edited

Loading

gereeter Mar 16, 2017 •

edited

Loading

codyps Mar 16, 2017 •

edited

Loading

est31 commented Mar 16, 2017 •

edited

Loading

nagisa commented Apr 1, 2017 •

edited

Loading