Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign up&move, DerefMove, DerefPure and box patterns #1646
Conversation
arielb1
force-pushed the
arielb1:missing-derefs
branch
from
b64b66e
to
5d68cad
Jun 9, 2016
oli-obk
reviewed
Jun 9, 2016
|
|
||
| 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: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
RalfJung
Jun 15, 2016
•
Member
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
arielb1
Jun 15, 2016
•
Author
Contributor
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.
This comment has been minimized.
This comment has been minimized.
RalfJung
Jun 24, 2016
•
Member
@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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
At what point is the memory freed? E.g. a |
This comment has been minimized.
This comment has been minimized.
|
@oli-obk |
This comment has been minimized.
This comment has been minimized.
|
This is not |
pnkfelix
reviewed
Jun 9, 2016
| unsafe impl<T> ops::DerefPure for Vec<T> {} | ||
| // no `Drop` impl is needed - `RawVec` handles | ||
| // that |
This comment has been minimized.
This comment has been minimized.
pnkfelix
Jun 9, 2016
Member
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 toBoxtoday. Then the normal destructor and the destructors of the fields are called.
This comment has been minimized.
This comment has been minimized.
Stebalien
reviewed
Jun 9, 2016
|
|
||
| Add a `DerefMove` trait: | ||
| ```Rust | ||
| pub trait DerefMove: DerefMut + DerefPure { |
This comment has been minimized.
This comment has been minimized.
Stebalien
Jun 9, 2016
Contributor
For the sake of discussion, what if I want to implement DerefMove on Cell?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Stebalien
Jun 10, 2016
Contributor
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.
This comment has been minimized.
This comment has been minimized.
|
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);
} |
This comment has been minimized.
This comment has been minimized.
I am not sure how we can guarantee the single-deref property in codegen. |
This comment has been minimized.
This comment has been minimized.
|
@arielb1 I mean, we guarantee single-drop; I imagine we could guarantee the same for deref_move. |
This comment has been minimized.
This comment has been minimized.
|
The way we guarantee single drop is by turning all duplicate drops to a no-op. If we want to go that way with |
This comment has been minimized.
This comment has been minimized.
|
@arielb1 Could you explain as if I don't understand what CSE is? |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
I think I figured out a way to implement impure Could you try to write some interesting test cases for it? |
nrc
added
the
T-lang
label
Jun 10, 2016
nrc
reviewed
Jun 10, 2016
| 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. |
This comment has been minimized.
This comment has been minimized.
nrc
Jun 10, 2016
Member
It would be nice to have some examples of this, or at least something a bit more concrete
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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 comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
@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);
} |
This comment has been minimized.
This comment has been minimized.
|
@arielb1 When would Note: still thinking about an interesting test case. |
This comment has been minimized.
This comment has been minimized.
|
No. You mean the allocation? It is not moved anywhere, so it will be dropped along with |
This comment has been minimized.
This comment has been minimized.
|
A DerefPure Box and an impure Box-like type have a different destruction order? That's probably worth calling out explicitly in the RFC. |
This comment has been minimized.
This comment has been minimized.
|
I've spent a few days working on a MIR formalization to handle |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Btw, is |
This comment has been minimized.
This comment has been minimized.
Ah, I didn't realize you were still on vacation, sorry. Indeed, It's no beach read :). |
Ericson2314
referenced this pull request
Jul 21, 2016
Closed
Tracking issue for `NonZero`/`Unique`/`Shared` stabilization #27730
ubsan
reviewed
Aug 12, 2016
|
|
||
| ## &move | ||
|
|
||
| Add a new mutability `move`. `&move` references are references that own their |
This comment has been minimized.
This comment has been minimized.
ubsan
Aug 12, 2016
•
Contributor
This isn't a new mutability. (mutable) Local variables already have this mutability.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ubsan
Aug 14, 2016
Contributor
That doesn't mean it doesn't exist. I believe prvalues (function returns, constants, etc.) also have this mutability.
This comment has been minimized.
This comment has been minimized.
arielb1
Aug 14, 2016
Author
Contributor
That's a matter of how you structure the specification - bikeshedding.
ubsan
reviewed
Aug 12, 2016
| whether `DerefPure` is implemented: | ||
|
|
||
| ```Rust | ||
| fn exmaple() { |
This comment has been minimized.
This comment has been minimized.
sfackler
referenced this pull request
Oct 28, 2016
Closed
Proposed backward-compatible syntax for owning and output references #1780
This comment has been minimized.
This comment has been minimized.
NXTangl
commented
Oct 29, 2016
•
|
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
|
This comment has been minimized.
This comment has been minimized.
|
ping @arielb1 @nikomatsakis Status? |
burdges
referenced this pull request
Feb 2, 2017
Closed
Allow fields in traits that map to lvalues in impl'ing type #1546
This comment has been minimized.
This comment has been minimized.
burdges
commented
Feb 2, 2017
•
|
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 |
This comment has been minimized.
This comment has been minimized.
|
I don't get why anyone mentioned EDIT: Oh, you used |
This comment has been minimized.
This comment has been minimized.
burdges
commented
Feb 2, 2017
•
|
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 |
This comment has been minimized.
This comment has been minimized.
burdges
commented
Feb 3, 2017
|
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 |
This comment has been minimized.
This comment has been minimized.
DemiMarie
commented
Feb 7, 2017
|
Will this create an ambiguity in the grammar? Can it be resolved by means of precedence? |
This comment has been minimized.
This comment has been minimized.
|
@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" |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Feb 11, 2017
•
|
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. |
This comment has been minimized.
This comment has been minimized.
I hope that pun is intended :D |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Feb 23, 2017
|
|
rfcbot
added
the
final-comment-period
label
Feb 23, 2017
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
@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. |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Mar 5, 2017
|
The final comment period is now complete. |
This comment has been minimized.
This comment has been minimized.
|
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. |
aturon
closed this
Mar 6, 2017
This comment has been minimized.
This comment has been minimized.
|
cc #997 (Just a cc. A DerefMove RFC was not referred anywhere in the DerefMove issue |
arielb1 commentedJun 9, 2016
•
edited
I think I managed to get these features together into something I like.
Rendered.