RFC: Alter mem::forget to be safe

[alexcrichton]

Alter the signature of the std::mem::forget function to remove unsafe
Explicitly state that it is not considered unsafe behavior to not run
destructors.

Rendered

[joshtriplett]

This effectively breaks any RAII pattern that depends on a destructor. What's the safe replacement that is guaranteed to run?

[nagisa]

nit: “may not” sounds wrong here. “might not” would be better, I think.

[nagisa]

@joshtriplett I believe our destructors are still guaranteed to run, unless you:

1. Kill the process without unwinding the stack; or
2. Explicitly leak an object with Drop implementation via e.g. mem::forget or Rc cycles.

This is actually how I’d want to see the “destructors might not run” part defined, so it puts people at ease, @alexcrichton. We’re not Java after all, which runs finalizers at its own discretion.

Otherwise, I’m undecided about this RFC. While, indeed, mem::forget is not unsafe in Rust’s definition of memory safety, it is very convenient to debug memory leaks and sigsegvs by just looking at unsafe blocks. After this RFC to debug memory leaks you will have to debug the whole codebase.

[aturon]

One thing the RFC doesn't mention is that it's not hard to work around this change in APIs like scoped: rather than exposing an RAII guard directly, you can instead take a closure that represents "the body of execution before the destructor would run". You then invoke the closure, and use a private RAII guard to ensure that the parent thread joins. Since you know the private guard doesn't escape, and you don't leak it, you can be confident that the dtor will be run. I'll be posting an RFC for an API change along these lines shortly.

That said, we will at some point need to spell out some minimal circumstances in which you can rely on a dtor being run, if we want to fully justify a scoped API implementation like that mentioned above.

[arielb1]

@aturon

Your destructor running is just control-flow integrity.

[alexcrichton]

@joshtriplett

This effectively breaks any RAII pattern that depends on a destructor. What's the safe replacement that is guaranteed to run?

As @nagisa mentioned, this RFC doesn't mean that destructors won't be run, it just means that they are not guaranteed to run. All "normal circumstances" will still have destructors run.

The current "replacement" for a destructor that's guaranteed to run is to construct a situation which you know avoids the pitfalls where the compiler does not run destructors. For example this means avoiding panicking from a destructor, avoiding Rc cycles, etc. @aturon's idea for the thread::scoped API falls into this category where the API changes shape to accommodate itself into a situation where the destructor is guaranteed to run.

---

@nagisa

This is actually how I’d want to see the “destructors might not run” part defined, so it puts people at ease

I do agree that the statement "destructors may not be run" may be a little weak, but I'm also somewhat wary of trying to exhaustively list either all location where leaks are possible or all locations where leaks are not possible. Perhaps we could try to list a subset of scenarios where leaks are bugs? For example this code should always run the destructor for x:

fn foo<T>(x: T, f: fn(&X)) {
    foo(&x)
}

After this RFC to debug memory leaks you will have to debug the whole codebase.

This is actually true today as well I believe. Due to the fact that a panicking destructor or an Rc cycle can cause leakage (in safe code), this RFC just means you should look around for mem::forget as well (which should be rare). I don't think that it will make it harder to track down leaks in that sense.

[aturon]

@arielb1

Your destructor running is just control-flow integrity.

It's not quite that simple today: rust-lang/rust#24292 (comment)

[arielb1]

@aturon

Destructors not running if a panic occurs in a destructor is a very annoying bug. On the other hand, destructors of locals always run on a panic (because a double-panic = abort), and you can explicitly drop the local in other cases to handle it assuming control-flow integrity.

If you don't put your struct inside of a Vec, then Vec's bugs aren't relevant to you.

[aturon]

@arielb1 I think we're in total agreement. All I'm saying is that, as with many other things, we will at some point need to write clear guidelines about what you can rely on and need to guarantee when writing unsafe code. The policy can't simply be "you cannot rely on destructors running".

[dgrunwald]

👍 I think this RFC the correct choice for 1.0.

Once we release 1.0 with the ability to write a safe mem::forget, backward compatibility means it'll always be possible to write a safe mem::forget. This means the only alternative to this RFC is to fix all possible causes of memory leaks in safe code prior to 1.0. This is impossible given the current time plan.

We can still add support for RAII guards post-1.0 by adding the Leak trait in a backwards-compatible fashion: the safe mem::forget<T> (and any user-written versions of it) can't be called with !Leak objects. We'd have to add a mem::forget_unsafe<T: ?Leak> function if it's really necessary to forget about RAII guards.

Yes, there'll probably be many places that won't be updated to use T: ?Leak even though they could... but then again, do you really need to put RAII guards into all kinds of containers?

[nikomatsakis]

@joshtriplett

This effectively breaks any RAII pattern that depends on a destructor. What's the safe replacement that is guaranteed to run?

Well, that depends on what you mean by "depends". :) If the destructor is cleaning up resources, then it continues to work fine. If the destructor is modifying state to move something from inaccessible to accessible (as with mutexes and RefCell, for example), that's fine too. It's just when the value will be accessible anyway but in an inconsistent state that you have a problem and want to use a closure (or perhaps some other mechanism, if we add one in the future).

In general, the intention of this RFC is not to say that you can't rely on destructors to run (though there are some important caveats to consider...) but rather that when you relinquish ownership of a value outside of your control, it may get leaked and not run, so you have to consider that.

[terpstra]

Hi! I'm new here, so sorry if this is a rather uniformed point of view.

One of the things I really liked about Rust is how it married C++-style RAII, a functional type system, and memory safety. This proposal essentially kills RAII! I think the alternative, guarantee that destructors run, is a much better option. This RFC renders guards of all sorts prone to leaking. I understand that "unsafe" means only "memory unsafe", but when writing real code, you care about other sorts of correctness, too. Guards are a very useful design pattern. I would argue they are more useful than RC!

The RFC mentions some outstanding bugs that can lead to unrun destructors. As a user, I expect bugs, even in a release version. It's only 1.0! A vain attempt to rid Rust of all bugs in time for release seems a very poor justification for fundamentally changing (for the worse) the semantics of Drop. Bugs can always be fixed later. You will never get RAII back once you declare that Drop cannot be depended upon.

I think that a solution to this problem should be aimed at fixing RC. RC is in many ways antithetical to the philosophy that Rust espouses and drew me to your project. [A]RC is essentially giving up on finding an owner. Most times I've seen reference counting used in C++, it was due to developer laziness.

Please don't ditch a very useful design pattern, familiar to every C++ programmer, in the name of RC!

[nagisa]

@terpstra C++ has the same ceveats as we do. If you somehow leak an object (via a loop in refcounted pointer, for example) in C++ it won’t run the destructor either.

[alexcrichton]

@terpstra

Please don't ditch a very useful design pattern, familiar to every C++ programmer, in the name of RC!

To be clear, this RFC is not proposing any changes to any of the existing #[stable] RAII patterns in the standard library. For example the lock() method on a Mutex will still return an RAII guard. This RFC states that the guard cannot require Drop to ensure memory safety (like thread::scoped's guard did).
I believe @nikomatsakis's comment expands on this as well.

I think that a solution to this problem should be aimed at fixing RC

Note that this will require an extensive audit as well to ensure that there are no other forms of leakage. It is also quite difficult (and may have a performance impact) to fix the existing bugs mentioned in this RFC.

[arielb1]

A working Scope implementation

[terpstra]

No. C++ will never move an object from the stack to the heap, in the Rust sense. The object on the stack is guaranteed to run its destructor, barring the sorts of things like your program dies. So, guards remain safe to use.

A memory leak in the heap is something else entirely. C++ programmers are well aware that heap objects might live forever.

[terpstra]

"To be clear, this RFC is not proposing any changes to any of the existing #[stable] RAII patterns in the standard library. For example the lock() method on a Mutex will still return an RAII guard."

That just makes this RFC even worse! Memory safety is not the only safety. Leaking a lock could be even more deadly than reading invalid memory. A straw-man of this proposal is:
1- We continue to depend on RAII for things that are "only" unsafe for non-memory issues
2- We allow destructors to be skipped, violating RAII

In other words: this proposal is willing to cause any other form of breakage, as long as it is not memory breakage.

[arielb1]

@terpstra

Rust already allows for many kinds of breakage that aren't memory-unsafety.

[Manishearth]

+1 for restricting what we mean by safety, -1 for marking mem::forget as safe.

In the future if we come up with a design that fixes the issue with scoped, we may wish to make leaks unsafe. In that case it would be a breaking change to unsafeify mem::forget.

I think it should be fine to be restrictive about what is considered unsafe behavior in Rust, but liberally apply the unsafe keyword to footgunny functions which may be included in the definition of unsafe in the future. Especially if we make a note to this effect in the docs.

[hanna-kruppe]

@terpstra First, C++ can move objects from the stack to the heap just fine. It does call the destructor on the stack thing, but since it was moved from, it won't close whatever resource we're talking about (assuming the type is properly written). Example: http://ideone.com/gmCR7I

Second, deadlocks are possible without leaking guards/not calling destructors (e.g., by acquire the guards in an inconsistent order). Likewise, many other wrong things can be done without it being unsafe, not least because there is no clear, usable set of rules that would disallow all wrong uses while still allowing most of the correct ones. A programming language that catches all bugs is a programming language that can't do anything.

[terpstra]

@rkruppe Typically you make a Guard without a copy constructor. And C++ never "moves" (in the rust sense) anything other than plain-old-data. Classes always copy construct + destruct the old value.

Obviously deadlocks and other bugs are outside of Rust's control. That's not the point. The point is that RAII is a useful design pattern and discipline used in avoiding bugs. This RFC proposes to take away its teeth, rending the design pattern useless.

[andrew-d]

👎 I'm really not a huge fan of this proposal. In any long-lived application, it's useful to be able to rely on the fact that destructors will run if the program/thread continues to run. If a thread panics, or if the process aborts, not running destructors is fine - otherwise, they should run. The alternative is not being able to rely on side-effects running (e.g. if you're writing a library), something that I think is a mistake.

A completely made up example, but: say you're writing a library that communicates over the network. Your Conn type implements Drop so that when the type is dropped, it can send a disconnect message to the other side. This would be a very useful concept, except for the fact that if the user decides to use your library in certain ways, it's now possible for your destructor to silently never run, and there's nothing you (as a library author) can do to prevent this. That feels like a problem to me.

_{Yes, you can just write the API another way, but this is the kind of thing that users will get wrong. Having such an easy way to shoot yourself in the foot seems like a bad idea.}

[hanna-kruppe]

As for the RFC itself: I am very sad about this whole affair, and I don't think marking mem::forget safe has any added value (beyond a bit of consistency, since, as the text says, unsafe doesn't mean "probably wrong"). But it seems pretty clear that guaranteeing that destructors always run is completely impractical, especially at this stage.

Marking Rc and everything else that can leak unstable would be a punch in the face of the stability guarantee, and if any leak-capable API is missed, we'd be stuck with it (and thus, the leaks) anyway. Moreover, Rc in particular is extremely useful, including in many situations where proving the absence of cycles to the compiler is impossible (well, maybe if the compiler was an automated theorem prover, but nobody wants to go there), so it's not like guaranteeing destructors being run would have no drawbacks in a perfect pre-1.0 world.

Alternative: If we get at least some restricted form of negative bounds, there is a backwards-compatible way forward for guaranteed destruction: An opt-in trait (basically the complement of the Leak trait discussed in the RFC), say, NeedsDrop. APIs can that leak could then just refuse to deal with, which is breaks no code because at the time of introduction no types implement this trait. It would of course limit what one can do with these types, but this is an acceptable trade-off for memory safety. Thoughts on this?

[petrochenkov]

@terpstra
Btw, destructors aren't guaranteed to run in C++ as well. For example they aren't run when copy/move elision happens.
A notable mistake based on this subtlety can be found in the recent TC++PL edition in example with finally - a class running arbitrary code in destructor. An object of class finally is returned by a factory function and is usually destroyed once due to copy elision, but can be destroyed twice (and run the code twice!) if copy elision doesn't happen.

Anyway, I still don't quite like making forget safe, because it's obviously not an ordinary function.

[dgrunwald]

@rkruppe: it wouldn't be backwards compatible to add negative NeedsDrop bound to existing types like Rc.

Unless we do some pretty significant breaking changes pre-1.0, the default for unbounded generics will always have to be "safe to leak".

[bluss]

@arielb1

The current decision just means that if you pass ownership of a value to a safe function, your value's destructor may never be called.

I wish the RFC was this specific! Proposed doc for mem::forget doesn't have this information either. rust-lang/rust#25187

@frankmcsherry

For the record, I think it is great that mem::forget is safe. The fewer unsafe things the better. It helps us understand what our code actually does, even if it isn't what we want.

For the record I'm completely fine with that too.

[joshtriplett]

Personally, I'm curious what legitimate use mem::forget has that doesn't involve unsafe code. Every use case I know of involves FFI.

[pcwalton]

I would like the ability to leak data that I know will live forever so that I can create cycles arbitrarily with 'static lifetimes in it.

[bluss]

@pcwalton Can you do that with std::boxed::into_raw<T>(Box<T>) -> *mut T ? It can too have its unsafe removed now. I guess it should have a variant that returns &'static T instead.

[pcwalton]

It needs &'static. I guess it would be a function separate from forget that applies to boxes specifically. But all of the arguments against making forget safe would apply equally well to this box-leaking function, and I do have a use case for it.

[frankmcsherry]

I have mem::forget in my code and I used to have to wonder: "could my method cause undefined behavior if is called with the wrong types?" and now I do not. This is good, because it wasn't ever clear what assumption mem::forget required to be safe. That is my use case.

Generally, I think the fewer methods marked unsafe the better. At least, I treat it to mean something ("might cause undefined behavior if used incorrectly") that requires serious attention, and if a method does not have this property (or is unclear) adding unsafe only makes my life harder.

[bombless]

Dont forget to mark ptr::write as safe.
I also feel that most leaky code such as reference cycles should trigger warning.

[ben0x539]

ptr::write is not only unsafe because of the destructor thing.

[lambda-fairy]

@bombless ptr::write should still be unsafe, because you can mutate shared references through it.

[huonw]

ptr::write's core unsafety is that it dereferences/writes to a raw pointer.

[Finomnis]

Sadly, this change makes implementing a safe DMA transfer a lot harder than before. You can no longer use a &mut DmaChannel object and &[u32] input buffer together with a WriteInProgress guard object to guarantee that the DMA transfer is finished/cancelled before the input buffer can get dropped or reused. This leads to repeated unsoundness problems in the embedded world, like imxrt-rs/imxrt-hal#137. I wish forget was still unsafe ...

Especially because the reasoning behind this change doesn't apply for the DMA problem. exit isn't a thing in embedded, and circular Arc loops do not release a &mut lifetime without running the proper destructor first. Thus, the statement allowing mem::forget from safe code does not fundamentally change Rust’s safety guarantees is simply incorrect.

Before this change, it was guaranteed that if an object contains a lifetime and a destructor, the destructor will get called before the lifetime is released. After this change this is no longer true.

[thomcc]

circular Arc loops do not release a &mut lifetime without running the proper destructor first.

This isn't really true, or rather, I think it's besides the point. Consider:

pub fn forget<T>(val: T) {
    use std::{cell::RefCell, rc::Rc};
    struct Foo<T>(T, RefCell<Option<Rc<Foo<T>>>>);
    let x = Rc::new(Foo(val, RefCell::new(None)));
    *x.1.borrow_mut() = Some(x.clone());
}

[Finomnis]

@thomcc Point taken. Forget what I said, if it's possible to implement it with purely safe code, then of course it's safe. No questions asked.