drop not always behaving since nightly-2016-08-03-x86_64-unknown-linux-gnu

[mikedilger]

This was discovered in database connection pooling where connections were not returned to the pool properly, and reported under sfackler/r2d2#25

Unfortunately I do not have example code small enough to share at this point.

I narrowed down the rust-nightly version that introduced the bug to nightly-2016-08-03-x86_64-unknown-linux-gnu

@sfackler

[mikedilger]

when compiled with RUSTFLAGS="-Zorbit=off" the issue does not appear, so yes it is the MIR as suspected.

[nagisa]

The compiler not running drop glue is not considered unsafe behaviour (i.e. it is allowed to happen), but it is still weird this happens.

cc @arielb1 could be your drop elaboration not catching this (&*Doop) case?

@mikedilger Something that we could run without having postgresql installed would be ideal test case, even if it depended on other crates for now.

[nagisa]

Minimal reproduction:

enum Error {
    Error
}

struct ConnWrap(Conn);
impl ::std::ops::Deref for ConnWrap {
    type Target=Conn;
    fn deref(&self) -> &Conn { &self.0 }
}

struct App;
impl App {
    fn conn(&self) -> Result<ConnWrap, Error> { Ok(ConnWrap(Conn)) }
}

struct Conn;
impl Drop for  Conn {
    fn drop(&mut self) { println!("drop") }
}

fn accessible_sites(app: &App) -> Result<Vec<i32>, Error>
{
    let conn = &*try!(app.conn());
    return Ok(vec![]);
}

fn main() {
    accessible_sites(&App);
}

[nagisa]

More minimal:

#![feature(core_intrinsics)]
struct ConnWrap(Conn);
impl ::std::ops::Deref for ConnWrap {
    type Target=Conn;
    fn deref(&self) -> &Conn { &self.0 }
}

struct Conn;
impl Drop for  Conn {
    fn drop(&mut self) { unsafe { ::std::intrinsics::abort() } }
}

fn main() {
    let conn = &*match Some(ConnWrap(Conn)) {
        Some(val) => val,
        None => return,
    };
    return
}

Removing either of early returns in the function makes the issue go away. Seems like a scoping issue.

[Stebalien]

@brson regression-from-stable-to-nightly (for a two more days at least).

[brson]

Yeah I jumped the gun on the tag, but the beta is building right now.

[nikomatsakis]

@nagisa I saw your commit -- I feel like the extent should be unique for the cleanup scope. That is, we should push a given extent only once -- but perhaps I am wrong about this? It'd be good to add an assertion (or warning) for when we push cleanup scopes with the same extent more than once so we can see what it happens.

[nikomatsakis]

Ah, but reading IRC chatter I see that the problem is that the extent of the "outermost scope" changed and thus the extent_of_return_scope helper is out of date? That seems pretty plausible.

[nagisa]

As pointed out on the IRC the commit linked above is a shot in the dark and is not a fix.