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 methods to 'leak' RefCell borrows as references with the lifetime of the original reference #68712

Merged
merged 3 commits into from
Feb 26, 2020

Conversation

HeroicKatora
Copy link
Contributor

@HeroicKatora HeroicKatora commented Jan 31, 2020

Usually, references to the interior are only created by the Deref and
DerefMut impl of the guards Ref and RefMut. Note that RefCell
already has to cope with leaks of such guards which, when it occurs,
effectively makes it impossible to ever acquire a mutable guard or any
guard for Ref and RefMut respectively. It is already safe to use
this to create a reference to the inner of the ref cell that lives as
long as the reference to the RefCell itself, e.g.

fn leak(r: &RefCell<usize>) -> Option<&usize> {
    let guard = r.try_borrow().ok()?;
    let leaked = Box::leak(Box::new(guard));
    Some(&*leaked)
}

The newly added methods allow the same reference conversion without an
indirection over a leaked allocation. It's placed on the Ref/RefMut to
compose with both borrow and try_borrow directly.

Usually, references to the interior are only created by the `Deref` and
`DerefMut` impl of the guards `Ref` and `RefMut`. Note that `RefCell`
already has to cope with leaks of such guards which, when it occurs,
effectively makes it impossible to ever acquire a mutable guard or any
guard for `Ref` and `RefMut` respectively. It is already safe to use
this to create a reference to the inner of the ref cell that lives as
long as the reference to the `RefCell` itself, e.g.

```rust
fn leak(r: &RefCell<usize>) -> Option<&usize> {
    let guard = r.try_borrow().ok()?;
    let leaked = Box::leak(Box::new(guard));
    Some(&*leaked)
}
```

The newly added methods allow the same reference conversion without an
indirection over a leaked allocation and composing with both borrow and
try_borrow without additional method combinations.
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2020
@Dylan-DPC-zz
Copy link

r? @dtolnay

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Neat!

@dtolnay
Copy link
Member

dtolnay commented Feb 11, 2020

Could you please make a tracking issue and add in the issue number?

@Mark-Simulacrum
Copy link
Member

Should we add similar functions to MutexGuard and RwLockReadGuard/RwLockWriteGuard?

I would also suggest adding forceful "deborrow" functions like parking_lot's https://docs.rs/lock_api/0.3.3/lock_api/struct.Mutex.html#method.force_unlock to accompany this API, but that can be done later, and potentially as a separate feature gate.

@wirelessringo
Copy link

Ping from triage. @HeroicKatora any updates on this? Thanks.

@wirelessringo wirelessringo added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2020
src/libcore/cell.rs Outdated Show resolved Hide resolved
src/libcore/cell.rs Outdated Show resolved Hide resolved
@HeroicKatora
Copy link
Contributor Author

I've reworded the sections in question. The decision of MutexGuard and forceful deborrow seems like it should be provided in a separate PR.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for including the MutexGuard, RwLockReadGuard/RwLockWriteGuard, and forceful deborrow suggestions in the tracking issue.

@dtolnay
Copy link
Member

dtolnay commented Feb 24, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2020

📌 Commit 329022d has been approved by dtolnay

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 25, 2020
…tolnay

Add methods to 'leak' RefCell borrows as references with the lifetime of the original reference

Usually, references to the interior are only created by the `Deref` and
`DerefMut` impl of the guards `Ref` and `RefMut`. Note that `RefCell`
already has to cope with leaks of such guards which, when it occurs,
effectively makes it impossible to ever acquire a mutable guard or any
guard for `Ref` and `RefMut` respectively. It is already safe to use
this to create a reference to the inner of the ref cell that lives as
long as the reference to the `RefCell` itself, e.g.

```rust
fn leak(r: &RefCell<usize>) -> Option<&usize> {
    let guard = r.try_borrow().ok()?;
    let leaked = Box::leak(Box::new(guard));
    Some(&*leaked)
}
```

The newly added methods allow the same reference conversion without an
indirection over a leaked allocation. It's placed on the `Ref`/`RefMut` to
compose with both borrow and try_borrow directly.
bors added a commit that referenced this pull request Feb 26, 2020
Rollup of 5 pull requests

Successful merges:

 - #68712 (Add methods to 'leak' RefCell borrows as references with the lifetime of the original reference)
 - #69209 (Miscellaneous cleanup to formatting)
 - #69381 (Allow getting `no_std` from the config file)
 - #69434 (rustc_metadata: Use binary search from standard library)
 - #69447 (Minor refactoring of statement parsing)

Failed merges:

r? @ghost
#[unstable(feature = "cell_leak", issue = "69099")]
pub fn leak(orig: Ref<'b, T>) -> &'b T {
// By forgetting this Ref we ensure that the borrow counter in the RefCell never goes back
// to UNUSED again. No further mutable references can be created from the original cell.
Copy link
Member

Choose a reason for hiding this comment

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

"No further mutable references" is not entirely correct -- there is get_mut which circumvents the borrow counter. (If we felt fancy, we could even make it reset the borrow counter.)

Copy link
Contributor Author

@HeroicKatora HeroicKatora Feb 26, 2020

Choose a reason for hiding this comment

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

The comment would be more accurate if it were along the lines of 'no further RefMut can be created'. That said, having a reset_borrows(&mut self) would work as well and then the comment would become outdated. Since the method seems so natural, I'll try to come up with a PR in the next days and fix this comment then.

@bors bors merged commit 86b9377 into rust-lang:master Feb 26, 2020
Centril added a commit to Centril/rust that referenced this pull request Mar 15, 2020
…tolnay

Add undo_leak to reset RefCell borrow state

This method is complementary for the feature cell_leak added in an
earlier PR. It allows *safely* reverting the effects of leaking a borrow guard by
statically proving that such a guard could not longer exist. This was
not added to the existing `get_mut` out of concern of impacting the
complexity of the otherwise pure pointer cast and because the name
`get_mut` poorly communicates the intent of resetting remaining borrows.

This is a follow-up to rust-lang#68712 and uses the same tracking issue, rust-lang#69099,
as these methods deal with the same mechanism and the idea came up
[in a review comment](rust-lang#68712 (comment)).

@dtolnay who reviewed the prior PR.
cc @RalfJung
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants