Implement rfc 1789: Conversions from `&mut T` to `&Cell<T>` #50494

F001 · 2018-05-07T07:36:14Z

I'm surprised that RFC 1789 has not been implemented for several months. Tracking issue: #43038

Please note: when I was writing tests for &Cell<[i32]>, I found it is not easy to get the length of the contained slice. So I designed a get_with method which might be useful for similar cases. This method is not designed in the RFC, and it certainly needs to be reviewed by core team. I think it has some connections with Cell::update #50186 , which is also in design phase.

rust-highfive · 2018-05-07T07:36:23Z

r? @joshtriplett

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

cramertj · 2018-05-07T16:53:35Z

src/libcore/cell.rs

@@ -235,7 +236,7 @@ use ptr;
 ///
 /// See the [module-level documentation](index.html) for more.
 #[stable(feature = "rust1", since = "1.0.0")]
-pub struct Cell<T> {
+pub struct Cell<T: ?Sized> {


Should this be #[repr(transparent)], or at least #[repr(C)]?

Should definitely be repr(transparent).

Thanks. Fixed.

pietroalbini · 2018-05-14T10:17:38Z

Ping from triage @joshtriplett! This PR needs your review.

joshtriplett · 2018-05-14T16:48:15Z

This looks plausible to me, but I'd feel more comfortable if a second reviewer looked at it.

cramertj · 2018-05-14T18:31:08Z

src/libcore/cell.rs

@@ -235,7 +236,8 @@ use ptr;
 ///
 /// See the [module-level documentation](index.html) for more.
 #[stable(feature = "rust1", since = "1.0.0")]
-pub struct Cell<T> {
+#[repr(transparent)]
+pub struct Cell<T: ?Sized> {
    value: UnsafeCell<T>,


@eddyb is UnsafeCell itself already effectively repr(transparent)?

It should be explicitly marked as such.

Note that #[repr(transparent)] means the same thing as #[repr(C)] except it also guarantees you can pass it by value and get the same call ABI as its only non-ZST field. If we don't want to make the call ABI transparency guarantee, we should use only #[repr(C)]. Although, transparency would be nice. So maybe we should have a proper decision on it?

Also, in terms of implementation details, within the libcore/libstd you can "just" assume that anything newtype-shaped already has the memory layout and call ABI of the inner type. So this PR by itself needs no annotations, and adding annotations should be considered as sort of insta-stable "guarantees".

It should be explicitly marked as such.

Should that be a separate PR or happen here?

Just trying to make sure it doesn't get forgotten.

cc @rust-lang/lang @rust-lang/libs Whose responsibility is it, for the addition of insta-stable #[repr] attributes like these ones?

For what it’s worth we’re about to stabilize the attribute itself: #43036

The attribute is stable. #51395 adds #[repr(transparent)] to UnsafeCell and Cell. (It’s waiting on checkboxes from team members.)

bors · 2018-05-17T19:35:49Z

☔ The latest upstream changes (presumably #50629) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2018-05-18T12:36:57Z

@F001 You need to rebase to get rid of the merge commit.

F001 · 2018-05-23T01:03:21Z

cc @rust-lang/libs . Is there anyone else can help to do the review? Thanks!

eddyb · 2018-05-23T10:48:34Z

cc @rust-lang/libs

cramertj · 2018-05-23T17:22:07Z

src/libcore/cell.rs

@@ -521,6 +567,20 @@ impl<T: Default> Cell<T> {
 #[unstable(feature = "coerce_unsized", issue = "27732")]
 impl<T: CoerceUnsized<U>, U> CoerceUnsized<Cell<U>> for Cell<T> {}

+#[unstable(feature = "as_cell", issue="43038")]
+impl<T, I> Index<I> for Cell<[T]>


Trait implementations are insta-stable, so this needs a review (and I think an FCP?) from @rust-lang/libs

Or perhaps it's effectively unstable because it's not possible to construct a Cell<[T]> safely without this change?

Sure it is, make a &Cell<[T; N]> and unsize it.

alexcrichton

@rfcbot fcp merge

Due to the new trait impl let's FCP for merge

alexcrichton · 2018-05-25T01:09:49Z

src/libcore/cell.rs

+    /// ```
+    #[inline]
+    #[unstable(feature = "as_cell", issue="43038")]
+    pub fn get_with<U, F>(&self, f: F) -> U


Was this in the RFC? As-is I think this isn't sound unfortunately :(

As I mentioned in the description of this pull request (at the top of the thread), I am not quite confident on this method. Could you please help to point out how to fix it, or remove it completely?

Unfortunately as-is this can't be safely written, but I think you can coerce &Cell<[i32]> to &[Cell<i32>] to get the length, right?

eddyb · 2018-05-26T10:00:16Z

src/libcore/cell.rs

+    #[unstable(feature = "as_cell", issue="43038")]
+    pub fn get_with<U, F>(&self, f: F) -> U
+    where
+        F: Fn(&T) -> U, U: 'static


FnOnce should be used in a situation like this.

U: 'static doesn't do anything useful here, U can't hold the reference to T anyway in Rust, because F gets no guarantee as to how long that anonymous lifetime is.

I'll make a separate comment on the PR about the unsoundness so it doesn't get lost

eddyb · 2018-05-26T10:13:55Z

To expand on what @alexcrichton said, Cell::get_with<F: FnOnce(&T) -> R, R>(&self, F) -> R is unsound because there's no way to enforce limits on what F can do (that is, that the closure doesn't do anything else to mutate self, which would conflict with the &T borrow).
One quick example demonstrating how unsoundness can go wrong is (feel free to test it):

let cell = Cell::new(Some(String::from("foo")));
cell.get_with(|s| {
    let s = s.as_ref().unwrap();
    cell.set(None);
    let s2 = String::from("bar");
    println!("{}", s); // crash or print "bar" or garbage
    println!("{}", s2); // keep `s2` alive
});

If there was, you could even make it give out &mut T, and I call this Cell::with_mut.
Once you have Cell::with_mut, you can implement all other Cell APIs safely through it, and the guarantee for F becomes "reentrace freedom" (i.e. F can never call with_mut again, transitively).
(in some sense this is a thread-local analogue of "atomic operation")

Sadly, we're not ready to support anything like this yet (cc @nikomatsakis @RalfJung).

eddyb · 2018-05-27T08:40:14Z

src/libcore/cell.rs

+    /// ```
+    #[inline]
+    #[unstable(feature = "as_cell", issue="43038")]
+    pub fn from_mut<'a>(t: &'a mut T) -> &'a Cell<T> {


get_mut relies on lifetime elision while this doesn't - they should be more uniform IMO.

eddyb · 2018-05-27T15:29:26Z

src/libcore/cell.rs

+impl<T, I> Index<I> for Cell<[T]>
+where
+    I: SliceIndex<[Cell<T>]>
+{


Oh, something potentially of note: it should be possible to implement Deref<Target=[Cell<T>]> on Cell<[T]> (instead of Index), to allow e.g. calling .len() directly.

Thanks! IMHO it is a great improvement. I think it is sound AFAIK.

F001 · 2018-05-28T02:41:26Z

I have no idea why linkchecker failed. It seems irrelevant to my last commit.

eddyb · 2018-05-28T06:03:41Z

src/libcore/cell.rs

+    /// let cell_slice: &Cell<[i32]> = Cell::from_mut(slice);
+    /// assert_eq!(cell_slice.len(), 3);
+    /// let slice_cell : &[Cell<i32>] = &cell_slice[..];
+    /// assert_eq!(slice_cell.len(), 3);


Please leave some empty lines in this example just like the get_mut example does.

eddyb · 2018-05-28T06:04:08Z

src/libcore/cell.rs

+    /// let slice: &mut [i32] = &mut [1,2,3];
+    /// let cell_slice: &Cell<[i32]> = Cell::from_mut(slice);
+    /// assert_eq!(cell_slice.len(), 3);
+    /// let slice_cell : &[Cell<i32>] = &cell_slice[..];


Nit: extraneous space before :.

eddyb · 2018-05-28T06:04:24Z

src/libcore/cell.rs

+    /// ```
+    /// #![feature(as_cell)]
+    /// use std::cell::Cell;
+    /// let slice: &mut [i32] = &mut [1,2,3];


Nit: missing spaces after ,.

eddyb · 2018-07-13T08:10:55Z

If it's an unstable-only method, I'd suggest as_slice_of_cells.

F001 · 2018-07-23T01:04:38Z

@alexcrichton I have replaced the "Deref" by "Index". Could you please help to do the review?

SimonSapin · 2018-07-23T09:05:17Z

@F001 I believe the objection was not about a trait v.s. another trait but having a trait impl that becomes stable as soon as this PR lands, rather an inherent method that is #[unstable] for now.

alexcrichton · 2018-07-23T14:10:21Z

@bors: r+

Thanks @F001!

bors · 2018-07-23T14:10:22Z

📌 Commit 489101c has been approved by alexcrichton

bors · 2018-07-23T19:43:44Z

⌛ Testing commit 489101c with merge 00204c2...

Implement rfc 1789: Conversions from `&mut T` to `&Cell<T>` I'm surprised that RFC 1789 has not been implemented for several months. Tracking issue: #43038 Please note: when I was writing tests for `&Cell<[i32]>`, I found it is not easy to get the length of the contained slice. So I designed a `get_with` method which might be useful for similar cases. This method is not designed in the RFC, and it certainly needs to be reviewed by core team. I think it has some connections with `Cell::update` #50186 , which is also in design phase.

bors · 2018-07-23T21:44:32Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 00204c2 to master...

Version 1.29.0 (2018-09-13) ========================== Compiler -------- - [Bumped minimum LLVM version to 5.0.][51899] - [Added `powerpc64le-unknown-linux-musl` target.][51619] - [Added `aarch64-unknown-hermit` and `x86_64-unknown-hermit` targets.][52861] Libraries --------- - [`Once::call_once` now no longer requires `Once` to be `'static`.][52239] - [`BuildHasherDefault` now implements `PartialEq` and `Eq`.][52402] - [`Box<CStr>`, `Box<OsStr>`, and `Box<Path>` now implement `Clone`.][51912] - [Implemented `PartialEq<&str>` for `OsString` and `PartialEq<OsString>` for `&str`.][51178] - [`Cell<T>` now allows `T` to be unsized.][50494] - [`SocketAddr` is now stable on Redox.][52656] Stabilized APIs --------------- - [`Arc::downcast`] - [`Iterator::flatten`] - [`Rc::downcast`] Cargo ----- - [Cargo can silently fix some bad lockfiles ][cargo/5831] You can use `--locked` to disable this behaviour. - [`cargo-install` will now allow you to cross compile an install using `--target`][cargo/5614] - [Added the `cargo-fix` subcommand to automatically move project code from 2015 edition to 2018.][cargo/5723] Misc ---- - [`rustdoc` now has the `--cap-lints` option which demotes all lints above the specified level to that level.][52354] For example `--cap-lints warn` will demote `deny` and `forbid` lints to `warn`. - [`rustc` and `rustdoc` will now have the exit code of `1` if compilation fails, and `101` if there is a panic.][52197] - [A preview of clippy has been made available through rustup.][51122] You can install the preview with `rustup component add clippy-preview` Compatibility Notes ------------------- - [`str::{slice_unchecked, slice_unchecked_mut}` are now deprecated.][51807] Use `str::get_unchecked(begin..end)` instead. - [`std::env::home_dir` is now deprecated for its unintuitive behaviour.][51656] Consider using the `home_dir` function from https://crates.io/crates/dirs instead. - [`rustc` will no longer silently ignore invalid data in target spec.][52330] [52861]: rust-lang/rust#52861 [52656]: rust-lang/rust#52656 [52239]: rust-lang/rust#52239 [52330]: rust-lang/rust#52330 [52354]: rust-lang/rust#52354 [52402]: rust-lang/rust#52402 [52103]: rust-lang/rust#52103 [52197]: rust-lang/rust#52197 [51807]: rust-lang/rust#51807 [51899]: rust-lang/rust#51899 [51912]: rust-lang/rust#51912 [51511]: rust-lang/rust#51511 [51619]: rust-lang/rust#51619 [51656]: rust-lang/rust#51656 [51178]: rust-lang/rust#51178 [51122]: rust-lang/rust#51122 [50494]: rust-lang/rust#50494 [cargo/5614]: rust-lang/cargo#5614 [cargo/5723]: rust-lang/cargo#5723 [cargo/5831]: rust-lang/cargo#5831 [`Arc::downcast`]: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.downcast [`Iterator::flatten`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.flatten [`Rc::downcast`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.downcast

rust-highfive assigned joshtriplett May 7, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2018

This comment has been minimized.

Sign in to view

F001 force-pushed the as_cell branch from 2e444f9 to 950bbae Compare May 7, 2018 09:06

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 7, 2018

cramertj reviewed May 7, 2018

View reviewed changes

cramertj reviewed May 14, 2018

View reviewed changes

F001 force-pushed the as_cell branch from e3a8aa7 to 9cef363 Compare May 18, 2018 16:23

cramertj reviewed May 23, 2018

View reviewed changes

alexcrichton reviewed May 25, 2018

View reviewed changes

eddyb reviewed May 26, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

F001 force-pushed the as_cell branch from a20dd05 to 52082e3 Compare May 26, 2018 21:20

eddyb reviewed May 27, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

eddyb reviewed May 28, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

F001 force-pushed the as_cell branch from 22f1391 to 02c87aa Compare May 30, 2018 12:48

F001 added 8 commits July 17, 2018 11:34

implement rfc 1789

9928baa

add repr transparent

20b50f5

remove "get_with" method

4bf8b57

use lifetime elision for consistency

4d99957

impl Deref instead of Index

bdf8630

code style fixes

b1344ab

impl DerefMut for Cell<[T]>

3eee486

remove useless feature(repr_transparent)

7e1b983

F001 force-pushed the as_cell branch from f468720 to db7f900 Compare July 18, 2018 02:51

This comment has been minimized.

Sign in to view

revert Deref

8812c6b

F001 force-pushed the as_cell branch from db7f900 to 8812c6b Compare July 18, 2018 04:30

F001 force-pushed the as_cell branch from d55363f to eb26d96 Compare July 23, 2018 11:23

This comment has been minimized.

Sign in to view

use inherent method instead

489101c

F001 force-pushed the as_cell branch from eb26d96 to 489101c Compare July 23, 2018 12:59

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 23, 2018

bors merged commit 489101c into rust-lang:master Jul 23, 2018

F001 deleted the as_cell branch August 28, 2018 10:24

pnkfelix mentioned this pull request Oct 9, 2018

error: internal compiler error: Accessing (*_310) with the kind Write(Move) shouldn't be possible #54597

Closed

Implement rfc 1789: Conversions from &mut T to &Cell<T> #50494

Implement rfc 1789: Conversions from &mut T to &Cell<T> #50494

Conversation

F001 commented May 7, 2018 • edited Loading

rust-highfive commented May 7, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented May 14, 2018

joshtriplett commented May 14, 2018

cramertj May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSapin Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

bors commented May 17, 2018

eddyb commented May 18, 2018

F001 commented May 23, 2018

eddyb commented May 23, 2018

cramertj May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented May 26, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

F001 commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

eddyb commented Jul 13, 2018

This comment has been minimized.

F001 commented Jul 23, 2018

SimonSapin commented Jul 23, 2018

This comment has been minimized.

alexcrichton commented Jul 23, 2018

bors commented Jul 23, 2018

bors commented Jul 23, 2018

bors commented Jul 23, 2018

Implement rfc 1789: Conversions from `&mut T` to `&Cell<T>` #50494

Implement rfc 1789: Conversions from `&mut T` to `&Cell<T>` #50494

F001 commented May 7, 2018 •

edited

Loading

cramertj May 14, 2018 •

edited

Loading

SimonSapin Jun 27, 2018 •

edited

Loading

cramertj May 23, 2018 •

edited

Loading

eddyb May 28, 2018 •

edited

Loading