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>, remove Cell<T> #9429

Closed
wants to merge 8 commits into from
Closed

RFC: Introduce Mut<T>, remove Cell<T> #9429

wants to merge 8 commits into from

Conversation

bluss
Copy link
Member

@bluss bluss commented Sep 23, 2013

Mut<T> implements an internally mutable slot, using #[no_freeze] and transmute_mut. It is much like Cell without the nullable part. It implements dynamic borrow checking, which is just an enum counting the current readers and writers.

Mut<T> provides methods .borrow() -> ReadPtr<'a, T> and .borrow_mut() -> WritePtr<'a, T> to obtain a wrapped borrowed pointer. These wrapped pointers have destructors that update the borrow flag in the Mut<T> when they go out of scope. Several ReadPtr can be alive at the same time.

Example:

let obj = Mut::new(5);
{
    let mut m_ptr = obj.borrow_mut();
    *m_ptr.get() = 7;
}
do obj.map |t| {  println!("value: {}", *t) }

For convenience, and to remove Cell, extra methods are implemented on Mut<Option<U>> that make it easy to treat it like an optional value. When replacing Cell's .take(), the only overhead of Mut<T> at the moment is the destructor function call of util::NonCopyable that's inserted.

Mut<T> is more generic than Cell<T>, and so can replace it better when it is used as an actual mutable slot. This represents very few previous users of Cell in the tree (see commit https://github.com/blake2-ppc/rust/commit/4f8c5b1954aef4e01c40935708757373d9b04b6f )

Mut<T> is also used to replace extra::rc::RcMut<T> with Rc<Mut<T>>. This requires a further restriction for cycle prevention (unless I'm mistaken) so that only T: Send values are allowed inside Rc<Mut<T>>. (This is because Rc<Mut<T>> is Freeze, while RcMut<T> was not.)

Clarifications and Known issues:

  • It doesn't merge. I'll wait for (RFC) option: rewrite the API to use composition #9359 before rebasing, and I'll update the commit messages to be more detailed.
  • The BorrowFlag enum is 16 bytes and is too large. This can be solved inelegantly by simply using an integer.
  • Mut<T> should be noncopyable because it needs to clear the borrow status when cloned.
  • For pcwalton: the name Mutable<T> was not possible since it conflicts with std::container::Mutable

Requests for comment:

  • Cell is replaced with a more fundamental type while 90% of users don't need this. Should Cell stay with just the constructor and the single method .take()? This represents the use case where closures wish to modify their moved-in upvalues.
  • Is it acceptable for a type to have both impl<T> Mut<T> and impl<U> Mut<Option<U>> ?
  • Should Mut<T> allow multiple ReadPtr (and thus it has to count them)?

blake2-ppc added 5 commits September 23, 2013 18:40
Replace the common Cell idiom with Mut<T>

before:

	let upvar_cell = Cell::new(x);
	do spawn {
		let xyz = upvar_cell.take().recv()
	}

after:

	let upvar_cell = Mut::new_some(x)
	do spawn {
		let xyz = upvar_cell.take_unwrap().recv()
	}
@alexcrichton
Copy link
Member

I'm personally a fan of this, it looks pretty awesome. Here's some thoughts:

  • I think Mut needs a destructor asserting that there are no outstanding loans on it. edit: nevermind, turns out rust is awesome for this exact reason.
  • I think ReadPtr/WritePtr should have a value fields instead of a get() function.
  • If Mut exists, I don't think Cell should at all.
  • Should this use similar conventions with as_imm/as_mut and drop the map functions?
  • I think two implementations is fine, especially because Cell is so popular right now. Perhaps you could even have type Cell<T> = Mut<Option<T>> in the mutable module.
  • I definitely think that multiple read pointers should be allowed (that's the point, right?).

I don't entirely remember why Mut was removed in the first place, so someone who remembers why should probably comment on whether we want to bring it back in this manifestation or not.

Nice work, though!

@bluss
Copy link
Member Author

bluss commented Sep 23, 2013

Thank you for the direct points!

I think ReadPtr/WritePtr should have a value fields instead of a get() function.

I'll explain why I think this is impossible. The lifetime relations are crucial.

// Mut<T>
pub fn borrow<'a>(&'a self) -> ReadPtr<'a, T>

The ReadPtr<'a, T> can maximally live as long as the Mut<T>.

// ReadPtr<T>
pub fn get<'a>(&'a self) -> &'a T

The derived borrowed pointer can maximally live as long as the ReadPtr, which is the duration that it is reserved in the parent Mut<T>. This is crucial. If we allowed to get a borrow pointer here with a life longer than the ReadPtr itself, the dynamic borrow check breaks down. I don't think this is possible to do with a value struct member. (This is an important thing to look at if we'd allow overloading dereferences or something similar.)

Should this use similar conventions with as_imm/as_mut and drop the map functions?

I'm not sure if this is possible! The map / map_mut methods are just convenience wrappers around the borrow methods, partly because you can't chain: *obj.borrow_mut().get() /* lifetime error */ probably just because of the lifetime logic elaborated above.

I think two implementations is fine, especially because Cell is so popular right now. Perhaps you could even have type Cell = Mut<Option> in the mutable module.

The Cell type is/was rarely named at all, anywhere; It's used in stack values, and so the choice of constructor function: Mut::new_some(u) -> Mut<Option<U>> and methods names like obj.take_unwrap() is the important part.

@bluss
Copy link
Member Author

bluss commented Sep 23, 2013

If Mut<T> should be simplified, it is possible to remove the possibility of multiple ReadPtr. It would turn into a token borrowed pointer wrapper, from which you could get the derived &T multiple times if wanted or &mut T exclusively. BorrowFlag would be just the cases Writing | Unused.

(But this simplification is harder to use/more restricted, especially when used together with Rc.)

@glaebhoerl
Copy link
Contributor

(This is because Rc<Mut<T>> is Freeze, while RcMut<T> was not.)

Isn't this just wrong?

From the manual:

Freeze
Types of this kind are deeply immutable; they contain no mutable memory locations directly or indirectly via pointers.

@nikomatsakis
Copy link
Contributor

The lack of chaining is #3511 (which I am in the process of fixing...)

In general, I like this PR, but this is definitely something that should be discussed in meeting before merging (as the RFC title suggests).

@bluss
Copy link
Member Author

bluss commented Sep 24, 2013

@glehel good question, I can't answer and haven't been able to find out. Rustc encodes logic for inheriting Freeze where even &Cell<T> is considered non-Freeze. There are no rules for *T or *mut T pointers.

@emberian
Copy link
Member

This needs a rebase.

@bluss
Copy link
Member Author

bluss commented Oct 2, 2013

Today's meeting didn't seem to dicuss this, but the closure changes are of big importance:
https://github.com/mozilla/rust/wiki/Meeting-weekly-2013-10-01

If ~fn() is replaced by something corresponding to ~once fn(), most of the Cell uses in the Rust tree disappear; I'd like to leave Cell as it is until it's no longer needed, and introduce Mut in parallel.

@alexcrichton
Copy link
Member

I'm personally in favor of this, but like composition I feel like this warrants more discussion. I would love to remove as many users of Cell as possible, I need to get up to speed on the function changes...

Regardless, I think that RcMut<T> == Rc<Mut<T>> is really cool :)

@thestinger
Copy link
Contributor

It's not actually possible to replace RcMut<T> with Rc<Mut<T>> due to the Freeze issue identified by @blake2-ppc. The type system would need to provide a way to have (lack of) Freeze pass through raw pointers.

@sfackler
Copy link
Member

sfackler commented Nov 7, 2013

It looks like 71acc54 fixes the Freeze issue.

@alexcrichton
Copy link
Member

Closing due to lack of activity, but feel free to reopen if this gets rebased!

sfackler added a commit to sfackler/rust that referenced this pull request Nov 23, 2013
Based off of blake2-ppc's work in rust-lang#9429.
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`?
djkoloski pushed a commit to djkoloski/rust that referenced this pull request Sep 21, 2022
Make `derivable_impls` machine applicable

changelog: [`derivable_impls`]: Now machine applicable
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.

7 participants