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

Add Box::leak<'a>(Box<T>) -> &'a mut T where T: 'a #45881

Merged
merged 7 commits into from
Nov 23, 2017

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 8, 2017

Adds:

impl<T: ?Sized> Box<T> {
    pub fn leak<'a>(b: Box<T>) -> &'a mut T where T: 'a {
        unsafe { &mut *Box::into_raw(b) }
    }
}

which is useful for when you just want to put some stuff on the heap and then have a reference to it for the remainder of the program.

r? @sfackler
cc @durka

@Centril Centril changed the title Add Box::leak(Box<T>) -> &'static T Add Box::leak(Box<T>) -> &'static mut T Nov 8, 2017
@durka
Copy link
Contributor

durka commented Nov 8, 2017

N.B. this exists in the ecosystem: https://crates.io/crates/leak

Is &'static mut T useful? It's unsafe to do anything with that, right? Maybe &'static T is better.

@mattico
Copy link
Contributor

mattico commented Nov 8, 2017

See also: rust-lang/rfcs#1233

@oli-obk
Copy link
Contributor

oli-obk commented Nov 8, 2017

It's not unsafe. There's a single mutable reference, the lifetime is irrelevant

@Centril
Copy link
Contributor Author

Centril commented Nov 8, 2017

Hmm, so @alexcrichton said then in rust-lang/rfcs#1233:

While this is indeed a pattern that could be added to the standard library, it's currently not greatly motivated in terms of use cases that require this sort of functionality to be in the standard library itself.

Since then the leak crate has been used: 12,959 times, but has one immediate dependent crate.

@cramertj
Copy link
Member

cramertj commented Nov 8, 2017

Since then the leak crate has been used: 12,959 times, but has one immediate dependent crate.

And that crate is transitively depended on by skia and servo-skia-- I'd be that's where many of the downloads are coming from.

@Centril
Copy link
Contributor Author

Centril commented Nov 8, 2017

@cramertj So I can't judge if this means that the feature is used very little or not... =)

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 9, 2017
@sfackler
Copy link
Member

sfackler commented Nov 9, 2017

I think this is a nice addition. &'static mut T seems like the right type, but 'static mutability is a bit subtle. @alexcrichton do you know if this'll run into any safety issues?

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 9, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 9, 2017
@alexcrichton
Copy link
Member

AFAIK there's no soundness holes from this, but we can always double check during stabilization!

@kennytm kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Nov 9, 2017
@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Nov 9, 2017

I wonder if there is no risk of BC break in event this function would be replaced with this. It may turn out that the API may be expanded in the future to allow other lifetimes than 'static.

fn leak<'a>(b: Box<T>) -> &'a mut T
where
    T: 'a,
{
    unsafe { &mut *Box::into_raw(b) }
}

@retep998
Copy link
Member

retep998 commented Nov 9, 2017

I have two points that people may be concerned about and it would be nice to have clear answers about them:

  • What if T is not 'static? Will the returned &mut 'static T still respect the lifetime of T itself?
  • What happens when Box gains the ability to be parametric over the allocator and that allocator has a lifetime so when the allocator dies, it takes all its allocations with it including any "leaked" allocations.

@Centril
Copy link
Contributor Author

Centril commented Nov 9, 2017

@xfix So; talked it over on #rust-lang and it seems strictly more powerful - so I'll change it to your proposal fn leak<'a, T: 'a>(b: Box<T>) -> &'a mut T instead, if @sfackler agrees. In any case, 'static should be backwards-compatible since you may pick any lifetime you wish for 'a including 'static.

@retep998

What if T is not 'static? Will the returned &mut 'static T still respect the lifetime of T itself?

Given this program:

fn leak<T>(b: Box<T>) -> &'static mut T {
    unsafe { &mut *Box::into_raw(b) }
}

fn main() {
    let static_ref: &'static mut &mut usize = {
        let mut local_variable = 41;
        let boxed: Box<&mut usize> = Box::new(&mut local_variable);
        leak(boxed)
    };
    **static_ref += 1;
    assert_eq!(**static_ref, 42);
}

we, rightly, get:

error[E0597]: `local_variable` does not live long enough

So a bound T: 'static is magically there.

What happens when Box gains the ability to be parametric over the allocator...

Well, then leak does not exist for that Box<T, Alloc> yet, but can be defined later by Alloc: 'a.

@Centril
Copy link
Contributor Author

Centril commented Nov 9, 2017

Updated to fn leak<'a, T: 'a>(b: Box<T>) -> &'a mut T after OKing with @sfackler.

@Centril Centril changed the title Add Box::leak(Box<T>) -> &'static mut T Add Box::leak<'a>(Box<T>) -> &'a mut T where T: 'a Nov 12, 2017
@carols10cents
Copy link
Member

ping @BurntSushi for your ticky box here!

///
/// # Examples
///
/// Simple usage:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a documentation line simply saying "Simple usage" before an example needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used it to differentiate between "simple usage" and "unsized data" - I think it makes it a bit more readable - but I can always remove it.

@BurntSushi
Copy link
Member

If 'static is there implicitly, does it make sense to make it explicit with T: 'static in the function signature?

@rfcbot
Copy link

rfcbot commented Nov 15, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 15, 2017
@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 15, 2017
@Centril
Copy link
Contributor Author

Centril commented Nov 15, 2017

@BurntSushi I generalized it to any lifetime 'a and made it explicit after popular demand, so yes =)

@BurntSushi
Copy link
Member

@Centril Ah great!

@aturon aturon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 21, 2017
@alexcrichton
Copy link
Member

@Centril mind updating the issue number reference in the unstable tag? After that this should be good to go!

@Centril
Copy link
Contributor Author

Centril commented Nov 22, 2017

@alexcrichton Done, issue is #46179 .

@kennytm
Copy link
Member

kennytm commented Nov 23, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Nov 23, 2017

📌 Commit bc18d99 has been approved by alexcrichton

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2017
@bors
Copy link
Contributor

bors commented Nov 23, 2017

⌛ Testing commit bc18d99 with merge 246a6d1...

bors added a commit that referenced this pull request Nov 23, 2017
Add Box::leak<'a>(Box<T>) -> &'a mut T where T: 'a

Adds:

```rust
impl<T: ?Sized> Box<T> {
    pub fn leak<'a>(b: Box<T>) -> &'a mut T where T: 'a {
        unsafe { &mut *Box::into_raw(b) }
    }
}
```

which is useful for when you just want to put some stuff on the heap and then have a reference to it for the remainder of the program.

r? @sfackler
cc @durka
@bors
Copy link
Contributor

bors commented Nov 23, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 246a6d1 to master...

@bors bors merged commit bc18d99 into rust-lang:master Nov 23, 2017
@Centril Centril deleted the box-leak branch November 29, 2017 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.