Tracking issue for borrow_state stabilization

[aturon]

The RefCell API offers a way to determine the state of the borrow, to avoid panics on additional borrowing. It's not clear how useful this API is in practice, but it's otherwise ready for stabilization.

---

EDIT: @KodrAus

Before stabilization:

• Add a concrete, motivating example to the documentation Introduce RefCell::try_borrow_unguarded #59211 (comment)

[sfackler]

I believe @nikomatsakis has mentioned that he's found a use for these methods at some point, but I can't remember what those uses were off the top of my head.

[nikomatsakis]

I've never used this pattern in Rust, but in previous lives I've been in the situation where I had a graph that was supposed to be acyclic. I was walking the graph, acquiring locks on the nodes as I went. This should not result in deadlock. The problem was that the graph was constructed from untrusted user input, and hence a deadlock was possible if the input was in error. This was neatly solved by using a 'try lock' structure, such that whenever we failed to acquire a lock, we could infer that the graph was potentially cyclic (and, if so, report an error). I can easily imagine a similar scenario arising with RefCell.

[alexcrichton]

This issue is now entering its cycle-long FCP for stabilization in 1.5

[Gankra]

I argued in the meeting today that we should instead opt to break the fallible/infallible convention and just have try_ versions of the borrow methods.

General ambivalence about this problem. This will remain unstable for at least one more release.

[alexcrichton]

The libs team discussed this during triage today and the decision was to punt to a future FCP.

[nikomatsakis]

I find the "borrow-state" version rather annoying to use in practice. For
example, the other day I was writing a Debug impl and I wanted to grab a
refcell for reading -- but it's just a Debug impl, so I didn't want to
cause a panic by doing so. Checking against borrow_state was annoying
enough to write that I decided to just call borrow instead because I
didn't think it likely to cause a problem. try_borrow would have been
just what I wanted.

On Wed, Oct 21, 2015 at 5:06 PM, Alex Crichton notifications@github.com
wrote:

The libs team discussed this during triage today and the decision was to
punt to a future FCP.

—
Reply to this email directly or view it on GitHub
#27733 (comment).

[SimonSapin]

+1 for one method returning Option<T> instead of two methods returning bool and T. It is generally an idiomatic pattern. (BorrowState is slightly more than bool, but the same idea applies.)

[Gankra]

So to be clear here, the issue at hand is that we long ago settled on a simple convention to avoid combinatoric bloat:

Either you expose a panicking version, or you expose a checked version. Not both.

This stance is strongly championed by @alexcrichton and @aturon, and for good reason! I don't want to live in a world with Vec::try_insert, try_remove, try_swap_remove, ...etc.

However I maintain that all good conventions exist to be broken! Most obviously, we break this convention with indexing (and soon, slicing!). Part of this is that indexing and slicing are so fundamental that they deserve several ways to do them. Indexing in particular has 4:

• get_unchecked
• get
• [ ]
• iter

At the heart of this convention is "decide if this is panicky or should be checked". For most interfaces we deem that it should be checked, because that's the strictly more general operation. There are two reasons to use panciky ops:

• The property to uphold can be trivially checked in a pinch.
• Everyone expects the op to always succeed and/or would have nothing to do if it didn't. (it would just always be unwrap'd)

Index-based ops like insert/remove have both of these properties, and are therefore a great candidate for panicky ops.

RefCell only has the second condition. It would be an ergonomic nightmare to have to unwrap ever borrow, so we went for panicky. However there's no way to introspect its state to check in a pinch.

In deference to The Convention, the original solution was to make it so it could be trivially checked, but the problem here is that checking is totally useless unless you're about to get (unlike looking at the len of an array). Therefore, I believe it makes the most sense to break convention and just provide a checked version of the op.

[nikomatsakis]

@gankro I definitely think try variants would be preferable to borrow_state. In general, I think that the decision to "expose the state you need to check whether an operation is legal" versus offering "try" variants is a tricky one (if there was one main take-away from our attempts to make a rule about panic-vs-check, it is that there is no one rule that really works in all cases). Exposing state has a lot of downsides:

1. It doesn't always work (e.g., locks and channels, where the check must be performed concurrently with the operation itself).
2. It is not very DRY. The caller has to "re-code" the logic as to when the operation is legal. e.g., I have to write borrow_state() != BorrowState::Write instead of try_read, and thus I wind up encoding that "the Write state is the only way to fail to read".
3. For the same reason, it is not efficient, as the check must be repeated both outside and inside the operation.
4. Again for the same reason, it can be less forward compatible, as it exposes internal state that might otherwise be opaque (like the state of the RefCell).
5. Ergonomically, it is often just less good. It's nice to be able to do things like if let Some(value) = cell.try_read(), for example.

The downside of "try" variants, of course, is just that you need to have one per function, typically, whereas you only need one method to expose the state.

I wanted to expand on point 4, about forwards compatibility. I could imagine wanting to extend RefCell in the future with new states: for example, we might want to add a take method that lets you swap the value out (I have frequently wanted this; I know you can do it with RefCell<Option<T>> but it's annoying and inefficient). In that case, there would be a new state (Taken), but that would be a breaking change, so it is ruled out. In contrast, if we just had try_read, there'd be no problem.

Another way to address concerns 2 and 4 (but not 3) by making the borrow-state opaque and have it offer methods like can_read, can_write, etc. That'd also improve ergonomics, as if cell.borrow_state().can_read() is better than what we have today (no imports required!). But it just seems like putting principle over practical, as you still need one method per operation, and returning Option is strictly better imo.

[alexcrichton]

To be clear, I'm not advocating that borrow_state is more ergonomic than try_borrow, to me that's not the issue at hand. I would far prefer to have an ironclad convention that foo and try_foo are never provided for situations like this. This sort of convention means you never need to ask yourself "do I have both?", it's simply always a matter of "which do I choose?" The answer to that question is the result-returning version 99% of the time as well because it's quite rare to have a case like RefCell where it's only expected to fail in terms of a programmer error. These simple conventions, though, (and sticking to them), I personally believe is far more of a gain over a slightly-more-ergonomic alternative here which is incredibly rarely called.

I see @gankro's slicing/indexing examples above as separate because they all deal with Index, which is documented as "panicking" (e.g. the implementation on maps). All methods related to indexing never panic.

@nikomatsakis I definitely agree with many of your points, and it's why this is an extreme case for RefCell. When "check the state" doesn't always work (like with channels), nothing panics and it all instead returns a result. I would be surprised if optimized code were actually less efficient, certainly the codegen is not as good, but the key part to remember is that the "panic by default" convention is only chosen if it's expected to be the 99.9% of the time use case. If you only call the "try borrow" 0.1% of the time it probably doesn't matter that much that it's the most optimized thing in the world.

I also disagree that it's a cliff in terms of ergnomics, e.g. compare:

if let Some(borrow) = cell.try_borrow_mut() {
    // ...
}

and

if let BorrowState::None = cell.borrow_state() {
   // .. work with cell.borrow_mut()
}

Again, though, I'm not claiming it to be more ergonomic, I just want to point out that it's not necessarily the end of the world. This is especially compounded by the fact that we're talking about the 0.1% use case, if it were expected to be called quite a bit then we'd of course choose the more ergonomic version. I think your suggestion about method accessors would also be fine as it'd even reduce the number of imports!

[nikomatsakis]

I think the real comparison is:

if let Some(borrow) = cell.try_borrow_mut() {
}

and

use std::cell::BorrowState;
...
if BorrowState::None == cell.borrow_state() {
    let borrow = cell.borrow_mut();
    ..
}

I certainly agree it's not the end of the world, but I still think it kind
of sucks. I think ultimately I just don't value ironclad rules as much as
you do. Certainly we should strive to return Option or Result whenever
we can. But if we choose to violate purity in favor of ergonomics, why not
go all the way?

I also disagree that this use case is 0.1%. I agree that detecting a cycle,
as I first subjected, is unusual. But I think that wanting to dump out the
contents of a RefCell in a Debug impl, or some other case where you
can't really tell what it's state is, is less unusual. Maybe as much as
0.2%!

However, ultimately, I would perhaps be mollified if we removed the borrow
state enum and replaced it with something opaque that just features
can_read, can_write methods. This way we can still extend RefCell to
have more states in the future. I think the current BorrowState is too
limiting.

[alexcrichton]

Although I nominated this for stabilization in 1.8, the libs team decided against it due to the double-check on the borrow flag. We'd instead like to explore methods like try_borrow

[cbreeden]

Isn't this code a race condition, in that it still may panic?

use std::cell::BorrowState;

if BorrowState::None == cell.borrow_state() {
    let borrow = cell.borrow_mut();
    /* use borrow */
}

I'm not saying that BorrowState isn't useful, but I'm not sure if it would be useful for this case.

[jonas-schievink]

@cbreeden RefCell can't be accessed by multiple threads at the same time though (it doesn't implement Sync)

[cbreeden]

oh right, thanks for clarifying that :)

[alexcrichton]

@rfcbot fcp close

We've added/stabilized try_borrow and try_borrow_mut, these should no longer be needed

[rfcbot]

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

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

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.

[nikomatsakis]

👍 from me to removing in favor of try_borrow and try_borrow_mut

[rfcbot]

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

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

[rfcbot]

The final comment period is now complete.

[nox]

It turns out I actually have a use case in Servo where I can't use try_borrow or try_borrow_mut but still need to know the refcell's borrow state. What would be the way to go to try to reintroduce it here? Make a PR adding it back?

[KodrAus]

We've re-introduced this API as the unstable try_borrow_unguarded method in #59211 so I'll re-open this issue.

[SimonSapin]

Servo has been using this since servo/servo#23196 to add a runtime check to some unsafe code, as discussed in PR #59211. Stabilizing would help do more of the same in libraries that also have users on Stable.

@rfcbot fcp merge

[SimonSapin]

It looks like rfcbot doesn’t want to do FCP again in the same thread, even after removing labels. I’ve proposed FCP in a stabilization PR: #60850

[jonas-schievink]

Add a concrete, motivating example to the documentation

It looks like this has not yet been addressed, but all the features discussed here are already stabilized.

[nox]

It looks like this has not yet been addressed, but all the features discussed here are already stabilized.

I'm not a technical writer and I don't see how to reduce the main use case into something smaller than how we are using this in Servo.

[jonas-schievink]

Sure, no problem. Just noting the inconsistency. I guess it's fine to close this tracking issue though, since everything tracked is now stable.