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

librustc: lifetimes in values-with-dtors must outlive such values themselves #16154

Closed
wants to merge 1 commit into from

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Aug 1, 2014

librustc: Require lifetimes within the types of values with destructors to have strictly greater lifetime than the values themselves.

Additionally, remove #[unsafe_destructor] in favor of these new
restrictions.

This broke a fair amount of code that had to do with RefCells. We will
probably need to do some work to improve the resulting ergonomics. Most
code that broke looked like:

match foo.bar.borrow().baz { ... }
use_foo(&mut foo);

Change this code to:

{
    let bar = foo.bar.borrow();
    match bar.baz { ... }
}
use_foo(&mut foo);

This fixes an important memory safety hole relating to destructors,
represented by issue #8861.

[breaking-change]

r? @nikomatsakis

If you want I can talk about the followups to improve ergonomics of RefCell that I want to do, but this is the BC change.

@@ -1 +1 @@
Subproject commit 0d999e5b315b6ff78fcea772466d985ce53fd8dc
Subproject commit cd24b5c6633b27df2b84249a65a46a610b734494
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this by accident?

@alexcrichton
Copy link
Member

That's pretty awesome to see unsafe_destructor go away!

@sfackler
Copy link
Member

sfackler commented Aug 1, 2014

Has this been fixed in the meantime?

struct Foo<T> { a: T }

impl Drop for Foo<int> {
    fn drop(&mut self) {}
}

parent.module().external_module_children.borrow_mut()
.insert(name.name, external_module.clone());
self.build_reduced_graph_for_external_crate(external_module);
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't quite understand why all these indirections are needed, actually. It could be I'm just not thinking through the ramifications of the rule, but it seems like the rvalue with a dtor (the RefMut, presumably) is live for the enclosing statement. Therefore, it must only be able to reach lifetime points that outlive the statement (i.e., are live for the enclosing block). I feel like this condition should be met. If you are getting errors, it seems likely to be an error in inference or somewhere else.

@alexcrichton
Copy link
Member

ping @pcwalton, any updates on this?

@pcwalton
Copy link
Contributor Author

Niko and I are planning to go over this at the work week.

@sfackler
Copy link
Member

What's the status of this?

@pcwalton
Copy link
Contributor Author

@sfackler Rebasing now

to have strictly greater lifetime than the values themselves.

Additionally, remove `#[unsafe_destructor]` in favor of these new
restrictions.

This broke a fair amount of code that had to do with `RefCell`s. We will
probably need to do some work to improve the resulting ergonomics. Most
code that broke looked like:

    match foo.bar.borrow().baz { ... }
    use_foo(&mut foo);

Change this code to:

    {
        let bar = foo.bar.borrow();
        match bar.baz { ... }
    }
    use_foo(&mut foo);

This fixes an important memory safety hole relating to destructors,
represented by issue rust-lang#8861.

[breaking-change]
@alexcrichton
Copy link
Member

ping @pcwalton

@pnkfelix
Copy link
Member

i'm going to try to help shepherd this PR (or some variant of it) through.

I plan to keep my working area at: https://github.com/pnkfelix/rust/commits/new-dtor-semantics (e.g. I just finished factoring it into distinct commits to ease review.)

@pnkfelix pnkfelix changed the title librustc: Require lifetimes within the types of values with destructors librustc: lifetimes in values-with-dtors must outlive such values themselves Oct 27, 2014
@@ -458,9 +458,26 @@ impl Clean<TyParam> for ast::TyParam {
}

impl Clean<TyParam> for ty::TypeParameterDef {
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'll make sure to clean this up.)

@alexcrichton
Copy link
Member

Closing due to inactivity (and @bors's queue is quite full!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants