Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Member destructor is always executed even if self is replaced in Drop #23611

Closed
nwin opened this issue Mar 22, 2015 · 6 comments · Fixed by #24765
Closed

Member destructor is always executed even if self is replaced in Drop #23611

nwin opened this issue Mar 22, 2015 · 6 comments · Fixed by #24765
Assignees
Labels
A-destructors Area: destructors (Drop, ..) A-typesystem Area: The type system
Milestone

Comments

@nwin
Copy link
Contributor

nwin commented Mar 22, 2015

The following code prints "Dropping!" although the corresponding variant had been removed and forgotten. Same happens if it is overwritten instead.

use std::mem;
use std::ptr;

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

enum Foo {
    Bar(Noisy),
    Baz
}
use Foo::*;

impl Drop for Foo {
    fn drop(&mut self) {
        match *self {
            Bar(_) => {
                //unsafe { ptr::write(self, Foo::Baz) }
                unsafe { mem::forget(mem::replace(self, Foo::Baz)) }
            }
            Baz => ()
        }
    }
}

fn main() {
    let _ = Foo::Bar(Noisy);
}

I’m not sure if this is a bug or intended behavior but it is not obvious to me that this would happen and might be documented in more detail.

@sfackler
Copy link
Member

cc @pnkfelix

@steveklabnik steveklabnik added the A-typesystem Area: The type system label Mar 22, 2015
@steveklabnik
Copy link
Member

(a bit of a strange tag, but I wasn't 100% sure what to put this under. kinda works, but may need recategorized once we pin down what's up here)

@nwin
Copy link
Contributor Author

nwin commented Mar 22, 2015

I just checked, the destructor is run also on the wrong (the new replaced one) value. The following code prints „Dropping deadbeef“

use std::mem;
use std::ptr;

struct Noisy(usize);
impl Drop for Noisy {
    fn drop(&mut self) { 
        println!("Dropping {:x}.", self.0)
    }
}

enum Foo {
    Bar(Noisy),
    Baz(usize)
}
use Foo::*;

impl Drop for Foo {
    fn drop(&mut self) {
        match *self {
            Bar(_) => {
                unsafe { mem::forget(mem::replace(self, Foo::Baz(0xdeadbeef))) }
            }
            Baz(_) => ()
        }
    }
}

fn main() {
    let _ = Foo::Bar(Noisy(0));
}

@pnkfelix pnkfelix added the A-destructors Area: destructors (Drop, ..) label Mar 22, 2015
@pnkfelix
Copy link
Member

hmm, I assume this has something to do with the drop glue assuming that the drop implementation will not change which variant of enum we are looking at.

I'm not exactly sure what the best thing to do is. Ideally we would ensure the drop method would not be able to change the enum variant in this fashion.

Nominating for discussion at triage meeting. Probably an I-needs-decision; probably can be decided by 1.0 (i.e. need not be resolved by the beta release itself.)

@pnkfelix
Copy link
Member

Current plan: Change the drop glue to inspect the discriminant after the destructor runs.

1.0 polish, P-backcompat-lang.

@pnkfelix pnkfelix added this to the 1.0 milestone Mar 26, 2015
@pnkfelix pnkfelix self-assigned this Mar 29, 2015
@pnkfelix
Copy link
Member

(i think i have a plausible patch for this worked out.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 24, 2015
without invoking the Drop::drop implementation.

This is necessary for dealing with an enum that switches own `self` to
a different variant while running its destructor.

Fix rust-lang#23611.
bors added a commit that referenced this issue Apr 27, 2015
Inspect enum discriminant *after* calling its destructor

Includes some drive-by cleanup (e.g. changed some field and method names to reflect fill-on-drop; added comments about zero-variant enums being classified as `_match::Single`).

Probably the most invasive change was the expansion of the maps `available_drop_glues` and `drop_glues` to now hold two different kinds of drop glues; there is the (old) normal drop glue, and there is (new) drop-contents glue that jumps straight to dropping the contents of a struct or enum, skipping its destructor.

 * For all types that do not have user-defined Drop implementations, the normal glue is generated as usual (i.e. recursively dropping the fields of the data structure).

  (And this actually is exactly what the newly-added drop-contents glue does as well.)

 * For types that have user-defined Drop implementations, the "normal" drop glue now schedules a cleanup before invoking the `Drop::drop` method that will call the drop-contents glue after that invocation returns.

Fix #23611.

----

Is this a breaking change?  The prior behavior was totally unsound, and it seems unreasonable that anyone was actually relying on it.

Nonetheless, since there is a user-visible change to the language semantics, I guess I will conservatively mark this as a:

[breaking-change]

(To see an example of what sort of user-visible change this causes, see the comments in the regression test.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: destructors (Drop, ..) A-typesystem Area: The type system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants