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

RFC: Introduce Mut<T> to libstd (round 2) #10514

Merged
merged 10 commits into from
Nov 24, 2013
Merged

RFC: Introduce Mut<T> to libstd (round 2) #10514

merged 10 commits into from
Nov 24, 2013

Conversation

sfackler
Copy link
Member

This is based off of @blake2-ppc's work on #9429. That PR bitrotted and I haven't been able to contact the original author so I decided to take up the cause.

Overview

Mut encapsulates a mutable, non-nullable slot. The Cell type is currently used to do this, but Cell is much more commonly used as a workaround for the inability to move values into non-once functions. Mut provides a more robust API.

Mut duplicates the semantics of borrowed pointers with enforcement at runtime instead of compile time.

let x = Mut::new(0);

{
    // make some immutable borrows
    let p = x.borrow();
    let y = *p.get() + 10;

    // multiple immutable borrows are allowed simultaneously
    let p2 = x.borrow();

    // this would throw a runtime failure
    // let p_mut = x.borrow_mut();
}

// now we can mutably borrow
let p = x.borrow_mut();
*p.get() = 10;

borrow returns a Ref type and borrow_mut returns a RefMut type, both of which are simple smart pointer types with a single method, get, which returns a reference to the wrapped data.

This also allows RcMut<T> to be deleted, as it can be replaced with Rc<Mut<T>>.

Changes

I've done things a little bit differently than the original proposal.

  • I've added try_borrow and try_borrow_mut methods that return Option<Ref<T>> and Option<RefMut<T>> respectively instead of failing on a borrow check failure. I'm not totally sure when that'd be useful, but I don't see any reason to not put them in and @cmr requested them.
  • ReadPtr and WritePtr have been renamed to Ref and RefMut respectively, as Ref is to ref foo and RefMut is to ref mut foo as Mut is to mut foo.
  • get on MutRef now takes &self instead of &mut self for consistency with &mut. As @alexcrichton pointed, out this violates soundness by allowing aliasing &mut references.
  • Cell is being left as is. It solves a different problem than Mut is designed to solve.
  • There are no longer methods implemented for Mut<Option<T>>. Since Cell isn't going away, there's less of a need for these, and I didn't feel like they provided a huge benefit, especially as that kind of impl is very uncommon in the standard library.

Open Questions

  • Cell should now be used exclusively for movement into closures. Should this be enforced by reducing its API to new and take? It seems like this use case will be completely going away once the transition to proc and co. finishes.
  • Should there be try_map and try_map_mut methods along with map and map_mut?

///
/// Fails if the value is currently mutably borrowed.
#[inline]
pub fn map<U>(&self, blk: |&T| -> U) -> U {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be called with and with_mut? It doesn't seem like there's any mapping going on here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@alexcrichton
Copy link
Member

I'm very much a fan of destroying Cell as much as possible. If cell were reduced to new, take, and put_back, then why again can't we define the same methods for Mut<Option<T>>? Otherwise, I'm in favor of deleting the extra Cell methods that should be superseded by Mut<T> now.

I would not vote for a try_map{,_mut}, map_mut is only a convenience method and if you want the try version then you could say try_borrow().map(|a| f(a.get())( which isn't too bad.

@emberian
Copy link
Member

Agree

On Sat, Nov 16, 2013 at 3:25 AM, Alex Crichton [email protected]:

I'm very much a fan of destroying Cell as much as possible. If cell were
reduced to new, take, and put_back, then why again can't we define the
same methods for Mut<Option>? Otherwise, I'm in favor of deleting the
extra Cell methods that should be superseded by Mut now.

I would not vote for a try_map{,_mut}, map_mut is only a convenience
method and if you want the try version then you could say try_borrow().map(|a|
f(a.get())( which isn't too bad.


Reply to this email directly or view it on GitHubhttps://github.com//pull/10514#issuecomment-28622183
.

@glaebhoerl
Copy link
Contributor

Is there any reason not to add type RcMut<T> = Rc<Mut<T>>?

@sfackler
Copy link
Member Author

@alexcrichton I'm hesitant to totally replace Cell<T> with Mut<Option<T>> since Cell is used to emulate move semantics at runtime and Mut is used to emulate mutability at runtime, which I see as being significantly different. Plus leaving Cell around for now will allow for easier cleanup once closure reform goes through.

@alexcrichton
Copy link
Member

Sounds reasonable to me, how do you feel about reducing Cell's api as part of this pull request?

@sfackler
Copy link
Member Author

Will do

@emberian
Copy link
Member

You can't call associated functions on typedefs, so it'd be weirdly
asymetric (let x: RcMut<T>= Rc::new(Mut::new(42)))

On Sat, Nov 16, 2013 at 6:47 AM, Gábor Lehel [email protected]:

Is there any reason not to add type RcMut = Rc<Mut>?


Reply to this email directly or view it on GitHubhttps://github.com//pull/10514#issuecomment-28625010
.

@nikomatsakis
Copy link
Contributor

I am in favor of this PR, but I'd like to do a thorough review. I think we'll need a few more compile-fail tests, if nothing else.

@sfackler
Copy link
Member Author

@alexcrichton I've stripped Cell's API down to new, take, take_opt and is_empty. It's a surprisingly small change. It's still called Cell for now, though I've updated the module documentation.

@nikomatsakis I've added a couple more compile-fail soundness tests.

@thestinger
Copy link
Contributor

RcMut currently requires Send or Freeze. If it's replaced with Rc<Mut<T>>, it will always require Send because the contained value will always be non-Freeze.

@sfackler
Copy link
Member Author

That is a problem. We could do something like

impl<T: Freeze> Rc<Mut<T>> {
    fn from_mut(val: Mut<T>) -> Rc<Mut<T>> { ... }
}

@thestinger
Copy link
Contributor

@sfackler: I don't think we need a from_send one since Rc has it, but that new function is going to conflict with the one on Rc<T> (the compiler should throw an error, not sure if it does).

@sfackler
Copy link
Member Author

@thestinger yeah, I just realized that and edited the comment.

@thestinger
Copy link
Contributor

@sfackler: Is that usable as Rc::from_mut? It seems like it would be, but I could be wrong!

@sfackler
Copy link
Member Author

@thestinger yep, it is. I've updated the PR.

@pcwalton
Copy link
Contributor

My current thinking is that:

  • Cell should go away;
  • This API should be renamed to BorrowableCell;
  • There should be a new API Cell which is like this API but (a) doesn't have the borrow flags and (b) doesn't allow borrowing (only get which copies out and set which sets the value). The reasoning here is that if you just have a scalar field like int, or even a small-ish field like Rect<int>, there's no need to support borrowing; you might as well just avoid wasting space on borrow flags and copy out the values every time. Furthermore, copying out values and setting a new values is an infallible operation that will never fail unless borrowing is allowed, so using this hypothetical future Cell instead of BorrowableCell when possible will lead to fewer dynamic borrow check failures. Finally, this Cell would be simpler for beginners to use.

@nikomatsakis
Copy link
Contributor

After some discussion on IRC, we decided that while @pcwalton's scheme of using a "tag-less" Cell is appealing, it doesn't quite work as specified, since there is the possibility that when you call get(), which in turn invokes clone(). The call to clone() invokes user-supplied code which could then call set(), overwriting the value being cloned.

There are some ways to overcome this: one option that appeals to me is to define a "POD" builtin bound, basically meaning any type that can be safely memcopied, and then say that Cell does not invoke clone but rather just memcopies. Of course this is essentially just reintroducing implicitly copyable as a thing.

Presuming we do wind up with two types, then RefCell is a better name than BorrowableCell.

@glaebhoerl
Copy link
Contributor

I think "Mut" is a much more helpful name than "Cell". You use it to introduce the ability to mutate. What association is "Cell" supposed to evoke?

@thestinger
Copy link
Contributor

The prison cell you're going to be sent to for bending the mutability rules :).

@bluss
Copy link
Member

bluss commented Nov 19, 2013

You have my blessings. Not that it's needed, but still.

@pcwalton
Copy link
Contributor

The consensus we came to is that this should be named RefCell. Once that's done, I'm happy to merge. Thanks a lot for the work on this!

@glehel The problem here is that there are multiple things called Mut (this, and the qualifier on locals) and it'll be hard to talk about the different kinds. I found myself having to say "capital-M Mut" and "lowercase-M mut" in person and that was pretty awkward. I took the name "cell" from ML.

@sfackler
Copy link
Member Author

Rc<RefCell<T>> seems like an exceptionally confusing type name, but I'll rename it and move it to std::cell.

@sfackler
Copy link
Member Author

@pcwalton updated.

@glaebhoerl
Copy link
Contributor

(I think reading or writing the name is a much more common use case than saying or hearing it, but w/e.)

Based off of blake2-ppc's work in rust-lang#9429.
Rc<Mut<T>> should be used instead
bors added a commit that referenced this pull request Nov 24, 2013
This is based off of @blake2-ppc's work on #9429. That PR bitrotted and I haven't been able to contact the original author so I decided to take up the cause.

Overview
======
`Mut` encapsulates a mutable, non-nullable slot. The `Cell` type is currently used to do this, but `Cell` is much more commonly used as a workaround for the inability to move values into non-once functions. `Mut` provides a more robust API.

`Mut` duplicates the semantics of borrowed pointers with enforcement at runtime instead of compile time.
```rust
let x = Mut::new(0);

{
    // make some immutable borrows
    let p = x.borrow();
    let y = *p.get() + 10;

    // multiple immutable borrows are allowed simultaneously
    let p2 = x.borrow();

    // this would throw a runtime failure
    // let p_mut = x.borrow_mut();
}

// now we can mutably borrow
let p = x.borrow_mut();
*p.get() = 10;
```
`borrow` returns a `Ref` type and `borrow_mut` returns a `RefMut` type, both of which are simple smart pointer types with a single method, `get`, which returns a reference to the wrapped data.

This also allows `RcMut<T>` to be deleted, as it can be replaced with `Rc<Mut<T>>`.

Changes
======
I've done things a little bit differently than the original proposal.

* I've added `try_borrow` and `try_borrow_mut` methods that return `Option<Ref<T>>` and `Option<RefMut<T>>` respectively instead of failing on a borrow check failure. I'm not totally sure when that'd be useful, but I don't see any reason to not put them in and @cmr requested them.
* `ReadPtr` and `WritePtr` have been renamed to `Ref` and `RefMut` respectively, as `Ref` is to `ref foo` and `RefMut` is to `ref mut foo` as `Mut` is to `mut foo`.
* `get` on `MutRef` now takes `&self` instead of `&mut self` for consistency with `&mut`. As @alexcrichton pointed, out this violates soundness by allowing aliasing `&mut` references.
* `Cell` is being left as is. It solves a different problem than `Mut` is designed to solve.
* There are no longer methods implemented for `Mut<Option<T>>`. Since `Cell` isn't going away, there's less of a need for these, and I didn't feel like they provided a huge benefit, especially as that kind of `impl` is very uncommon in the standard library.

Open Questions
============
* `Cell` should now be used exclusively for movement into closures. Should this be enforced by reducing its API to `new` and `take`? It seems like this use case will be completely going away once the transition to `proc` and co. finishes.
* Should there be `try_map` and `try_map_mut` methods along with `map` and `map_mut`?
@bors bors closed this Nov 24, 2013
@bors bors merged commit bdfaf04 into rust-lang:master Nov 24, 2013
@sfackler sfackler deleted the mut branch December 23, 2013 03:55
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 16, 2021
Introduce {Ref, RefMut}::try_map for optional projections in RefCell

This fills a usability gap of `RefCell` I've personally encountered to perform optional projections, mostly into collections such as `RefCell<Vec<T>>` or `RefCell<HashMap<U, T>>`:

> This kind of API was briefly featured under Open questions in rust-lang#10514 back in 2013 (!)

```rust
let values = RefCell::new(vec![1, 2, 3, 4]);
let b = Ref::opt_map(values.borrow(), |vec| vec.get(2));
```

It primarily avoids this alternative approach to accomplish the same kind of projection which is both rather noisy and panicky:
```rust
let values = RefCell::new(vec![1, 2, 3, 4]);

let b = if values.get(2).is_some() {
    Some(Ref::map(values.borrow(), |vec| vec.get(2).unwrap()))
} else {
    None
};
```

### Open questions

The naming `opt_map` is preliminary. I'm not aware of prior art in std to lean on here, but this name should probably be improved if this functionality is desirable.

Since `opt_map` consumes the guard, and alternative syntax might be more appropriate which instead *tries* to perform the projection, allowing the original borrow to be recovered in case it fails:

```rust
pub fn try_map<U: ?Sized, F>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, Self>
where
    F: FnOnce(&T) -> Option<&U>;
```

This would be more in line with the `try_map` method [provided by parking lot](https://docs.rs/lock_api/0/lock_api/struct.RwLockWriteGuard.html#method.try_map).
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
Introduce {Ref, RefMut}::try_map for optional projections in RefCell

This fills a usability gap of `RefCell` I've personally encountered to perform optional projections, mostly into collections such as `RefCell<Vec<T>>` or `RefCell<HashMap<U, T>>`:

> This kind of API was briefly featured under Open questions in rust-lang#10514 back in 2013 (!)

```rust
let values = RefCell::new(vec![1, 2, 3, 4]);
let b = Ref::opt_map(values.borrow(), |vec| vec.get(2));
```

It primarily avoids this alternative approach to accomplish the same kind of projection which is both rather noisy and panicky:
```rust
let values = RefCell::new(vec![1, 2, 3, 4]);

let b = if values.get(2).is_some() {
    Some(Ref::map(values.borrow(), |vec| vec.get(2).unwrap()))
} else {
    None
};
```

### Open questions

The naming `opt_map` is preliminary. I'm not aware of prior art in std to lean on here, but this name should probably be improved if this functionality is desirable.

Since `opt_map` consumes the guard, and alternative syntax might be more appropriate which instead *tries* to perform the projection, allowing the original borrow to be recovered in case it fails:

```rust
pub fn try_map<U: ?Sized, F>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, Self>
where
    F: FnOnce(&T) -> Option<&U>;
```

This would be more in line with the `try_map` method [provided by parking lot](https://docs.rs/lock_api/0/lock_api/struct.RwLockWriteGuard.html#method.try_map).
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
Introduce {Ref, RefMut}::try_map for optional projections in RefCell

This fills a usability gap of `RefCell` I've personally encountered to perform optional projections, mostly into collections such as `RefCell<Vec<T>>` or `RefCell<HashMap<U, T>>`:

> This kind of API was briefly featured under Open questions in rust-lang#10514 back in 2013 (!)

```rust
let values = RefCell::new(vec![1, 2, 3, 4]);
let b = Ref::opt_map(values.borrow(), |vec| vec.get(2));
```

It primarily avoids this alternative approach to accomplish the same kind of projection which is both rather noisy and panicky:
```rust
let values = RefCell::new(vec![1, 2, 3, 4]);

let b = if values.get(2).is_some() {
    Some(Ref::map(values.borrow(), |vec| vec.get(2).unwrap()))
} else {
    None
};
```

### Open questions

The naming `opt_map` is preliminary. I'm not aware of prior art in std to lean on here, but this name should probably be improved if this functionality is desirable.

Since `opt_map` consumes the guard, and alternative syntax might be more appropriate which instead *tries* to perform the projection, allowing the original borrow to be recovered in case it fails:

```rust
pub fn try_map<U: ?Sized, F>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, Self>
where
    F: FnOnce(&T) -> Option<&U>;
```

This would be more in line with the `try_map` method [provided by parking lot](https://docs.rs/lock_api/0/lock_api/struct.RwLockWriteGuard.html#method.try_map).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants