Skip to content

Add as_mut methods to the std::cell structs #32565

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

Merged
merged 1 commit into from
May 6, 2016

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Mar 28, 2016

This is safe since the borrow checking ensures that we have the only
mutable reference to the struct, thus we can safely borrow its interior.

@rust-highfive
Copy link
Contributor

r? @alexcrichton

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

@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 21, 2016

Would it make sense to make Cell implement DerefMut instead of having its own as_mut method? (The implementation of deref_mut would be identical to as_mut except for the name.)

One advantage is simplified syntax: cell.as_mut().mutate() becomes cell.mutate().

A possible disadvantage is that it is surprising for a type to implement DerefMut but not Deref.

Edit: Or, it could implement AsMut.

@stevenblenkinsop
Copy link

Implementing DerefMut is impossible, because DerefMut: Deref. AsMut seems possible, but there's no associated target type, which is unfortunate.

/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn into_inner(self) -> T {
pub fn into_inner(self) -> T {
Copy link
Member

Choose a reason for hiding this comment

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

This is a stable function so it cannot change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing an unsafe qualifier shouldn't be a breaking change, right? What code could it break?

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory at least it could break code like this:

let f: unsafe fn(Cell<u8>) -> u8 = Cell::<u8>::into_inner;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fair enough.

Copy link
Member

Choose a reason for hiding this comment

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

Theoretical break isn't enough for it to be considered a major change (see https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md ), but the RFC does not cover this case.

@alexcrichton
Copy link
Member

Thanks for the PR @tbu-! I don't think we can change into_inner because it's stable, and otherwise I'll tag this with T-libs to ensure it comes up during triage.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 21, 2016
@bluss
Copy link
Member

bluss commented Apr 22, 2016

Since new users are often confused by special case mutation methods like this (in my experience), the docs need to be clear somehow that this is a special case and regular modification of a Cell uses .set().

@alexcrichton
Copy link
Member

The libs team discussed this PR during triage today, and the conclusions were:

  • Conventionally these methods should be called get_mut
  • The implementations on Cell and RefCell seem fine.
  • We'd prefer to leave out the modifications to UnsafeCell for now. That change seems less motivated for now and easy to extract.

Could you make those changes @tbu- and I'll merge? Thanks again for the PR!

@tbu- tbu- force-pushed the pr_cell_as_mut branch from c4ba12f to 3930e6b Compare May 5, 2016 09:09
@tbu-
Copy link
Contributor Author

tbu- commented May 5, 2016

I made those changes. Note that I also modified the UnsafeCell documentation to state that the method isn't actually unsafe (there's no reason it should be, aside from backwards compatibility).

Also, I want to ask whether you really want to name these functions get. First of all, get is a prefix that suggests that work is done in the function, as implies that it's a no-op, which it really is (going from &mut Cell<T> to &mut T is nothing, and going from &mut RefCell<T> to &mut T doesn't cost anything if the RefCell is a local variable.

The style document you referenced seems horribly out of date, it tells us to use unwrap for today's into_inner and assert instead of Option's unwrap.

@alexcrichton
Copy link
Member

@tbu- no, let's leave UnsafeCell documentation as-is and can you open up a tracking issue for these methods?

This is safe since the borrow checker ensures that we have the only
mutable reference to the struct, thus we can safely borrow its interior.

Tracking issue is rust-lang#33444.
@tbu- tbu- force-pushed the pr_cell_as_mut branch from 3930e6b to 9370d3a Compare May 5, 2016 22:41
@tbu-
Copy link
Contributor Author

tbu- commented May 5, 2016

@alexcrichton Updated.

@alexcrichton
Copy link
Member

@bors: r+ 9370d3a

@bors
Copy link
Collaborator

bors commented May 5, 2016

⌛ Testing commit 9370d3a with merge c02ee81...

@bors
Copy link
Collaborator

bors commented May 6, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Collaborator

bors commented May 6, 2016

⌛ Testing commit 9370d3a with merge b071c1f...

bors added a commit that referenced this pull request May 6, 2016
Add `as_mut` methods to the `std::cell` structs

This is safe since the borrow checking ensures that we have the only
mutable reference to the struct, thus we can safely borrow its interior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants