mem::forget is unsafe, but can be written in entirely safe code

[reem]

For instance:

fn forget<T>(val: T) { 
    let x = Rc::new((val, RefCell::new(None))); 
    *x.1.borrow_mut() = x.clone();
}

This is problematic, and an indicator that something is amiss in our rules.

cc @aturon @nikomatsakis @bstrie from IRC

[bstrie]

Nominating for 1.0 because some men just want to watch the world burn.

[theemathas]

This is one of the root causes of #24292

[elinorbgr]

Hmm... I was trying to understand this example, but it doesn't compile:

test.rs:6:25: 6:34 error: mismatched types:
 expected `core::option::Option<_>`,
    found `alloc::rc::Rc<(T, core::cell::RefCell<core::option::Option<_>>)>`
(expected enum `core::option::Option`,
    found struct `alloc::rc::Rc`) [E0308]
test.rs:6     *x.1.borrow_mut() = x.clone();
                                  ^~~~~~~~~

My first guess was to wrap x.clone() into Some(x.clone()) so that the types matches, but it doesn't compile either:

test.rs:6:25: 6:40 error: mismatched types:
 expected `core::option::Option<_>`,
    found `core::option::Option<alloc::rc::Rc<(T, core::cell::RefCell<core::option::Option<_>>)>>`
(cyclic type of infinite size) [E0308]
test.rs:6     *x.1.borrow_mut() = Some(x.clone());
                                  ^~~~~~~~~~~~~~~

[retep998]

Here is a working example:

fn forget<T>(val: T) {
    use std::cell::RefCell;
    use std::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());
}
struct DontDropMe;
impl Drop for DontDropMe {
    fn drop(&mut self) { unreachable!() }
}
fn main() {
    forget(DontDropMe)
}

[nikomatsakis]

My current thinking:

1. There were other ways to forget even before Rc, at least in some cases. For example, if T:Send holds, you could send the value to a thread that runs an infinite loop, or which is deadlocked on a port.
2. It is true that one cannot assume that a destructor will run, and hence that forget is not itself unsafe (rather, it is unsafe to write a dtor that must run). This is what happened with std::thread::JoinGuard (and scoped) are unsound because of reference cycles #24292. You can sidestep this by writing destructors that must run but never giving up ownership of the value with the destructor. This often involves some closures.
3. It'd be nice if we could use RAII as we initially wanted to. So I suspect we may in the future work on a solution for identifying types whose destructors must run and preventing them from being placed into Rc. (Some kind of trait, most likely.)
4. If we did step 2, then we would probably want forget to continue to "forget" the values in question -- which would mean that forget would become unsafe again. So that argues in favor of leaving it as unsafe for now -- we can always make it safe later if we so choose.

[Kimundi]

@nikomatsakis Did you mean "4. If we did step 3, [...]" rather than "4. If we did step 2, [...]"?

[pythonesque]

@nikomatsakis My concern is that (3) couldn't be done backwards-compatibly, at least not without making it unsafe to create a new type that leaks like Rc does. Doing it now would subsume all four points (except point [1], but as I mentioned earlier Rust is a long way off from providing guarantees of totality or anything crazy like that, so it will be sufficient in practice).

[arielb1]

@nikomatsakis

If you have T: 'static, then you can move it into a thread-local Vec<Box<SomeTrait>> (with an impl<T:?Sized> SomeTrait for T {}) or something and lose it, so forget is only "interesting" on lifetime-bounded traits, where it allows the lifetime to end without the destructor running.

[dgrunwald]

A safe mem::forget has the advantage that it makes it easier to write the counterexample proving thread::JoinGuard unsafe. Safe mem::forget makes it more likely that people will know that destructors are not realiable, so they can avoid repeating this mistake.

we can always make it safe later if we so choose.

Making an unsafe function safe is a (minor) breaking change (fn(T) is not a subtype of unsafe fn(T)).

We might want two functions:

fn forget<T>(val: T) { ... }
unsafe fn forget_unsafe<T: ?Leak>(val: T) { ... }

[alexcrichton]

I commented on #24292, but the gist is that there are multiple ways to leak memory today (e.g. #14875 and #16135), so a targeted solution at Rc may not cover all use cases. Although as I mention in #24292 these other bugs can also be considered separate bugs on their own which need to be fixed regardless (but sometimes is quite difficult to do so).

[alexcrichton]

I have created an RFC for marking forget as a safe function.

[petrochenkov]

By the way, is it okay to leak references through Rc-powered forget?

fn forget<T>(val: T) {
    use std::cell::RefCell;
    use std::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());
}
fn main() {
    {
        let mut a = 10;
        forget(&mut a); // Leak the reference to `a`
        let b = &mut a; // Now we have two mut references to `a`, one here and one somewhere
    }
    // The dangling reference to `a` still lives somewhere. Can it be made accessible without `unsafe`?
}

[pnkfelix]

closing this issue to encourage all further discussion of this matter to happen on the RFC thread, see rust-lang/rfcs#1066

[reem]

@pnkfelix seems weird to close the issue if it's not fixed, how about just leaving the comment?

[pnkfelix]

@reem the idea was to funnel conversation over to the RFC. A single comment in the middle of a thread won't do that.

(To be clear: this was was the outcome of the team triage, since the issue had been nominated; it was was a group decision. My initial instinct had been to let it sit for a week with the nomination, but was persuaded by arguments to close in favor of the RFC.)