Add undo_leak to reset RefCell borrow state #69528

HeroicKatora · 2020-02-27T21:02:47Z

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 #68712 and uses the same tracking issue, #69099,
as these methods deal with the same mechanism and the idea came up
in a review comment.

@dtolnay who reviewed the prior PR.
cc @RalfJung

rust-highfive · 2020-02-27T21:02:52Z

r? @kennytm

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

HeroicKatora · 2020-02-27T21:03:04Z

r? @dtolnay

Mark-Simulacrum · 2020-02-27T21:30:25Z

I would have expected this to not take &mut self, but instead be more like force_unlock_read and force_unlock_write on lock_api/parking_lot's RwLock's.

HeroicKatora · 2020-02-27T22:17:51Z

This is a completely safe interface that fully resets the borrows. I just noticed that the name might be suboptimal as the implementation doesn't differentiate between shared and mut borrows and keeping the name unborrow might complicate naming future methods that do. So I'm open for ideas here.

The unsafe API for removing single read/write borrows, force_unock would be possibly but it is less clear to me. It might be better to have a to_raw/from_raw equivalent pair on both Ref and RefMut and then change the borrow state by the usual Drop. This would behave more similar to other types in std such as Box.

HeroicKatora · 2020-02-27T22:19:36Z

Other names that are possible and come to mind:

reset_borrows
reset
clear

rust-highfive · 2020-02-27T22:30:43Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-02-27T21:03:19.2140245Z ========================== Starting Command Output ===========================
2020-02-27T21:03:19.2144643Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/254f02f5-eb73-4b79-ac9f-312aa9f90caa.sh
2020-02-27T21:03:19.2145136Z 
2020-02-27T21:03:19.2152688Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-27T21:03:19.2179619Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69528/merge to s
2020-02-27T21:03:19.2183083Z Task         : Get sources
2020-02-27T21:03:19.2183405Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-27T21:03:19.2183722Z Version      : 1.0.0
2020-02-27T21:03:19.2183951Z Author       : Microsoft
---
2020-02-27T21:03:20.2140031Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-27T21:03:20.2148892Z ##[command]git config gc.auto 0
2020-02-27T21:03:20.2155049Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-27T21:03:20.2160851Z ##[command]git config --get-all http.proxy
2020-02-27T21:03:20.2171350Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69528/merge:refs/remotes/pull/69528/merge
---
2020-02-27T22:02:49.7676259Z .................................................................................................... 1700/9733
2020-02-27T22:02:54.0665979Z .................................................................................................... 1800/9733
2020-02-27T22:03:05.3830517Z ....................................................................i............................... 1900/9733
2020-02-27T22:03:12.2365109Z .................................................................................................... 2000/9733
2020-02-27T22:03:26.7518765Z ..........................................................iiiii..................................... 2100/9733
2020-02-27T22:03:37.0693080Z .................................................................................................... 2300/9733
2020-02-27T22:03:39.3017178Z .................................................................................................... 2400/9733
2020-02-27T22:03:42.3271685Z .................................................................................................... 2500/9733
2020-02-27T22:04:03.2841205Z .................................................................................................... 2600/9733
---
2020-02-27T22:06:35.6392003Z ..................i...............i................................................................. 5000/9733
2020-02-27T22:06:44.8909908Z .................................................................................................... 5100/9733
2020-02-27T22:06:50.2589074Z .............................................................i...................................... 5200/9733
2020-02-27T22:06:56.5280335Z .................................................................................................... 5300/9733
2020-02-27T22:07:04.9451278Z ......................................ii.ii........i...i............................................ 5400/9733
2020-02-27T22:07:13.2591071Z .................................................................................................... 5600/9733
2020-02-27T22:07:22.3272776Z .................................................................................................... 5700/9733
2020-02-27T22:07:28.9737261Z .............................i...................................................................... 5800/9733
2020-02-27T22:07:34.8732968Z .................................................................................................... 5900/9733
2020-02-27T22:07:34.8732968Z .................................................................................................... 5900/9733
2020-02-27T22:07:45.5212657Z .................................................................................................... 6000/9733
2020-02-27T22:07:54.9975142Z ....................ii...i..ii...........i.......................................................... 6100/9733
2020-02-27T22:08:10.4642128Z .................................................................................................... 6300/9733
2020-02-27T22:08:14.0616754Z .................................................................................................... 6400/9733
2020-02-27T22:08:14.0616754Z .................................................................................................... 6400/9733
2020-02-27T22:08:23.3390161Z ...................................................i..ii............................................ 6500/9733
2020-02-27T22:08:46.3464100Z .................................................................................................... 6700/9733
2020-02-27T22:08:48.5634227Z ...........................................i........................................................ 6800/9733
2020-02-27T22:08:50.6270281Z .................................................................................................... 6900/9733
2020-02-27T22:08:52.7688189Z .........................................................................i.......................... 7000/9733
---
2020-02-27T22:10:26.7707358Z .................................................................................................... 7700/9733
2020-02-27T22:10:31.7598562Z .................................................................................................... 7800/9733
2020-02-27T22:10:36.5366614Z .................................................................................................... 7900/9733
2020-02-27T22:10:44.2500605Z ..................i................................................................................. 8000/9733
2020-02-27T22:10:52.6379628Z ...................................................................iiiiiii.i........................ 8100/9733
2020-02-27T22:11:07.2405742Z ........i......i.................................................................................... 8300/9733
2020-02-27T22:11:12.3144573Z .................................................................................................... 8400/9733
2020-02-27T22:11:25.0152054Z .................................................................................................... 8500/9733
2020-02-27T22:11:34.0682543Z .................................................................................................... 8600/9733
---
2020-02-27T22:13:50.7647954Z  finished in 7.314
2020-02-27T22:13:50.7846333Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-27T22:13:50.9437325Z 
2020-02-27T22:13:50.9438232Z running 178 tests
2020-02-27T22:13:53.7107011Z iiii......i...........ii..iiii...i....i...........i............i..i..................i....i......... 100/178
2020-02-27T22:13:55.8665039Z ...i.i.i...iii..iiiiiiiiiiiiiiii.......................iii............ii......
2020-02-27T22:13:55.8670882Z 
2020-02-27T22:13:55.8671060Z  finished in 5.082
2020-02-27T22:13:55.8855496Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-27T22:13:56.0407969Z 
---
2020-02-27T22:13:57.8646786Z  finished in 1.978
2020-02-27T22:13:57.8834709Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-27T22:13:58.0375483Z 
2020-02-27T22:13:58.0376856Z running 9 tests
2020-02-27T22:13:58.0379030Z iiiiiiiii
2020-02-27T22:13:58.0380118Z 
2020-02-27T22:13:58.0380276Z  finished in 0.154
2020-02-27T22:13:58.0575323Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-27T22:13:58.2155127Z 
---
2020-02-27T22:14:17.1968763Z  finished in 19.131
2020-02-27T22:14:17.2088621Z Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-27T22:14:17.3639985Z 
2020-02-27T22:14:17.3640312Z running 116 tests
2020-02-27T22:14:30.5878197Z iiiii..i.....i..i...i..i.i.i..i..i..ii....i.i....ii..........iiii..........i.....i..i.......ii.i.ii. 100/116
2020-02-27T22:14:32.4465197Z ....iiii.....ii.
2020-02-27T22:14:32.4466745Z 
2020-02-27T22:14:32.4469406Z  finished in 15.238
2020-02-27T22:14:32.4476278Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-02-27T22:14:32.4477066Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2020-02-27T22:26:44.9786036Z 
2020-02-27T22:26:44.9786996Z    Doc-tests core
2020-02-27T22:26:49.3380097Z 
2020-02-27T22:26:49.3380992Z running 2481 tests
2020-02-27T22:26:57.9471063Z ......iiiii.........................................................F............................... 100/2481
2020-02-27T22:27:05.9954893Z .....................................................................................ii............. 200/2481
2020-02-27T22:27:25.1871312Z ....................i............................................................................... 400/2481
2020-02-27T22:27:25.1871312Z ....................i............................................................................... 400/2481
2020-02-27T22:27:34.2885361Z .........................................................................i..i..................iiii. 500/2481
2020-02-27T22:27:49.4453172Z .................................................................................................... 700/2481
2020-02-27T22:27:57.3040918Z .................................................................................................... 800/2481
2020-02-27T22:28:05.1632110Z .................................................................................................... 900/2481
2020-02-27T22:28:13.1096590Z .................................................................................................... 1000/2481
---
2020-02-27T22:30:08.5565951Z .....................................................................................i.............. 2400/2481
2020-02-27T22:30:15.2479401Z ....................i............................................................
2020-02-27T22:30:15.2480884Z failures:
2020-02-27T22:30:15.2484670Z 
2020-02-27T22:30:15.2486500Z ---- cell.rs - cell::RefCell<T>::unborrow (line 969) stdout ----
2020-02-27T22:30:15.2488664Z error[E0599]: no method named `is_err` found for struct `std::cell::Ref<'_, {integer}>` in the current scope
2020-02-27T22:30:15.2490116Z   --> cell.rs:976:20
2020-02-27T22:30:15.2490781Z    |
2020-02-27T22:30:15.2491549Z 10 | assert!(c.borrow().is_err());
2020-02-27T22:30:15.2492669Z    |                    ^^^^^^ method not found in `std::cell::Ref<'_, {integer}>`
2020-02-27T22:30:15.2493281Z error: aborting due to previous error
2020-02-27T22:30:15.2569985Z 
2020-02-27T22:30:15.2571316Z For more information about this error, try `rustc --explain E0599`.
2020-02-27T22:30:15.2572500Z Couldn't compile the test.
---
2020-02-27T22:30:15.2774976Z   local time: Thu Feb 27 22:30:15 UTC 2020
2020-02-27T22:30:15.8193111Z   network time: Thu, 27 Feb 2020 22:30:15 GMT
2020-02-27T22:30:15.8193661Z == end clock drift check ==
2020-02-27T22:30:16.3658669Z 
2020-02-27T22:30:16.3727646Z ##[error]Bash exited with code '1'.
2020-02-27T22:30:16.3742381Z ##[section]Finishing: Run build
2020-02-27T22:30:16.3793629Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69528/merge to s
2020-02-27T22:30:16.3798936Z Task         : Get sources
2020-02-27T22:30:16.3799300Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-27T22:30:16.3799656Z Version      : 1.0.0
2020-02-27T22:30:16.3799893Z Author       : Microsoft
2020-02-27T22:30:16.3799893Z Author       : Microsoft
2020-02-27T22:30:16.3800278Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-27T22:30:16.3800723Z ==============================================================================
2020-02-27T22:30:16.7155062Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-27T22:30:16.7209643Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69528/merge to s
2020-02-27T22:30:16.7298585Z Cleaning up task key
2020-02-27T22:30:16.7299916Z Start cleaning up orphan processes.
2020-02-27T22:30:16.7486503Z Terminate orphan process: pid (3599) (python)
2020-02-27T22:30:16.7720088Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2020-02-27T22:30:57Z

I'm not really opposed to this interface but I personally suspect almost everyone will want the raw unsafe API rather than this.

RalfJung · 2020-02-27T22:33:59Z

Maybe emphasize in the PR description that this allows safely reverting the effects of leaking a borrow.

dtolnay

I am on board with adding a safe unborrow API but we will need to pay attention during stabilization that we have been able to identify some concrete examples where using this API is the best solution.

The most important consideration when adding niche methods is how to keep them from being a distraction when people are looking for the non-niche behaviors they care about. I think unborrow isn't great in that aspect. Instead I would expect something like:

fn undo_leak(&mut self);

// later, if needed:
unsafe fn force_undo_leak(&self);

From these names it is clear that if someone isn't planning on leaking refs then these methods are not going to be relevant to them.

This method is complementary for the feature refcell_leak added in an earlier PR. It allows 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.

HeroicKatora · 2020-03-05T17:06:20Z

The name suggestion seems very clearly motivated and convincing to me, so I changed the PR.

HeroicKatora · 2020-03-14T13:10:59Z

The commit above my comment addresses the review (I think), I'm not sure why this is now tagged as S-waiting-on-author?

RalfJung · 2020-03-14T13:45:44Z

@dtolnay this is awaiting your (re-)review.

RalfJung · 2020-03-14T13:47:01Z

src/libcore/cell.rs

+    /// assert!(c.try_borrow().is_ok());
+    /// ```
+    #[unstable(feature = "cell_leak", issue = "69099")]
+    pub fn undo_leak(&mut self) -> &mut T {


What is the motivation for returning a reference here? The example does not even make use of that.

I had previously seen it as a strengthened version of get_mut. It could be fine fully separating the two methods tough.

dtolnay

Looks good!

dtolnay · 2020-03-15T04:50:35Z

@bors r+

bors · 2020-03-15T04:50:37Z

📌 Commit 51b9396 has been approved by dtolnay

@ghost

Rollup of 8 pull requests Successful merges: - #69528 (Add undo_leak to reset RefCell borrow state) - #69589 (ast: `Mac`/`Macro` -> `MacCall`) - #69661 (Implement From<&mut str> for String) - #69988 (rustc_metadata: Remove `rmeta::MacroDef`) - #70006 (resolve: Fix two issues in fresh binding disambiguation) - #70011 (def_collector: Fully visit async functions) - #70013 (Return feature gate as a `Symbol` ) - #70018 (Fix "since" field for `Once::is_complete`'s `#[stable]` attribute) Failed merges: r? @ghost

rust-highfive assigned kennytm Feb 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2020

rust-highfive assigned dtolnay and unassigned kennytm Feb 27, 2020

This comment has been minimized.

Sign in to view

HeroicKatora force-pushed the finalize-ref-cell branch from 2c9ff08 to dbd9847 Compare February 28, 2020 17:47

dtolnay requested changes Mar 4, 2020

View reviewed changes

HeroicKatora force-pushed the finalize-ref-cell branch from dbd9847 to 51b9396 Compare March 4, 2020 19:46

HeroicKatora changed the title ~~Add unborrow to reset RefCell borrow state~~ Add undo_leak to reset RefCell borrow state Mar 6, 2020

JohnCSimon 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 Mar 11, 2020

RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2020

RalfJung reviewed Mar 14, 2020

View reviewed changes

dtolnay approved these changes Mar 15, 2020

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2020

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 15, 2020

Centril mentioned this pull request Mar 15, 2020

Rollup of 8 pull requests #70024

Merged

bors merged commit 83aad6b into rust-lang:master Mar 15, 2020

HeroicKatora deleted the finalize-ref-cell branch March 15, 2020 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add undo_leak to reset RefCell borrow state #69528

Add undo_leak to reset RefCell borrow state #69528

HeroicKatora commented Feb 27, 2020 •

edited

Loading

rust-highfive commented Feb 27, 2020

HeroicKatora commented Feb 27, 2020

Mark-Simulacrum commented Feb 27, 2020

HeroicKatora commented Feb 27, 2020 •

edited

Loading

HeroicKatora commented Feb 27, 2020 •

edited

Loading

rust-highfive commented Feb 27, 2020

Mark-Simulacrum commented Feb 27, 2020

RalfJung commented Feb 27, 2020

This comment has been minimized.

dtolnay left a comment

HeroicKatora commented Mar 5, 2020

HeroicKatora commented Mar 14, 2020

RalfJung commented Mar 14, 2020

RalfJung Mar 14, 2020

HeroicKatora Mar 14, 2020 •

edited

Loading

dtolnay left a comment

dtolnay commented Mar 15, 2020

bors commented Mar 15, 2020

Add undo_leak to reset RefCell borrow state #69528

Add undo_leak to reset RefCell borrow state #69528

Conversation

HeroicKatora commented Feb 27, 2020 • edited Loading

rust-highfive commented Feb 27, 2020

HeroicKatora commented Feb 27, 2020

Mark-Simulacrum commented Feb 27, 2020

HeroicKatora commented Feb 27, 2020 • edited Loading

HeroicKatora commented Feb 27, 2020 • edited Loading

rust-highfive commented Feb 27, 2020

Mark-Simulacrum commented Feb 27, 2020

RalfJung commented Feb 27, 2020

This comment has been minimized.

dtolnay left a comment

Choose a reason for hiding this comment

HeroicKatora commented Mar 5, 2020

HeroicKatora commented Mar 14, 2020

RalfJung commented Mar 14, 2020

RalfJung Mar 14, 2020

Choose a reason for hiding this comment

HeroicKatora Mar 14, 2020 • edited Loading

Choose a reason for hiding this comment

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Mar 15, 2020

bors commented Mar 15, 2020

HeroicKatora commented Feb 27, 2020 •

edited

Loading

HeroicKatora commented Feb 27, 2020 •

edited

Loading

HeroicKatora commented Feb 27, 2020 •

edited

Loading

HeroicKatora Mar 14, 2020 •

edited

Loading