Manually Drop

[nagisa]

Rendered

[KalitaAlexey]

@nagisa,
I like the idea.
I see that you don't propose to make the compiler emit an error if a struct has a manually dropped field but doesn't impl Drop.
I think that the compiler must emit an error.

[KalitaAlexey]

@nagisa,
You don't describe the situation where a struct has as a manually dropped field as a common one.

[KalitaAlexey]

You write adding following struct, but it is a union.

[KalitaAlexey]

I think it should have impl Debug if T: Debug.

[KalitaAlexey]

Replace this structure with this union

[nagisa]

I see that you don't propose to make the compiler emit an error if a struct has a manually dropped field but doesn't impl Drop. I think that the compiler must emit an error.

ManuallyDrop may also be used as a method to suppress the destructors, similar to the functionality provided by the nodrop crate. Automatic mem::forget. In that sense I disagree with compiler having to emit an error.

You don't describe the situation where a struct has as a manually dropped field as a common one.

What do you mean? I’d expect ManuallyDrop to be used quite rarely (for its intended purpose, anyway). I even said this in the “How we teach this” section:

It is expected that the functions and wrapper added as a result of this RFC would be seldom necessary.

That is, I believe that large majority of code does not care about the drop order.

[KalitaAlexey]

ManuallyDrop may also be used as a method to suppress the destructors, similar to the functionality provided by the nodrop crate. Automatic mem::forget. In that sense I disagree with compiler having to emit an error.

The one who needs to suppress the destructor can add #[allow] to the field.
It is better than people will get annoyed because of leaks.

What do you mean? I’d expect ManuallyDrop to be used quite rarely (for its intended purpose, anyway). I even said this in the “How we teach this” section:

struct FruitBox { 
    peach: ManuallyDrop<Peach>, 
    banana: ManuallyDrop<Banana>,
    other_fruit: OtherFruit,
} 

[joshtriplett]

It seems odd to call this a struct. Perhaps "type" instead?

[Ericson2314]

I feel like &move drop is far more elegant? Could we just do this on an experimental/temporary basis?

[KalitaAlexey]

@Ericson2314,
What are you talking about?

[Ericson2314]

@KalitaAlexey Sorry, it's in a bunch of old RFCs scattered around.

[Ericson2314]

At random, here's newer #1646 and older https://internals.rust-lang.org/t/pre-rfc-allow-by-value-drop/1845

[codyps]

Perhaps: "this pattern" instead of "such pattern"? Alternates would be "such patterns" and "such a pattern", but "this" seems more natural here.

[nagisa]

I feel like &move drop is far more elegant? Could we just do this on an experimental/temporary basis?

&move is not possible to produce from a &mut, and we cannot change the signature of Drop::drop. If we had &move already, I’d be interested in discussing a MoveDrop::move_drop(&move self), but it does not exist yet. Stabilising unions and having something like this proposal as a crate also sounds okay to me, although it is awkward to recommend people to use a crate for what I propose to make the idiomatic (while we haven’t &move, at least) solution to the problem.

[joshtriplett]

Based on discussion in the lang team meeting: this is a libs RFC, not a lang RFC. Labeling accordingly.

[nagisa]

I feel that its at least to some extent related to T-lang, even if the only relation is about the intent to consider ManuallyDrop the idiomatic approach.

[alexcrichton]

I wonder, was perhaps a Deref and DerefMut implementation considered for this type? This seems like a swell type to have in libstd, and with deref we'd trim the API down to:

impl<T> ManuallyDrop<T> {
    pub fn new(T) -> Self;
    pub fn into_inner(self) -> T;
    pub unsafe fn manually_drop(slot: &mut ManuallyDrop<T>); // not inherent to avoid conflicts
}

impl<T> Deref for ManuallyDrop<T> {
    type Target = T;
}

impl<T> DerefMut for ManuallyDrop<T> {}

[SimonSapin]

    pub fn into_inner(self) -> T;
    pub unsafe fn manually_drop(slot: &mut ManuallyDrop<T>); // not inherent to avoid conflicts

pub fn into_inner(ManuallyDrop<T>) -> T for the same reason?

[alexcrichton]

@SimonSapin yeah I was initially going to recommend that, but I hesitated because almost all into_* methods consume self, so if you're reaching them through Deref you're by definition unable to call them. In that sense the reason we avoid inherent methods on Deref types, to avoid method conflicts, may not actually apply to the into_* terminology.

Although with that in mind it may be too clever, we have Box::into_raw, so it may be not worth it. Either way I'm fine.

[Ericson2314]

&move is not possible to produce from a &mut, and we cannot change the signature of Drop::drop.

These were never problems. It's easy to write a blanket impl from Old Drop to new Drop. Because new Drop grants the implementation a more powerful reference, plain old drop (the function) can be used---that solution is giving the callee more power, not requiring less of the caller as this does.

If we had &move already, I’d be interested in discussing a MoveDrop::move_drop(&move self), but it does not exist yet.

I think the reason we don't have &move is because people were insufficiently peeved with the problem this RFC duct-tapes. &move-Drop is, I'll wager, the only safe solution to the drop-from-drop instance problem. I was completely OK with the reasoning in #1646 (comment), but if this RFC is going to be accepted perhaps we should revisit some of the conclusions of insufficient motivation there.

Taking a step back, I'm fine with this as a stop-gap, especially if it remains unstable until further thinking about &move. I hope it increases the pressure for the more elegant and safe solution, but I fear it will decrease it instead.

[alexcrichton]

ping @BurntSushi (checkbox)

[alexcrichton]

also ping @aturon

[rfcbot]

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

[gereeter]

This should be updated to ManuallyDrop::drop(...).

[oli-obk]

Is it valid to ptr::read from the deref of a ManualDrop to obtain an owned value in a Drop impl?

[nagisa]

I do not immediately see any reason you wouldn't be able to. Operation leaves wrapper in a undefined state so you should take care to not drop or act on the wrapper in any other way after the ptr::read

…

On Mar 16, 2017 12:59 AM, "Oliver Schneider" ***@***.***> wrote: Is it valid to ptr::read from the deref of a ManualDrop to obtain an owned value in a Drop impl? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1860 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0vYJcDm8RUWtRj6Yc0TikpHXnKU1ks5rmG1jgaJpZM4Lpopg> .

[oli-obk]

OK. This is a use case I'd like to see mentioned in the docs and tests. Or even explicitly added as an unsafe take method?

[est31]

Cross linking rust-lang/rust#40559

[sfackler]

This should be a static method like drop.

[alexcrichton]

@sfackler there's some discussion above about whether it's a free function or method, and the method was chosen because conventionally all into_* methods require self-by-value which is typically unusable through Deref and DerefMut

[sfackler]

I was unfortunately not tuned in to the conversation at that point, oops :(

I feel somewhat strongly that it should be static - since we have an otherwise universal convention that smart pointer methods are static, if I see a foo.bar(), I can assume the bar is calling into the inner type. In addition, if/when we support DerefMove, an inner into_inner call could be made!

[alexcrichton]

That's true yeah, I'd personally be fine either way

[rfcbot]

The final comment period is now complete.

[alexcrichton]

Looks like the main issue to arise during FCP is whether into_inner is an inherent method or not, but overall nothing much major arised so I'm going to merge! We can always tweak the implementation during stabilization and come back to tweak the RFC if necessary.

Thanks for the RFC @nagisa!