Propose `Interior<T>` data-type, to allow moves out of the dropped value during the drop hook.

[aidancully]

rendered

[eefriedman]

Is this proposing to add a special case to the orphan rules? (Normally, you can't implement traits defined in another crate for types defined in another crate.)

[aidancully]

The idea on this line is that T is defined in a local crate, so I think impl DropValue for Interior<T> is valid per orphan rules, but I'll double-check.

[eddyb]

Drop has special rules so you can't have partial impls for generic types:

struct Foo<T>(T);
// error: Implementations of Drop cannot be specialized [E0366]
impl Drop for Foo<()> {
    fn drop(&mut self) {}
}

[aidancully]

Right, I've just read the discussion around that... You're right, I don't want to change this behavior. I'll amend the RFC.

[eefriedman]

You probably want to explicitly note how private fields of T are handled.

[aidancully]

They would continue to be private to the module where T was defined, but I can note that explicitly.

[eefriedman]

Maybe explicitly note that these functions are not unsafe?

[eefriedman]

Actually, I'm not completely sure whether these need to be unsafe. into_interior and from_interior seem suspicious.

[aidancully]

The way I see it, the safety of these functions should match the safety of mem::forget, which has recently been made safe. But I'll add a note to that effect in the text.

[eefriedman]

Consider an example like this:

struct MaybeCrashOnDrop { c: bool }
impl Drop for MaybeCrashOnDrop {
    fn drop(&mut self) {
        if self.c {
            unsafe { *(1 as *mut u8) = 0 }
        }
    }
}
pub struct InteriorUnsafe { m: MaybeCrashOnDrop }
impl InteriorUnsafe {
    pub fn new() -> InteriorUnsafe {
        InteriorUnsafe { m: MaybeCrashOnDrop{ c: true } }
    }
}
impl Drop for InteriorUnsafe {
    fn drop(&mut self) {
        self.m.c = false;
    }
}

Currently, safe code can't break safety rules using an InteriorUnsafe, but it could if into_interior were considered safe, I think?

[aidancully]

I'm not the best person to make this argument, but my understanding of Rust's current definition of "unsafe" is that it refers specifically to memory-unsafety - that is, in the ability to access invalid memory. As your example shows, into_interior can provide the ability to violate API invariants, in the same way that mem::forget can (see the thread::scoped discussion, rust-lang/rust#24292, and the mem::forget discussion, #1066). It is a foot-gun, just like mem::forget is. But it does NOT do anything memory unsafe, and it does not cause unsafe behavior (though it can, as you show, cause an invariant to be violated in such a way that it results in unsafe behavior). I'm not persuaded that these functions are unsafe.

[eefriedman]

mem::forget is specifically documented as something safe code is allowed to do, into_interior is not, and changing that isn't backwards-compatible.

Safe code staying safe depends on unsafe code maintaining its invariants. One invariant that unsafe code can currently depend on is that the drop implementation for a struct runs before the drop implementations of its members. Breaking that invariant breaks backward-compatibility: a library whose invariants can't be broken with safe code in 1.0 can have its invariants broken in 1.x.

I think you could get around the safety issue if the members of an Interior<T> were also of type Interior<T>. Of course, that leads to ergonomic issues because you can accidentally leak members of a struct.

[eddyb]

I have to agree with @eefriedman here, being able to break the invariants of a type you didn't define, from safe code, is a no-no.
For some reason I thought T -> Interior<T> was safe, and it might be in most existing Drop implementations, but not all of them: some put their fields in a state that would make the fields' destructors noops, or at least not memory-unsafe.

[aidancully]

I base the argument that into_interior is safe around a lot of the discussion I saw in the mem::forget discussion. Given that discussion, if, hypothetically, this proposal had been made and accepted before Rust 1.0 shipped, would this function have been called safe or unsafe? Considering comments like this one, it's hard for me to believe that into_interior would have been made unsafe.

I think what's being described is not so much an "unsafety" issue is actually a backwards-compatibility issue... (And, note, this same back-compat issue would also exist under the consuming-destructuring alternative.) The only consistent resolution I see is to disallow into_interior for Drop types. Which means that both Drop and into_interior need more special-case logic in the language than I had originally thought...

[eefriedman]

Yes, you could think of it as a backwards-compatibility issue.

Under the consuming-destructuring proposal, we would probably only allow destructuring Drop types if the type is defined in the same crate as the destructuring use, unless someone has a better idea for a rule. A bit restrictive, but it dodges the backwards-compatibility concerns.

[eefriedman]

Might be worth explicitly describing the shim the compiler has to generate for trait vtables. (Consider the code necessary to implement core::intrinsics::drop_in_place::<Any>.)

Overall, this seems like a solid proposal.

[pnkfelix]

cc me

[Ericson2314]

Thanks for proposing this @aidancully, especially making it backwards compatible!

[Ericson2314]

Is it possible to do partial moves like this? Otherwise do:

let Bar(x, y) = this;
DropValue::<Foo>::drop_value(into_interior(x));
DropValue::<Foo>::drop_value(into_interior(y));

[aidancully]

In the proposal, it's possible to do this so long as DropValue isn't defined for Interior<Foo>: partial moves are allowed when type doesn't implement DropValue. In the example, DropValue is implemented for Bar, but not for Interior<Bar>; so a partial move out of Interior<Bar> is allowed. That said, the proposal needs better discussion of partial moves.

[eddyb]

Interior<T> cannot implement DropValue if the same sanity checks we do for Drop are kept (and I don't see why that should be possible).

[Ericson2314]

As I said like in our discussion long below, I still prefer tying destructuring and functional update to whether the type has private fields, rather than whether any trait is implemented.

[Ericson2314]

On the other hand, seeing that mem::forget is safe, I don't know why we need to care about circumnavigating user-defined destructors at all.

[aidancully]

Well, just because mem::forget is safe, doesn't mean we should be encouraging its use...

[Ericson2314]

The point of manual drops it to override the default behavior (duh). So the final drop behavior and the default exist in a sort of self-super relationship.

The point of Interior<T> is to expose the default drop implementation. But in that regard, all the freely generated Interior<...Interior<T>...> make no sense, as there is no "extra default" implementation.

IMO it is simpler/less magical just to expose a different function which is the default drop_value implementation (both would take plan T). Ideally this would just be the default implementation of drop_value, if it was possible to call the default implementation of a method in the definition of it's override. The point of it will be to avoid infinite recursion with drop_value.

The vast majority of implementations will not need to worry about infinite recursion, as all interesting things to do in a drop_value implementation involve destructuring this. But it is useful in the definition of the Drop => DropValue blanket impl: drop_value would just call the default implementation after to avoid recursion.

[aidancully]

IMO it is simpler/less magical just to expose a different function which is the default drop_value implementation (both would take plan T).

I think that would be acceptable if infinite recursion were the only concern... But it doesn't solve the problem of allowing partial moves from the value during the drop-hook: partial moves are still disallowed for types implementing Drop, and taking T by value in drop_value means we're still working with a type that implements Drop. Partial moves would still be disallowed...

I know there's been discussion of using destructuring to allow consumption of the container type without consuming the fields, which would inhibit invocation of the drop-hook while continuing to provide access to fields. The difficulty I've had with this approach is that I haven't seen how it would work with some kinds of enums, which don't have moving destructures:

enum Foo {
  Bar,
  // the way this enum gets destructured might depend on its discriminant:
  //Baz(Box<u32>),
  // but in this case, doesn't really...
  Qux,
}
impl DropValue for Foo {
  fn drop_value(self) {
    // use `match` to find the discriminant:
    match self {
      when Bar => (),
      when Qux => (),
    }
    // "self" is still accessible? argh, must use `forget`.
    mem::forget(self)
  }
}

When this came up before, I suggested a match move operation, which would be a consuming destructuring for an enum type. I'd be ok with that, but would argue against any solution that requires forget... forget can be used to deliberately circumvent an API design, whenever the API has meaning attached to drop, its use shouldn't be encouraged, IMO.

[Ericson2314]

@aidancully Thanks, I hadn't realized the problems with Copy before. Exposing a "default method" thing would help with the boilerplate, but not with Copy before. [Nor am I willing just say a type can't be DropValue and Copy, linear types that impl Copy might perhaps be useful].

match move is a great solution, semantically a lot lighter-weight than Interior<T>. I hope we can adopt it.

[eddyb]

@Ericson2314 You currently cannot impl Copy and Drop at the same time.
Abstractions have been built that assume T: Copy implies dropping T is a noop (Cell would be one example).
Allowing destructors on copyable types would be a breaking change.

[eddyb]

Why aren't these static methods? Interior::of(T), for example.

[aidancully]

Because I wanted to avoid possibility of namespace collision. This is nonsensical, but demonstrates the problem I was trying to avoid:

struct Foo;
impl Foo {
  fn of(&self) -> &Foo {
    self
  }
}

If we have impl Deref for Interior<T> with Target == T, then there's an ambiguity in how of would be resolved, right?

[eddyb]

No, because resolving paths does not touch Deref at all.

[aidancully]

OK, right - we can make these all static methods on Interior. I played it a little too conservative.

[Ericson2314]

@eddyb OK, but then we don't even need match move!

[eefriedman]

@Ericson2314 the problem isn't with a type that implements Copy and Drop; the problem is with a type that implements Drop, but all the members are Copy. There's also the issue that making an unmarked pattern match skip dropping a value might be a bit too easy to screw up.

[Ericson2314]

Ah, sorry I was confused.

[aidancully]

One of the reasons I haven't written up match move is that it didn't seem to get any interest when I suggested it, but I probably wouldn't approach it in such a narrow way, either... I'd try defining move as a low-precedence, right-associative operator, which would locally mask Copy behavior for a type, and ensure that the moved value doesn't outlive its expression scope, as in:

let x = 0u32;
let y = move x; // x is no longer accessible
fn blah(arg: u32) {}
blah(move y);
#[derive(Copy,Clone)]
struct Foo(u32, u32);
let z = Foo(0, 1);
let (a0, a1) = move z;

Of course this likely conflicts with other uses of move, especially in closure definition, and I still haven't thought about it very seriously.

[eddyb]

@aidancully I really don't see the utility of move <expr>, unless only macros or syntax extensions expanded to it (and then it could be prototyped in a compiler-implemented macro).

[Ericson2314]

It seems to me that the move should actually be part of the pattern, as we are trying to overrides and "optimization" where patterns that would otherwise consume something don't. Also this allows
let move _ = something_that_gives_a_result, which is a nice way to get around must-use.

[Diggsey]

Given that mem::forget() is safe, is there any reason why partial moves should still be disallowed on drop types, assuming the fields are accessible?

Infinite recursion can be avoided by either:

• Have the compiler add an implicit call to mem::forget() at the end of the destructor if self is not consumed.
• If that's too magical, a lint to detect the infinite recursion would at least prevent accidental problems, and mem::forget() can be called manually. After all, it's a sufficiently rare occurence that I think explicitness wins out over verbosity.

[eddyb]

@Diggsey ... "sufficiently rare occurrence"?
We're talking about a replacement for Drop, you'd want the default to do the right thing, always calling mem::forget at the end of destructors to get the correct behaviour seems like an anti-pattern.

[Diggsey]

@eddyb It's rare if you continue to use old Drop for destructors which only need to borrow self. A DropValue destructor should consume self - if it doesn't do that via destructuring then it should call mem::forget().

It seems like a fairly straightforward choice - either you special-case DropValue in the compiler by implicitly forgetting self if it hasn't been consumed, or you require the programmer to manually consume self. Interior<T> in this proposal is just a very round-about way of doing the former.

[eddyb]

@Diggsey I thought the whole point of this proposal was to allow moving out of self in the destructor (and also avoid infinite recursion) by having it be Interior<Self> and not just Self, as the latter would continue to have the restrictions it does today.
What am I missing?

I should also mention that current Drop situation is acknowledged as suboptimal.
It's not supposed to take &mut self, but we didn't have anything better.
self: Own<Interior<T>> is the "correct" type, although it could be a while before that can be implemented.

[Ericson2314]

What exactly is a partial move? I assume that if you move a non-copy value out of a non-copy value, it consumes the original. I agree with @Diggsey that the only Drop implementation that shouldn't easily consume self is the Drop => DropValue adapter.

as the latter would continue to have the restrictions it does today.

Certainly for backwards compatibility Drop would need to prevent all destructuring. Ideally all other destructuring could be privacy based, but that also breaks backward compatibility. It may have to be that the privacy-based destructuring is used only for types that implement DropValue, (and in the future, linear types).

[eddyb]

@Ericson2314 if you want to move out of foo outside of the destructor, you'll need to first "peel off" the destructor with Interior::of(foo).
That was my intention, at least, you seem to be talking about an alternate proposal that doesn't involve either of Interior or DropValue (as defined here).

[Ericson2314]

@eddyb Sorry for the confusion, I am talking about the alternative of having drop_value take self.

The privacy-based rule would only allow one to "peel off" the destructor through destructuring when all fields are visible.

[Diggsey]

I thought the whole point of this proposal was to allow moving out of self in the destructor (and also avoid infinite recursion) by having it be Interior and not just Self, as the latter would continue to have the restrictions it does today.
What am I missing?

OK, let me present an alternative which has drop_value take self:

1. Allow partial moves out of types with destructors. Instead, the restriction is that variables which currently have one or more fields 'moved-out-of' cannot be used or even dropped, and it's a compile error for a variable to go out of scope while in an incomplete state.
2. Add mem::destructure(), an intrinsic whose behaviour mimics what happens when an External<T> from this RFC is dropped: all fields which have not been 'moved-out-of' are themselves dropped.
3. drop_value() has an implict call to mem::destructure(self) at the end.
4. (optional) Upgrade mem::forget() to also accept incomplete values, as a complement to mem::destructure().

[eddyb]

@Diggsey uh oh, all of those are pretty much hacks:
2. requires special-casing calls to a specific intrinsic in borrow-checking (and passing information from borrow-checking to codegen).
3. requires special and potentially surprising codegen for an otherwise regular method.
4. more special-casing intrinsics - not to mention that std::mem contains wrappers for intrinsics, would they have to be special-cased, too, and how would that be achieved?

I really can't see how that could be accepted without proper refinement types describing the partially-moved-out state, and those are even further out than Own<T>.

[aidancully]

I think that doesn't work, since CanDrop would also be defined on Copy types, and Copy types should never have drop-glue... I should update the RFC to remove the drop_glue routine... I think something like it is a good idea, but I'm not sure it's the right time to define it (would the drop_glue signature change when / if we get immovable types? how will it interact with affine and linear types?), and it isn't particularly relevant to the main thrust of Interior<T>. drop_glue can be moved into its own RFC when the time comes...

[nrc]

@pnkfelix this RFC hasn't had any comments in a year, should we FCP? Could you summarise the status please?

[Ericson2314]

@nrc if we got &move this would have new life breathed into it. As it stands this is blocked because Drop can be implemented for DSTs (right?) but they cannot be moved by value.

[eefriedman]

Given that #1444 was accepted, this isn't very important. SmallVec can be implemented in a straightforward manner on top of a union. And it's possible to write a wrapper type which allows zero-overhead safe consume-on-drop:

// This API is terrible and incomplete, but it's enough to make my point
union NoDrop<T> {
    t: T
}
pub struct ConsumeOnDrop<T, F: FnMut(T)> {
    t: NoDrop<T>
    f: F
}
impl<T, F: FnMut(T)> ConsumeOnDrop<T, F> {
    pub fn new(t: T, f: F) -> ConsumeOnDrop<T, F> {
        ConsumeOnDrop { t: NoDrop { t: t }, f: f }
    }
}
impl<T, F: FnMut(T)> Drop for ConsumeOnDrop<T, F> {
    fn drop(&mut self) {
        unsafe {
            (self.f)(ptr::read(&mut t.t));
        }
    }
}

[Ericson2314]

@eefriedman I think moving-drop is still valuable because a) it accurately reflects the semantics of destruction and b) it confines all mandatory unsafety to drop_glue.

[nikomatsakis]

Hear ye, hear ye! This RFC is now entering final comment period. The @rust-lang/lang team is inclined to close, since the addition of unsafe unions allows for things like the small vector use case to be handled that way, and in general there doesn't seem to be a great amount of urgency for this change.

[Ericson2314]

Could we close with the understanding that it could be revisited if we get &move / used as justification for &move?

[jethrogb]

it's possible to write a wrapper type which allows zero-overhead safe consume-on-drop

For some definitions of "safe".

I still support something like this RFC. Looking at current alternatives: I find neither the Option version nor the union version terribly ergonomic, in particular because they require unwrap or unsafe at every point of use, whereas I'd like any special handling limited to just the drop function.

[Ericson2314]

My main motivation is, well, a belief that &mut for drop just is disgustingly incorrect---to the point where it probably makes the language harder to learn by creating a special case where the usual rules for what makes a &mut borrow safe don't apply.

[ubsan]

@Ericson2314 implementing drop glue by yourself sucks :(

[Ericson2314]

@ubsan hmm? I may have personally mused on a no-auto-drop-glue world :), but this RFC doesn't make it manual, especially that in the presence of partial moves.

[jethrogb]

Is this a safe public interface?

pub union NoDrop<T> {
    inner: T,
}

impl<T> NoDrop<T> {
    fn into_inner(self) -> T { /*...*/ }
}

impl<T> From<T> for NoDrop<T> { /*...*/ }

impl<T> Deref for NoDrop<T> {
    type Target = T;
    //...
}

impl<T> DerefMut for NoDrop<T> { /*...*/ }

I think so, and I think it would fix the ergonomics issues I was talking about before.

[ubsan]

@Ericson2314 Afaict, any person that implements DropInterior must deal with drop glue themselves. A struct that does not implement a Drop trait may not have to...

[Ericson2314]

@ubsan hmm Interior<T>'s fields aren't themselves "Interior'd", so I don't think that's the case. Interior<T> has no drop instance, but not no drop glue.

[ubsan]

@Ericson2314 I guess I see it. Still seems... overcomplicated.

[aidancully]

I'm out of practice with Rust, but I'm not sure why @eefriedman's union is used? Is it just to prevent the interior type from implementing Drop? Because I don't see that the union RFC actually does prevent Drop on fields: it generates a lint, but does not actually prevent Drop fields. Which is an improvement, but if the embedded type does define Drop, then I think the union approach ends up calling Drop twice on the contained value. (Once in the consuming function, and once in the drop glue for the ConsumeOnDrop type.) Which would be bad.

I think the DropGlue marker-trait from this RFC, with negative trait bounds, would make this more robust, but my test code showed this basic approach to be pretty awkward to use. (Which could well be my fault, I'm out of practice.) I made another variant that was a little nicer:

trait DropConsumer: !DropGlue {
  fn drop_consume(self);
}
struct ConsumeOnDrop<T: DropConsumer> {
  t: T,
}
impl<T: DropConsumer> ConsumeOnDrop<T> {
  fn new(t: T) -> Self {
    ConsumeOnDrop { t: t }
  }
}
impl<T: DropConsumer> Drop for ConsumeOnDrop<T> {
  fn drop(&mut self) {
    unsafe {
      ptr::read(&self.t).drop_consume()
    }
  }
}

I don't see this as that different than the RFC... There are still two types, but instead of being T and Interior<T>, it's ConsumeOnDrop<T> and T; and it's up to the user to explicitly opt into using consuming drop, which makes it a certainty that consuming drop will never be the default drop mechanism... And I think that's a shame, because consuming drop is (IMO) more "correct". On the other hand, it's certainly true that the explicit drop-consumer is easier to understand than the weird implicitly-generated Interior<T> type.

Either way, I won't have time in the foreseeable future to put serious effort into this or other RFCs, so I can't object to closing it.

[ubsan]

@aidancully union types don't have any drop glue.

union Forget<T> { t: T }

Forget { t: ... };

is equivalent to

std::mem::forget(...);

[aidancully]

@ubsan, Thanks for pointing that out, I didn't read the union RFC carefully enough and missed that. Using union would allow the consume-on-drop implementation described above to work. And I think would work with the scheme I described, too, just replacing struct ConsumeOnDrop<T: DropConsumer> with union ConsumeOnDrop<T: DropConsumer>.

I think my point that consuming drop, while possible, would never become as ergonomic as &mut drop remains...

[nikomatsakis]

My main motivation is, well, a belief that &mut for drop just is disgustingly incorrect

I'm aware you feel this way, but I continue to think this is an exaggeration. From the point of view of the Rust type system guarantees alone, &mut T is a perfectly valid type. That is, the data is not owned by the drop code -- the overarching drop glue will go ahead and expect those fields to have valid types, ready to be dropped in turn -- so they are borrowed.

Certainly it's true that Drop marks a state transition, and if you consider an extended type, the extended permissions on drop are thus different from other methods that use &mut (the type goes from "normal" to "post-drop", basically, which means some invariants might not hold). But then again, if you have an unsafe fn, the same could be true. So in general I think the "flow" of these logical invariants requires richer type signatures that what the Rust itself provides.

None of this is to say that I think &mut is a perfect fit -- but it's also not "disgusting".

[Diggsey]

In that case I think that Drop is not really equivalent to destructors in other languages, but is very similar to eg. finalize in .net. Now that drop-flags are gone, it means that to actually implement a destructor you have to extend the set of possible states for your type to be in to include "post-drop", which may come at a memory/performance/maintenance cost.

[nrc]

Agree we should close - we might want to consider something like this, but I think we should re-evaluate with unions, and after considering &move in its own right, etc. Doesn't seem much gain in leaving this RFC open

[ubsan]

@Diggsey I think it's exactly equivalent. ~T in C++ is:

class This {
    ...
public:
    ...
    ~This() {
        ...
    }
    // not
    ~This() && {
        ...
    }
};

[nikomatsakis]

Thanks everyone for the discussion. The @rust-lang/lang team has decided to close this RFC, as previously proposed. As @Ericson2314 noted, this proposal could be revisited in the future when we have more experience with union and/or other language features.