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

Introduce non-panicking borrow methods on `RefCell<T>` #1660

Merged
merged 1 commit into from Jul 27, 2016

Conversation

Projects
None yet
@nox
Copy link
Contributor

commented Jun 27, 2016

No description provided.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

I’ve commented before in favor of this to replace borrow_state.

@notriddle

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

@alexcrichton alexcrichton added the T-libs label Jun 27, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 27, 2016

cc rust-lang/rust#27733 where a bit of discussion about this has already taken place, especially with respect to breaking the convention of not having panicking and non-panicking methods on a type (in my mind the major drawback here).

@Ericson2314

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

Seems like with respect to that convention, we should have only had the optional ones from day one? [Indexing, brought up in the linked thread, additionally had to be panic because otherwise a[b] could not be an lvalue. There is no such restriction here]

So +1 this, and maybe even deprecate the old API.

@nox

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2016

We (Servo) has only one use case for such a feature in ipc-channel, I wouldn't like to see its panicking counterpart be deprecated.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

Personally I think "don't have both panicking and non-panicking methods" is a fine convention. But it's still just a convention. A guideline, a rule of thumb. I don't think it makes sense to tie ourselves in knots around it if it's clearly not ideal for a particular case.

Whenever something is built from user input, for example a graph in which nodes
are `RefCell<T>` values, it is primordial to avoid panicking on bad input. The
only way to avoid panics on cyclic input in this case is a way to
conditionally-borrow the cell contents.D

This comment has been minimized.

Copy link
@ticki

ticki Jun 28, 2016

Contributor

Typo?

This comment has been minimized.

Copy link
@nox

nox Jun 28, 2016

Author Contributor

Fixed.

@nox nox force-pushed the nox:try-borrow branch from 80fac83 to 57063ad Jun 28, 2016

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

@alexcrichton There is precedent in slices that have both s[i] and s.get(i). @Ericson2314 mentions lvalues as a reason for this, but I think it’s just as important that in the majority of cases the index is known (modulo bugs) to be in bounds, and requiring users to write .unwrap() that often would be tedious. But get is still useful sometimes.

That said, the borrow_state method is a viable alternative. (I just think it’s a bit more awkward.)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

It's worth mentioning here as well, but the major drawback of borrow_state is that it does runtime checks twice rather than once. First you check the borrow flag to calculate the BorrowState, then you check it again if you determine you should call borrow or borrow_mut. While the optimizer may be able to take care of this, it's not always generally applicable to other abstractions, so as a general rule of thumb it's not always a suitable replacement.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

Also, if one can work around an already-borrowed RefCell, the borrow_state method still leaves one with a singe unreachable panic, which is annoying false red flag for a totality lint, etc.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2016

I still prefer try_foo to checking the borrow state. There is both the runtime cost that @alexcrichton mentioned but also the current incarnation of BorrowState is bad in several ways:

  • it constraints future API development. Since it's an exhaustive enum, we could never add new states (e.g., I'd like to see a take() method on RefCell that takes ownership and leaves nothing remaining, but that would be a new borrow state).
  • it's not ergonomic to use it. You want to write something like cell.borrow_state().can_read(), I think, not match cell { ... }. The match is bad both b/c it's annoying and because it requires you to understand which states allow which operations.
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

🔔 This RFC is now entering its week-long final comment period 🔔

The libs team discussed this and we feel pretty comfortable adding these methods at this point, so we're thinking of merging.

@aturon

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

I'm 👍 on this change, and agree with @glaebhoerl that conventions need to be flexible.

We should think carefully about the name here, because we're also establishing a kind of convention for non-panicking alternatives. In particular, try_ is a common prefix for making non-blocking attempts for otherwise-blocking operations (like try_send), and it's plausible that the two uses of the name could conflict. For example, we could've designed Mutex such that the default methods panicked, and wanted to provide a try_lock method that returns a Result -- but that name would be very confusing.

That said, no better name is immediately leaping to mind, and I'm happy to accept the RFC and see whether we can think of anything better prior to stabilization. If we end up with overlapping conventions, it's not the end of the world.

@sfackler

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

I like this as-is. The try_ prefix does have some prior art in try_from and try_into.

@aturon

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Yeah, I forgot to mention we're effectively doing the same thing with try_from, though it hasn't been stabilized just yet. (Not quite "prior" art ;-)

Anyway, I sort of doubt we'll be able to find a better prefix.

@notriddle

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

checked_borrow() is to borrow() as checked_add() is to add()?

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@notriddle

But add does not panic in release builds.

@notriddle

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2016

@arielb1 The only reason Add and RefCell are different is because turning off RefCell checking in release mode would allow memory safety violation. The current Add behavior is also not guaranteed in perpetuity; it can switch to panic in release or overflow in debug in future versions.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

The libs team got a chance to discuss this RFC yesterday, and the decision was to merge. We've long felt that this function would likely come into existence so it seems reasonable to merge as unstable at least. We're still a little iffy on the naming (e.g. try vs checked), but we're relatively confident we'll end up at try.

That being said, these APIs will remain unstable in the standard library before actually stabilizing, and the libs team is fine having a point of stabilization be "finalize the conventions around these sorts of fallible functions", so that's something that we can postpone and doesn't need to block this RFC.

@nox one point brought up though is that we probably want these functions to return a Result instead of an option, so that way we can extend the API in the future with flavorful information about why the borrow failed, if any. Could you update this to indicate that? Afterwards I'd be happy to merge!

@nox nox force-pushed the nox:try-borrow branch from 57063ad to 068c39d Jul 26, 2016

@nox

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2016

@alexcrichton alexcrichton merged commit 4809190 into rust-lang:master Jul 27, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

Thanks again for the RFC @nox!

Tracking issue

@nox nox deleted the nox:try-borrow branch Aug 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.