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

Conversions from &mut T to &Cell<T> #1789

Merged
merged 18 commits into from
Jul 3, 2017
Merged

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Nov 13, 2016

A proposal about legitimizing the cast of a &mut T to a &Cell<T>.

Rendered

@Kimundi Kimundi added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Nov 13, 2016
@edef1c
Copy link

edef1c commented Nov 13, 2016

Having this trait makes no real sense — I'd rather see Cell::from_mut and Cell::from_mut_slice than a trait that covers two semantically incompatible conversions. Having a trait only makes sense when one can meaningfully abstract over its implementors.

@Kimundi
Copy link
Member Author

Kimundi commented Nov 13, 2016

True, the trait only exists to enable method call syntax - maybe thats too extravagant...

@edef1c
Copy link

edef1c commented Nov 13, 2016

Making this usable with method call syntax only serves to further obscure what's going on, and this whole proposal is already designed to do non-obvious (but very useful) things by its very nature.

@ahicks92
Copy link

How does this interact with the borrow checker? It seems to me that the point here is to get rid of the &mut so that you can do things with the Vec again, but if you're already iterating over the Vec via iter_mut, our current borrow checker isn't smart enough to let you. At least, in so far as I know.

Can you add an example to the RFC that shows usage of the trait? You've only got one that shows how it's currently a problem and one that shows how I can make it work in current Rust. I would like to see this so I can conceptualize it, because I really feel like the current borrow checker brings this crashing down.

@edef1c
Copy link

edef1c commented Nov 13, 2016

@camlorn Casting &mut T to &Cell<T> works today — this RFC merely proposes adding it to the stdlib. Concurrently obtaining another &mut T would be undefined behaviour, however.
Iterating and mutating concurrently can be done by converting an &mut [T] into &[Cell<T>] and then iterating over the cells (with iter, not iter_mut — that would create aliased mutable references)

@glaebhoerl
Copy link
Contributor

glaebhoerl commented Nov 13, 2016

Just for reference, there is a crate (by @huonw) providing this on crates.io. (See also #1106)

For this kind of thing, where the interesting part is not the writing down of the transmutes, but the clauses implicit in the language definition which make them OK, I do think it makes more sense to provide these directly in std than in an external crate. In other words my feeling is that, like Cell itself and Rc and others, these are essentially language primitives (part of the "trusted code base") that we're choosing to make available as std types and functions instead of as built-in syntax, for aesthetic reasons or whatever.

@Kimundi
Copy link
Member Author

Kimundi commented Nov 13, 2016

Alright, I added an example of how it would be used, and switched over to using Cell constructors instead.

Also adding the lang team, since the main point here is whether the language can guarantee the cast to be correct.

@Kimundi Kimundi added the T-lang Relevant to the language team, which will review and decide on the RFC. label Nov 13, 2016
@ahicks92
Copy link

I see it now. I focused in on the first conversion and missed the second.

What is the use case of the first conversion? This RFC is all about iteration, but I'm not seeing how &mut T->Cell<T> helps. I think it will in the future, but not until the borrow checker is no longer based on blocks. Is the future the point?

@eddyb
Copy link
Member

eddyb commented Nov 13, 2016

@camlorn Can you give an example of code you think won't work? If I transmute &mut T to &Cell<T> I can make as many copies as I want, and the compiler will treat the mutable borrow as expiring when the last &Cell<T> goes away (each lexically scoped, of course).

let v_slice: &[Cell<T>] = Cell::from_mut_slice(&mut v);

// example 1
for i in v_slice.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could just be for i in v_slice {

@glaebhoerl
Copy link
Contributor

glaebhoerl commented Nov 13, 2016

I think the &mut [T] -> &[Cell<T>] conversion (from_mut_slice) could be further decomposed into:

  1. &mut [T] -> &Cell<[T]> (Cell<[T]>::from_mut)
  2. &Cell<[T]> -> &[Cell<T>] -- perhaps using an Index impl?

So the API could look like this:

impl<T: ?Sized> Cell<T> {
    fn from_mut(val: &mut T) -> &Cell<T>;
}

impl<T> Index<usize> for Cell<[T]> {
    type Output = Cell<T>;
    fn index(&self, idx: usize) -> &Cell<T>;
}

impl<T> Index<Range<usize>> for Cell<[T]> {
    type Output = [Cell<T>];
    fn index(&self, idx: Range<usize>) -> &[Cell<T>];
}

impl<T> Index<RangeFrom<usize>> for Cell<[T]> {
    type Output = [Cell<T>];
    fn index(&self, idx: RangeFrom<usize>) -> &[Cell<T>];
}

impl<T> Index<RangeTo<usize>> for Cell<[T]> {
    type Output = [Cell<T>];
    fn index(&self, idx: RangeTo<usize>) -> &[Cell<T>];
}

impl<T> Index<RangeFull> for Cell<[T]> {
    type Output = [Cell<T>];
    fn index(&self, idx: RangeFull) -> &[Cell<T>];
}

So given a cell of a slice (&Cell<[T]>), you could index it however you like and get cells or slices of cells (&[Cell<T>]).

As an aside, it'd also analogously make sense to support &Cell<(A, B)> -> &(Cell<A>, Cell<B>), and likewise for tuples of other sizes and structs. It might be possible to express the former in the general case with #1582, the latter would almost certainly need compiler support. (No need to have these as part of this RFC, just mentioning it for the record.)

@eddyb
Copy link
Member

eddyb commented Nov 13, 2016

I don't think &Cell<[T]> -> &[Cell<T>] is sound, it violates the no interior references principle, although I'm not sure I can prove any problematic interactions. You'd need code that works on Cell<[T]> and doesn't take the existence of the transformation into account.

That said, I do quite like xs[i].set(x), there's a nice aesthetic to it.

@glaebhoerl
Copy link
Contributor

The idea is that internal references (&) together with shared mutability (Cell) is safe and sound as long as the structure is "flat": the references don't go under indirections (e.g. Box) or branches (enums). That means there is no way for the outer references to invalidate the memory observed by the inner references, because mutation cannot change the structure (because there is only one possible structure), only the data.

Furthermore, given two references to the same memory, one typed as &Cell<(X, Y, Z)>, and the other as &(Cell<X>, Cell<Y>, Cell<Z>), doing cell.set((x, y, z)) with the first is completely equivalent to doing cell.0.set(x); cell.1.set(y); cell.2.set(z) with the second, which is some kind of algebraic property I'm sure. (The same principle applies to arrays (slices), it's just nicer to illustrate with tuples.)

@ahicks92
Copy link

@eddyb
it's hard for me to come up with code here because I don't understand the proposed use case of the first conversion. The problem as I see it is that the lifetime of the &Cell<T> cannot be larger than the lifetime of the &mut T, consequently you still have the &mut T in the way of all the interesting manipulations you might want to do.

Let me put it this way instead: can someone show me why it's useful, and then I'll come back with an objection if I still have one?

@eddyb
Copy link
Member

eddyb commented Nov 13, 2016

@camlorn I've used it in a constraint solver. A highly heterogenous UI tree contained f32s indicating coordinates on screen, scattered inside it. They were mutably traversed and &mut f32 was turned into &Cell<f32>. Now once I had the &Cell<f32> I could copy it many types and use it as a "variable reference" in constraints, and every single one of them could update the original if they wanted to.

I could've done all of that through a Vec<&mut f32> and integer indices though, so maybe not the best example? It just felt cleaner, at least that part (you really don't want to read the actual "solver").

@ahicks92
Copy link

@eddyb
Ah, I kind of see. It sounds like the kind of thing where it's not useful save in larger programs. That seems workable, though, yes.

@Kimundi
Copy link
Member Author

Kimundi commented Nov 14, 2016

@camlorn: &mut T -> &Cell<T> is also the general approach you can use if you just have a iterator that yields you &mut T references, as oppose of a slice directly.

let mut set: HashSet<i32> = ...;

let tmp: Vec<&Cell<i32>> = set.iter_mut().map(Cell::from_mut).collect();
for i in &tmp {
    for j in &tmp {
        i.set(j.get()   i.get());
    }
}

@ahicks92
Copy link

@Kimundi
Oh. That's clever. Though I will say that I don't like the temporary Vec.

@Kimundi
Copy link
Member Author

Kimundi commented Nov 14, 2016

Sure, but the need for the vector is kind of independent to this feature, and only caused by the need to have random access to all elements.

Eg, the &Cell<i32>'s might just as well have been picked up by a consumer of the iterator directly and stashed away in whatever data structure works with them.

@ahicks92
Copy link

@Kimundi
I know. I still don't like the need for temporary intermediate data structures.

One of the problems of Rust (orthogonal to this RFC) is that some things like this suddenly require you to collect into a vec, which is fine until the container is large. Other such scenarios include deleting multiple keys from a map based on a predicate. The solution to this problem is for me to write up the extensions I think we should have and put them in an RFC, as I think most of them would be accepted without much argument.

Nevertheless, every time I see something that ends in .collect::<Vec<_>>() just because it's Rust and not C , I get a little sad inside.

@bluss
Copy link
Member

bluss commented Nov 25, 2016

Collecting into a vec is a workaround for when a more specific solution do not exist yet, for example an (random access?) iterator where each iterator element is a &Cell<T>. The canonicalization of this conversion does pave the way for such iterators to be written.

@aturon aturon self-assigned this Dec 22, 2016
@Havvy
Copy link
Contributor

Havvy commented Jan 18, 2017

No discussion has been going on in this RFC for almost a month. I'd nominate for FCP, but I don't know think I can.

@Kimundi Kimundi changed the title AsCell conversion from &mut T to &Cell<T> Conversion from &mut T to &Cell<T> Jan 24, 2017
@Kimundi Kimundi changed the title Conversion from &mut T to &Cell<T> Conversions from &mut T to &Cell<T> Jan 24, 2017
@Kimundi
Copy link
Member Author

Kimundi commented Jan 24, 2017

Agreed - I updated the RFC text a bit for a post-T: Copy world, but so far I haven't seen any actual objection, so I'm putting this into fcp. :)

@rfcbot fcp merge

@RalfJung
Copy link
Member

RalfJung commented Jun 2, 2017

Ah, I see... even with a &mut [T], the size cannot actually change because there is no way to change the size of the allocation. So even the following assertion would always hold:

fn truth_about_slices<T, F: FnOnce(&mut [T])>(x: &mut [T], f: F) {
  let len = x.len();
  f(x);
  assert_eq!(x.len(), len);
}

I was not aware of this, sorry for the noise.

Well, actually the reason is even deeper than not changing the size... if I think about trait objects, it's not legal even to change the type to sth. else of the same size, right? The existential quantifier is, so to speak, outside of the reference, because it is literally stored on the "outside". Hm.

@Kimundi
Copy link
Member Author

Kimundi commented Jun 2, 2017

Glad to see this RFC received so much formal scrutiny. :)

@nrc / @aturon: Sorry, I totally forgot to address your comments. I've pushed updated wordings.

@pnkfelix: Yeah, the slice conversion is independent to the core proposal, and weird in its own way. If there where major objections I'd be happy to remove it from this RFC, but apparently no one sees an actual issue with it. (notwithstanding @RalfJung's confusion 😄)

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 11, 2017

The final comment period is now complete.

@aturon aturon merged commit 246ff86 into rust-lang:master Jul 3, 2017
@aturon
Copy link
Member

aturon commented Jul 3, 2017

Huzzah! This RFC has been merged. Tracking issue.

@whitequark
Copy link
Member

Question, why is this merged but slice::ref_slice is deprecated? Aren't these fundamentally fairly similar?

@eddyb
Copy link
Member

eddyb commented Jul 4, 2017

@whitequark There is no slice <-> non-slice conversion here. I don't know why ref_slice is deprecated, it's really useful in quite a few cases.

@whitequark
Copy link
Member

@eddyb I mean this comment by @alexcrichton:

The libs team discussed this during triage today and the decision was to deprecate these functions. The standard library currently doesn't house many nuggets of functionality like this (e.g. see the recent discussions around Box::leak), and we've also deprecated similar functionality (Option::as_slice).

While certainly useful from time to time, it's not clear that these functions belong in the standard library right now. There can always be a helper crate implementing all sorts of conversions like this, however!

This seems to me to fall right into the "little nugget of functionality" area. I would rather we get ref_slice back, it's pretty useful in embedded contexts, if you have no Box!

@mzji
Copy link

mzji commented Jul 4, 2017

@whitequark Then should it go to libcore? These useful functions could be a part of core, just won't be exported by std, thus, they will available in #[no_std] but not in the "normal" crates.

@whitequark
Copy link
Member

I don't think we have precedent for that and it seems like a really odd decision to me.

@edef1c
Copy link

edef1c commented Jul 7, 2017

Note that ref_slice is available as a Cargo crate, should you need it.
It doesn't require any additional unsafe guarantees from the compiler, so it is fine to implement outside the stdlib.

@whitequark
Copy link
Member

It doesn't require any additional unsafe guarantees from the compiler, so it is fine to implement outside the stdlib.

This proposal could have been done the same way and without even any new RFCs, just mark Cell as #[repr(transparent)] I think.

@aturon
Copy link
Member

aturon commented Jul 12, 2017

I think the libs team would be open to reconsidering ref_slice (which was deprecated quite a long time ago), if there are more documented use-cases.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 17, 2017

For what it's worth, I still find uses for ref_slice relatively frequently. Luckily, in the compiler, we have a copy available. Here is a quick analysis of the various uses in the compiler. As one would expect, there are two main patterns.

Most commonly, you have a function that takes an &[T], and you have a &T. Here are the cases that seem to be like that:

src/librustc/middle/resolve_lifetime.rs:431:            self.resolve_elided_lifetimes(slice::ref_slice(lifetime_ref));
src/librustc_const_eval/check_match.rs:100:        self.check_patterns(false, slice::ref_slice(&loc.pat));
src/librustc_const_eval/check_match.rs:108:            self.check_patterns(false, slice::ref_slice(&arg.pat));
src/librustc_lint/unused.rs:93:                self.check_unused_mut_pat(cx, slice::ref_slice(&l.pat));
src/librustc_lint/unused.rs:106:            self.check_unused_mut_pat(cx, slice::ref_slice(&a.pat));
src/librustc_typeck/astconv.rs:850:        self.prohibit_type_params(slice::ref_slice(item_segment));
src/librustc_typeck/astconv.rs:925:        self.prohibit_type_params(slice::ref_slice(item_segment));
src/librustc_typeck/check/mod.rs:3987:        (def, Some(ty), slice::ref_slice(&**item_segment))
src/librustc_typeck/check/mod.rs:4119:                Some(e) => ref_slice(e),

Less commonly, you have an enum that sometimes has T and sometimes Vec<T>, and you want to return a &[T]. This occurs primarily in the MIR, except that we are returning Cow<[T]>:

src/librustc/mir/mod.rs:579:            Goto { target: ref b } => slice::ref_slice(b).into_cow(),
src/librustc/mir/mod.rs:586:                slice::ref_slice(t).into_cow(),
src/librustc/mir/mod.rs:587:            Call { destination: None, cleanup: Some(ref c), .. } => slice::ref_slice(c).into_cow(),
src/librustc/mir/mod.rs:595:                slice::ref_slice(target).into_cow()
src/librustc/mir/mod.rs:598:            Assert { ref target, .. } => slice::ref_slice(target).into_cow(),

I continue to think this function is a natural candidate for the standard library. It's a bit of unsafe code that is useful as a bit of glue in various scenarios and absolutely no dependencies or runtime implications. It is not entirely obvious that it is safe -- in particular, it requires understanding Rust's memory layout rules (which of course are not entirely defined). So putting it in the standard library is a way for us to advertise that indeed it is safe. =) It's also something that people might not realize is possible, and hence they wouldn't know to search for the crate.

It's also ok for it to exist in a crate, of course, but I don't think there's any good reason to put it there.

@whitequark
Copy link
Member

I'd like to get it back in std. What's the process here? Is it an RFC or just a PR?

@aturon
Copy link
Member

aturon commented Jul 17, 2017

@whitequark I think it's a small enough addition that a PR suffices. Feel free to CC me.

@comex
Copy link

comex commented Aug 1, 2017

This is late, but I have a concern about this RFC's design. To go from &mut [T] to &[Cell<T>] it requires going through &Cell<[T]>. That's a cute decomposition and works fine - for Cell.

However, a natural extension to the proposed use case would be a thread safe variant: in particular, a way to go from &mut u8 to &AtomicU8 and from &mut [u8] to &[AtomicU8], and similarly for other atomic types. That would be its own RFC of course, but notably, the API design couldn't directly mirror this one; there would have to be a direct conversion function/method from &mut [u8] to &[AtomicU8], since there's no type like &Atomic<[u8]> to serve as an intermediary (after all, you can't atomically mutate arbitrarily large slices). For the sake of consistency, therefore, it might make sense to revise this RFC to add a direct conversion from &mut [T] to &[Cell<T>].

@edef1c
Copy link

edef1c commented Aug 3, 2017

@comex You also can't swap out the contents of a Cell<[T]> with another [T] freely, so that seems analogous?

@glaebhoerl
Copy link
Contributor

I was going to reply with something similar but I think their point was that there is no Atomic<T> type in the first place that you could instantiate to Atomic<[T]>.

@comex
Copy link

comex commented Aug 3, 2017

@edef1c Ah... I kind of forgot about that. It might be possible to support the full Cell API on Cell<[T]> in the future with stack-based DSTs. But if that doesn't happen / isn't desired, it only strengthens my belief that the design should be changed. If Cell<[T]> does not provide any of the normal Cell functionality, what's the point of having it? It's not 'modular' to go through an intermediary type if the intermediary type is completely useless - other than for the purpose of being an intermediary for that exact conversion.

But in any case, as @glaebhoerl said, there is no Atomic<T>. Perhaps there should be one in the future, but it would be quite strange to have an Atomic<T> that can't actually be read/written atomically...

bors added a commit to rust-lang/rust that referenced this pull request Nov 2, 2017
Bring back slice::ref_slice as slice::from_ref.

These functions were deprecated and removed in 1.5, but such simple
functionality shouldn't require using unsafe code, and it isn't
cluttering libstd too much.

The original removal was quite contentious (see #27774), since then
we've had precedent for including such nuggets of functionality (see rust-lang/rfcs#1789),
and @nikomatsakis has provided a lot of use cases in rust-lang/rfcs#1789 (comment).
Hence this PR.

I'm not too sure what to do with stability, feel free to correct me.
It seems pointless to go through stabilization for these functions though.

cc @aturon
@Centril Centril added A-conversions Conversion related proposals & ideas A-machine Proposals relating to Rust's abstract machine. A-cell Proposals relating to interior mutability. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cell Proposals relating to interior mutability. A-conversions Conversion related proposals & ideas A-machine Proposals relating to Rust's abstract machine. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.