Missing drop in FnOnce adapter for closures

[eefriedman]

struct Foo(i32);
impl Foo {
    pub fn dowork(&self) {
        println!("foo: doing work {}", self.0);
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("foo: drop {}", self.0);
    }
}

fn f<T: FnOnce()>(t: T) {
    t()
}

fn main() {
    let x = Foo(1);
    let x = move || x.dowork();
    f(x);
}

playpen

Somehow, the Foo object leaks: my guess is that it has something to do with the way we generate code for the FnOnce() trait for closures which also implement Fn().

Originally reported at #28796 (comment) .

[Manishearth]

cc @pnkfelix

I think I've seen this before, happens with other traits as well.

[nikomatsakis]

triage: P-medium

[nikomatsakis]

triage: P-medium

[thepowersgang]

This appears to only happen if 1. the closure is called via a generic function, and 2. the captured variable is not explicitly moved into the closure body.

If "let a = x;" is used in place of calling a borrowing method, the destructor is called.

[thepowersgang]

Preliminary investigation - The cause is from the closure not being treated as FnOnce when destructor calls are added to the "body" (in rustc_trans/trans/closure.rs load_closure_environment)

This is exposed when the closure is explicitly called as FnOnce (leading to ownership transfer into the closure, and hence no destructor being called).

Routes to fixing:

1. Alter ClosureKind determining to take into account usage
2. Insert a checked destructor invocation after calling a FnOnce closure (and let the drop flags prevent double-drop for working cases)
3. (Unknown if possible) When a Fn/FnMut closure is called as a FnOnce, explicitly drop after calling (in the call_once "method").

[thepowersgang]

::rustc_trans::trans::closure::trans_fn_once_adapter_shim exists, and is possibly the culprit.

[arielb1]

I don't think the shim is used here.

[thepowersgang]

I'm reasonably sure it is. It is being run, and registers self to be dropped, but cleans up the scope without generating destructor calls

[thepowersgang]

src/librustc_trans/trans/closure.rs#L424 appears to be the culprit, changing that line from

fcx.pop_custom_cleanup_scope(self_scope);

to

fcx.pop_and_trans_custom_cleanup_scope(bcx, self_scope);

Leads to my test case passing.

I'm running the full test suite now in preparation for a PR.

[arielb1]

trans::meth is such a hack

[thepowersgang]

@arielb1

trans::meth is such a hack

What do you mean by that?

[thepowersgang]

PR with fix filed #31462 - Running full local tests at the moment, stage1 passed