-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
&move, DerefMove, DerefPure and box patterns #1646
Conversation
|
||
The `DerefMove` trait can't be called directly, in the same manner | ||
as `Drop` and for exactly the same reason - otherwise, this | ||
would be possible: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't the argument to deref_move
simply a &move self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because then it would drop self
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's not what the implementation of deref_move
wants, it can always to mem::forget
.
Permission wise, the deref_move
type somehow does not seem to make sense to me: The &mut self
argument means that when the (implicit) lifetime 'a
ends, ownership of the referred data (and the memory) is transferred back to the lender. On the other hand, the return type indicates that full, direct ownership of the data is returned, just the memory remains borrowed. That just does not work together. It seems impossible to safely implement this function, say, for a newtype alias of Box
.
Am I missing something?
EDIT: Notice that your example for why deref_move
cannot be called directly is also a direct consequence of not using &move
in the signature. Wrt. drop
, actually it seems to me that &move T
would be the better type to use for the argument of drop
, since it would actually reflect its destructive nature. I guess that ship has long sailed, though...
Anyway, I think I am arguing that indeed, drop
and deref_move
are similar, and both should use &move
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Ericson2314
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still won't be able to call deref_move
directly. For example, if you call deref_move
on a Box
, the "exterior" of the Box
is still present and needs to be dropped.
Well okay, it will only result in leaks, but that kind of random leaking feels bad.
DerefMove
can't really be typed under Rust's type-system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arielb1 That's equivalent to calling foo(&move *x)
where x: Box<T>
, right? When foo
returns, the exterior of the box (the heap allocation) is still around, but there is no ownership in there anymore.
I feel like &move
exactly adds the possibility to type drop
and deref_move
, or at least that's what it should do. If it doesn't, I failed to understand its intended meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DerefMove needs typestate to be safe and expressive.
At what point is the memory freed? E.g. a |
@oli-obk |
This is not |
unsafe impl<T> ops::DerefPure for Vec<T> {} | ||
|
||
// no `Drop` impl is needed - `RawVec` handles | ||
// that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A type is allowed to implement both Drop
and DerefMove
, correct?
I've copied some text below that I think does imply this is the case. If so, it would be good to have an example of that (even an artificial one that just calls println!
), explicitly spelling out the event ordering.
When such a type is dropped,
*x
(akax.deref_move()
) is dropped first if it was not moved from already, similarly toBox
today. Then the normal destructor and the destructors of the fields are called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure it is.
|
||
Add a `DerefMove` trait: | ||
```Rust | ||
pub trait DerefMove: DerefMut + DerefPure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of discussion, what if I want to implement DerefMove
on Cell
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I was trying to come up with an example where one might what to implement DerefMove
but not Deref
. Cell
was the only case I could think of where this might make sense but it's not really a useful example.
Some things I notice are missing:
Why is DerefPure required for DerefMove? I'd much rather have the guarantee of only being called once, and I'll copy my reply to the other thread over: This [ An example like: struct Foo(Box<u32>, Box<u32>);
fn main() {
let x = Box::new(Foo(Box::new(0), Box::new(1)));
drop(x.1);
} would be turned into struct Foo(Box<u32>, Box<u32>);
fn main() {
let x: Box<Foo>;
let x_inner: Foo;
let tmp0: Box<u32>;
let tmp1: Box<u32>;
let tmp2: Foo;
tmp0 = Box::new(0);
tmp1 = Box::new(1);
tmp2 = Foo(tmp1, tmp2);
x = Box::new(tmp2); x_inner = x.deref_move();
drop(x_inner.1);
// drop glue
Drop::drop(&mut x_inner.0);
Drop::drop(&mut x);
} |
I am not sure how we can guarantee the single-deref property in codegen. |
@arielb1 I mean, we guarantee single-drop; I imagine we could guarantee the same for deref_move. |
The way we guarantee single drop is by turning all duplicate drops to a no-op. If we want to go that way with |
@arielb1 Could you explain as if I don't understand what CSE is? |
Okay, I looked up common subexpression elimination and... it doesn't really seem to have anything to do with what we're talking about? If DerefMove isn't pure, then calling it twice won't be the same as calling it once, therefore you can't eliminate a common subexpression. |
It turns out there is a way to implement it, but it introduces more complexities.
I think I figured out a way to implement impure Could you try to write some interesting test cases for it? |
traits: it is possible to match on `Box` with box patterns, and to | ||
move out of it. | ||
|
||
User-defined types also want to make use of these features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have some examples of this, or at least something a bit more concrete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the Vec
example
Potentially interesting impure DerefMove example: fn f(v: Vec<Foo>, b: bool) {
if b {
drop(v[1]); // Calls deref_move()
}
drop(v[0]); // Conditionally calls deref_move()
} |
This does not compile. It is translated into: fn f(v: Vec<Foo>, b: bool) {
if b {
let tmp0 = DerefMove::deref_move(&move v);
//~^ NOTE value moved here
drop(tmp0[1]);
drop tmp0; // end of temporary scope
}
let tmp1 = DerefMove::deref_move(&move v);
//~^ ERROR use of moved value
drop(tmp1[0]);
drop tmp1; // end of temporary scope
drop v; // end of argument scope
} Of course, if fn f(v: Vec<Foo>, b: bool) {
if b {
drop((*v)[1]);
}
drop((*v)[0]);
drop v; // end of argument scope
// ^ (*v)[0] and (*v)[1] are already dropped - drop the rest and the allocation
} CHANGED: added drop for BTW, the code as written would not compile because you can't move out of an array - you have to use a pattern on the |
@arielb1 So this RFC changes the semantics of Box? struct Foo(i32);
impl Drop for Foo {
fn drop(&mut self) {
println!("{}", self.0);
}
}
fn f(x: Box<(Foo, Foo)>, b: bool) {
if b {
drop(x.1);
// RFC says x.0 gets dropped here
}
println!("X");
// On nightly, x.0 gets dropped here
}
fn main() {
f(Box::new((Foo(1),Foo(2))), true);
} |
@arielb1 When would Note: still thinking about an interesting test case. |
No. You mean the allocation? It is not moved anywhere, so it will be dropped along with |
A DerefPure Box and an impure Box-like type have a different destruction order? That's probably worth calling out explicitly in the RFC. |
I've spent a few days working on a MIR formalization to handle |
It might be possible to make impure DerefMove work like pure DerefMove by keeping track of a separate "have we called DerefMove yet" flag. Then we just call deref_move the first time we need the value, stash the returned value in a local variable, and reuse the previous value where appropriate. I'm not sure it's a good idea, but I don't see any obvious reason why it wouldn't work. |
Btw, is |
Mm. So, what I'm getting here is two things:
Hey, I've thought of something that nobody else seems to have brought up yet: what should If we do use type state for this eventually, will we also add type state for Drop? And if so, wouldn't that be a breaking change? This also made me realize that type state is, in many cases, doing the same thing as initialization analysis--that is, dropck is arguably just checking a special case of a type-state transition (any sort of consumption is equivalent to This also made me realize that type state is, in many cases, doing the same thing as initialization analysis--that is, dropck is arguably just checking a special case of a type-state transition. Specifically, you can implement automatic drops with three rules:
Question: What about There is a point to it--here's an example (assuming that
|
ping @arielb1 @nikomatsakis Status? |
Just a few notes from the @glaebhoerl suggested that all traits could have an @glaebhoerl found a crate using what you'd call "AsMutPure" here : https://docs.rs/atomic_ring_buffer/*/atomic_ring_buffer/trait.InvariantAsMut.html And Also @eddyb observed that if a I asked if a |
I don't get why anyone mentioned EDIT: Oh, you used |
I wrote it out with just
Initially I wrote this with a I'd envision smart pointers pointing their own Edit: I suppose this version might encounter your vtable problem anytime you want to |
I suppose one could maybe combine both approaches with some negative reasoning, so
although this still does not work for All this sounds needlessly complex just to avoid a few |
Will this create an ambiguity in the grammar? Can it be resolved by means of precedence? |
@rfcbot fcp close I would like to propose that we postpone this RFC. I think that the underlying ideas that it has are very good, as I would expect from @arielb1, but I don't feel that the "time is right" for this addition yet. Let me spell out my thoughts here. To start, though, I think it's helpful to enumerate the motivations
To those I would add the following:
Now I'll go through and spell out why I think we ought to close: Learning curve. A primary goal of the 2017 roadmap is to decrease the learning curve. My feeling is that this RFC moves somewhat counter to that goal, by introducing new "base reference" types into the language. The main counterargument to this that I have heard is roughly that by making things like In some cases, simpler solutions exist. Specifically for the problem of passing ownership values of unsized type, I would prefer to see a solution based on allocas. I think @arielb1 agrees, and indeed we elaborated the outlines of such a solution a few days back. =) Somewhat more controversially, I am not convinced that In some cases, more elaborate problems exist. If we were going to address extending the base reference types, I think we would want to "get more" than It might well be the case that we should address these problems as separate extensions to the language, but if we wind up with new types and syntax for all of them, that (in my mind) is a failure state. Rather, if we are going to extend lifetimes and references, I would want to strive for some more unifying concept that allows us to address more things with less mechanism ("eliminate the tradeoff"). I think that @RalfJung's work as well as @Ericson2314's "stateful mir" suggest some pointers as to what these mechanisms might be, not to mention the huge body of research that is out there around affine references, permissions, and the like. We expect to be learning more in the near future. In the case of the unsafe code guidelines in particular, I feel like work is very much still ongoing there. It seems premature to "bless" |
Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
I hope that pun is intended :D |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I agree that this is good idea at a bad time. I am very excited to see these ideas develop in the long run though. |
@nikomatsakis Could you maybe elaborate your thoughts (when you get the time) about everything that you ideally would like an extension to the reference system to buy us if you could have it all? I think having something like that written down somewhere would make it easier for everybody to frame ideas and proposals like this (and in particular future ones) into a "more global picture". I don't know, maybe this would belong into a "vision RFC" where it can be discussed, but a blog post with an internal threads could be a start. |
https://internals.rust-lang.org/t/idea-extending-the-rfc-process-experimental-features/4832 + something over-the-top-but-not-actually-new-theoretically like my stateful MIR would be a killer masters thesis :D. |
The final comment period is now complete. |
The FCP has elapsed, and it looks like there's still agreement that postponing is the right move for now. We'll surely be revisiting this topic in the future! Thanks @arielb1 for the RFC. |
cc #997 (Just a cc. A DerefMove RFC was not referred anywhere in the DerefMove issue 🙄) |
Given that it has been 2 years since this was last discussed, I think we should revist In order to simplify this RFC, I think we should only introduce trait DerefMove: DerefMut {
fn deref_move(&move self) -> &move Self::Target;
} I would also like to look at the work done in I would request adding a new function to trait Drop {
fn drop(&mut self);
// new, bikeshed name
fn drop_after_deref_move(&mut self) where Self: DerefMove {
/* leaks are fine, so this default should be fine */
}
} If you implement Another option is a new trait, trait DropAfterDerefMove: Drop + DerefMove {
fn drop_after_deref_move(&mut self);
} But I think that a new function in An example implementation given fn take_move<T: ?Sized>(t: &move T) {}
fn box_example<T: ?Sized>(bx: Box<T>) {
take_move(&move bx);
}
fn box_example_desugar<T: ?Sized>(bx: Box<T>) {
let __temp_bx_move: &move Box<T> = &move bx;
let __temp_bx_move_deref: &move T = <Box<T> as DerefMove>::deref_move(__temp_bx_move);
take_move(__temp_bx_move_deref);
Drop::drop_after_deref_move(&mut bx); // this is generated by the compiler, so it is fine
}
impl<T: ?Sized> Drop for Box<T> {
fn drop(&mut self) { /* drop T and dealloc heap */ }
fn drop_after_deref_move(&mut self) { /* dealloc heap (don't drop T) */ }
}
impl<T: ?Sized> DerefMove for Box<T> {
fn deref_move(&move self) -> &move T {
Unique::get_move(&move self.0)
}
} We should also consider how We should consider The most minor note for |
@RustyYato I agree |
Note: #![allow(incomplete_features)]
#![feature(deref_patterns, deref_pure_trait)]
use std::ops::{Deref, DerefMut, DerefPure};
struct MyBox<T: ?Sized>(Box<T>);
impl<T: ?Sized> Deref for MyBox<T> {
type Target = T;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl<T: ?Sized> DerefMut for MyBox<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
unsafe impl<T: ?Sized> DerefPure for MyBox<T> {}
fn main() {
match MyBox(Box::new(4)) {
deref!(4) => {},
_ => unreachable!(),
}
} |
@RustyYato FYI I've since then found that having |
I think I managed to get these features together into something I like.
Rendered.