Implement rfc 1789: Conversions from `&mut T` to `&Cell<T>` by F001 · Pull Request #50494 · rust-lang/rust

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

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

Should definitely be repr(transparent).

Thanks. Fixed.

emilyalbini · 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

@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

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

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

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

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

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

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

Nit: extraneous space before :.

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

src/libcore/cell.rs

Nit: missing spaces after ,.

F001 · 2018-06-05T07:58:03Z

@pietroalbini What should I do next? I have no idea how to fix the travis failure.

eddyb · 2018-07-12T19:59:32Z

@alexcrichton the Deref impls supersede the less ergonomic Cell::from_mut_slice from the RFC.
We could keep just the uncontroversial Cell::from_mut method in the PR, and not add anything for being able to convert &Cell<[T]> (from &mut [T]) into &[Cell<T>].

I highly prefer Deref, but even with a dedicated method, I'd still rather have that step of "moving the Cell inward" remain orthogonal from "turning a &mut T into a &Cell<T>".

alexcrichton · 2018-07-12T20:43:40Z

@eddyb indeed yeah, but we felt that the Deref was a bit too magical to stabilize right now. If we move it to a standalone method we may later decide to stabilize as Deref still, it's basically just preserving the functionality to test it.

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

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

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

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

emilyalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2018

alexcrichton removed I-nominated labels Jul 12, 2018

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

This comment has been minimized.

Sign in to view

revert Deref

8812c6b

This comment has been minimized.

Sign in to view

use inherent method instead

489101c

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

Uh oh!

Conversation

F001 commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 7, 2018

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilyalbini commented May 14, 2018

Uh oh!

joshtriplett commented May 14, 2018

Uh oh!

cramertj May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonSapin Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 17, 2018

Uh oh!

eddyb commented May 18, 2018

Uh oh!

F001 commented May 23, 2018

Uh oh!

eddyb commented May 23, 2018

Uh oh!

cramertj May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented May 26, 2018

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

F001 commented May 28, 2018

Uh oh!

Choose a reason for hiding this comment

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

eddyb commented Jul 12, 2018 •

edited

Loading