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

RFC for DerefMove #2439

Closed
wants to merge 2 commits into from
Closed

RFC for DerefMove #2439

wants to merge 2 commits into from

Conversation

alercah
Copy link
Contributor

@alercah alercah commented May 13, 2018

Rendered

Part of #997.

@comex
Copy link

comex commented May 13, 2018

👍, but note that Cow::into_owned is probably not a good candidate forDerefMove since it may perform an expensive operation (cloning).

@alercah
Copy link
Contributor Author

alercah commented May 13, 2018

Right, that's why it doesn't implement DerefMut either, I believe.

this case, autodereference could either dereference via `deref_move` or via
`deref` or `defer_mut`. In this case, it will prefer the latter (for `deref`, it
is a special case of the `Copy` rule above, but for `deref_mut` it is not) to
avoid unnecessarily moving out of the receiver.
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph could use some more clarification and examples. Why prefer &mut to &? Why not call Deref when the Target type of the receiver is a & and DerefMut when the target type of the receiver is &mut? What does DerefMove have to do with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a bit confusing. If the Target is &T, and there's a self method on &T, then since &T is Copy, the rules above will mean that we use deref and then copy. But with Target = &mut, there are two options: either call deref_mut getting an &mut &mut T and then dereference that, or just call deref_move to get a &mut T; the former should be preferred.

I'll try to clarify this paragraph.

Moreover, the behaviour is arguably a bug because it is taking a mutable borrow
of an immutable variable (and one can modify the above example to get an error
message implying exactly that), though the semantics are more like the unique
immutable borrows used by closures.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that this feels like a bugfix.

The status quo is disfavoured because it requires special-casing `Box` in the
compiler, privileging it over other smart pointers. While there's no obvious
call in the stdlib for `DerefMut` other than `Box` (see below), it prevents
other library authors from writing similar code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean DerefMove rather than DerefMut?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, will fix.

@bstrie
Copy link
Contributor

bstrie commented May 13, 2018

I find the DerefMut bound interesting because it means that the Deref traits will operate in exactly the opposite, er, direction from the Fn traits: DerefMove implies DerefMut implies Deref, whereas Fn implies FnMut implies FnOnce. Not sure if this is somehow profound or just an interesting symmetry. :)

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Mostly just copy-editing nits.

Looks pretty good, although it's unclear to me whether the motivation is worth the effort? Would be nice to get some ecosystem usecases.

value is dynamically-sized and thus cannot be moved onto the stack. Indeed, one
can call a `Box<T>` where `T` is a concrete type implementing `FnOnce()`.
`DerefMove` would allow `Box<FnOnce()>` to continue not working, just without a
special case in the compiler to allow it to do so.
Copy link
Contributor

Choose a reason for hiding this comment

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

These last two sentences are jumbled up, and I'm not actually 100% clear what they're trying to express. Maybe just:

Although DerefMove would allow smart pointers containing a concrete FnOnce instance to be called.

And it may be worth noting that if by-value DSTs ever get implemented (which is theoretically possible and has long been desireable), then the combination of that feature with DerefMove would in fact be sufficient to make Box<FnOnce()> work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By-value DSTs are sufficient and necessary for Box<FnOnce()>, even in absence of DerefMove. As you noted, concrete FnOnce instances can be called with DerefMove and they can be called from a Box today.

I'll try to clarify this.

# Motivation
[motivation]: #motivation

Currently, two smart pointer traits provide access to values contained in smart
Copy link
Contributor

Choose a reason for hiding this comment

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

nix the first instance of "smart pointer", I think ("two traits provide...")

a number of places, such as method calls and index expressions.

These two traits, however, only dereference references to produce references to
the contained value. They do not offer the option to dereference by value,
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is a mouthful. Perhaps:

Unfortunately, these two traits only provide borrowed access to the contained value.

`deref_move` is only called in contexts where a value is needed to be moved out.
This can occur either because an explicit dereference is used in a context where
a value is to be moved, or because autoderef selects a method taking `self` (or
`Box<self>`) as a parameter. In both cases, when evaluating whether to call
Copy link
Contributor

Choose a reason for hiding this comment

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

I forget, is Box<self> magic or would this also work for MyBox<self>? (should this be made explicit?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Box<self> is magic as a receiver type, and I'm not proposing to eliminate that here. It's only mentioned here for completeness.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is sort of getting fixed with arbitrary self types, right? Or is this specifically about the Box<self> syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, once arbitrary receiver types are accepted then Box won't be magical. But that shouldn't affect this RFC one way or another---it's just that I felt I needed to mention that not all by-value receivers are Self. Overload resolution will continue the way it normally is (note: I said "autoderef selects" above, but I really meant overload resolution).


## Copy behaviour is unintuitive in generic cases

If a type has nontrivial/side-effectful `defer_move`, it may be slightly
Copy link
Contributor

Choose a reason for hiding this comment

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

deref_move typo

## Breaking change to an edge case of `Box<&mut T>`

Currently, a `Box<&mut T>` variable [can be
dereferenced](https://play.rust-lang.org/?gist=5b3f046ecf29da8f00fde3bd1d2a3df4&version=nightly&mode=debug)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if you inlined this example, so that these documents are less prone to bitrot.


The status quo is disfavoured because it requires special-casing `Box` in the
compiler, privileging it over other smart pointers. While there's no obvious
call in the stdlib for `DerefMut` other than `Box` (see below), it prevents
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: DerefMove

to `DerefMove`, which would consume a collection and return a single element. I
have not included this in this RFC because no compelling use case was presented,
and the behaviour of `let s = v[0];` actually consuming `v` seems likely to add
cognitive overhead to the language.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i just thought of a use-case; arrays. IndexMove is necessary to get e.g. a Box out of an Array. But in that case into_iter or slice patterns are probably better.

Copy link

Choose a reason for hiding this comment

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

Well, there's definitely a use case for an Index trait which returns things by value. For example, consider a type that wants to expose an API similar to Vec<bool>, but is implemented as a bit set, so indexing into it can't return an existing &bool reference. However, you wouldn't usually want to have self be by value – but afaik that provides the most flexibility, since you could always do impl<'a> DerefMove for &'a MyBitSet. (By itself, this would only allow reading values, not writing them, but #997 also proposed an IndexSet that would support writing.)

Copy link
Contributor

Choose a reason for hiding this comment

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

IndexGet is the intended solution to that problem, but yes it could just be provided through IndexMove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially worked on all versions proposed by #997, but quickly abandoned that when I realize that the IndexGet and IndexSet make more sense as a separate RFC. Note that you couldn't impl DerefMove on a reference since it would have to have the same Target as the provided Deref instance; I don't think that the same restriction would necessarily apply to IndexMove, however.

@scottmcm scottmcm added 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. labels May 14, 2018
The following trait is defined in `std::ops`:

```rust
trait DerefMove : DerefMut {
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me why this should require DerefMut and not just Deref. I can certainly imagine wrapper types that aren't DerefMut because the constructor checked an invariant, but that doesn't need to block DerefMove because once the inner value has been moved out, the invariant is no longer relevant. Example I pulled out of a hat: struct AlignedPtr<T>(*const T); wouldn't be DerefMut, but could be Deref and DerefMove.

Copy link
Contributor

@pythonesque pythonesque May 14, 2018

Choose a reason for hiding this comment

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

Yes, I agree. DerefMut -> Deref only makes sense because the caller of deref_mut can always reborrow the returned value to &Target, and worst case the Deref implementation can always be implemented to do the exact same thing. By contrast, DerefMut can't always be implemented by the caller of deref_move, so we don't have DerefMove -> DerefMut.

Actually, the DerefMove -> Deref implication does not work either, in the sense that you can't always implement Deref with a target for which you can implement DerefMove, even if the target is Copy: this is easy to see since you can implement DerefMove with any (sized) type as a target.

There is an implication the other way (Deref -> DerefMove where Target: Copy) so hypothetically the thing you actually want is something like this:

trait DerefMove {
  type Target;
  // ...
}

impl<T> DerefMove for T where T: Deref, <T as Deref>::Target: Copy {
  // We can avoid consuming ourselves *and* fork over the deref target.
  type Target = (T, <T as Deref>::Target);
 // [insert copying implementation using `Deref` on `self`]
}

Actually using this implementation would be irritating, but it gets the idea across (namely, how the Copy special case works). In general we can also implement Deref for a type that we can't move (for instance, by reborrowing a static instance of the type), so of course this implication also fails to hold for non-copy Targets, and this holds for DerefMut as well (e.g. we could be holding a mutable reference to a type for which we have only one instance, so we can't swap or replace it). One could probably argue that scenarios like this are abuses of Deref, DerefMove, or both, but either way it's not a forced decision, and I'm not entirely sure they need to share a target: the strategy is to ignore DerefMove entirely when T: Deref has a Copy target, so requiring them to have the same type doesn't accomplish anything, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rationale is below, but I had not considered this use case. The ergonomic drawbacks may be worth it if this use case is worth supporting.

Copy link
Contributor Author

@alercah alercah May 14, 2018

Choose a reason for hiding this comment

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

The behaviour of ignoring DerefMove when Target : Copy for Deref would be complicated if the Target types were different; that would possibly create scenarios where a generic smart pointer would fail for a non-Copy Target because DerefMove is ignored but Deref has a different Target.

This would also complicate autoderef and in particular its relationship with overload resolution, probably unbearably so. I think it would be so hard to use a type with different Deref and DerefMove Targets that, if we decided to make the two independently implementable, the compiler must enforce that the Targets are identical.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I suppose something like struct TaggedPtr(usize); is a case where it could plausibly be DerefMove but not Deref -- the move version is cheap, just masking away the extra bits in the alignment, and conceptually reasonable, since it's still a pointer -- but it can't be Deref because it doesn't actually have the real pointer to borrow.

That would need that "split a trait into two" RFC, I think, to introduce DerefTarget or something that just provides the associated type. But it might also be too much of a stretch to bother supporting even if we could...

Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: this trait requires a Sized super trait and where Self::Target: Sized bound.

Copy link
Contributor

Choose a reason for hiding this comment

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

@scottmcm The TaggedPtr case could still be Deref, I think, since usize is Copy, as long as the target was the pointed-to value, not the pointer itself. It would require unsafe code to cast the masked usize to an actual pointer with a lifetime, but that would require unsafe code anyway--it would still (presumably) be safe.

Copy link
Member

Choose a reason for hiding this comment

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

@pythonesque Good point! I forget that pointers get to cheat 🙂

Copy link
Contributor

@pythonesque pythonesque left a comment

Choose a reason for hiding this comment

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

Overall I like it, but I think it will be more useful if/when by-value DSTs are implemented and think there's a good case for lowering the DerefMut bound to a Deref, since as far as I can tell there is absolutely no relationship between when you can implement Deref or DerefMut and when you can implement DerefMove unless your target is Copy, in which case the current strategy is to ignore the DerefMove implementation anyway. You could also remove the Deref without any bad logical consequences, I think, but I understand that there's almost no reason to want that that isn't abuse.

The following trait is defined in `std::ops`:

```rust
trait DerefMove : DerefMut {
Copy link
Contributor

@pythonesque pythonesque May 14, 2018

Choose a reason for hiding this comment

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

Yes, I agree. DerefMut -> Deref only makes sense because the caller of deref_mut can always reborrow the returned value to &Target, and worst case the Deref implementation can always be implemented to do the exact same thing. By contrast, DerefMut can't always be implemented by the caller of deref_move, so we don't have DerefMove -> DerefMut.

Actually, the DerefMove -> Deref implication does not work either, in the sense that you can't always implement Deref with a target for which you can implement DerefMove, even if the target is Copy: this is easy to see since you can implement DerefMove with any (sized) type as a target.

There is an implication the other way (Deref -> DerefMove where Target: Copy) so hypothetically the thing you actually want is something like this:

trait DerefMove {
  type Target;
  // ...
}

impl<T> DerefMove for T where T: Deref, <T as Deref>::Target: Copy {
  // We can avoid consuming ourselves *and* fork over the deref target.
  type Target = (T, <T as Deref>::Target);
 // [insert copying implementation using `Deref` on `self`]
}

Actually using this implementation would be irritating, but it gets the idea across (namely, how the Copy special case works). In general we can also implement Deref for a type that we can't move (for instance, by reborrowing a static instance of the type), so of course this implication also fails to hold for non-copy Targets, and this holds for DerefMut as well (e.g. we could be holding a mutable reference to a type for which we have only one instance, so we can't swap or replace it). One could probably argue that scenarios like this are abuses of Deref, DerefMove, or both, but either way it's not a forced decision, and I'm not entirely sure they need to share a target: the strategy is to ignore DerefMove entirely when T: Deref has a Copy target, so requiring them to have the same type doesn't accomplish anything, right?

```

If the `Target` type is `Copy`, then `DerefMove` will not be used, and instead
`Deref` will be used and the resulting referenced value will be copied:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think understand why this is necessary after staring at it for a while (since otherwise existing code that expects to be able to copy out of *x would be broken as it would move out of b incorrectly). But as I noted above: it's not clear to me why the Target of DerefMove has to be the same as that for Deref since the rule seems to be that if the target for Deref is Copy we ignore DerefMove. You could always lint for implementations of DerefMove with targets that are known to be Copy at definition time.

`deref_move` is only called in contexts where a value is needed to be moved out.
This can occur either because an explicit dereference is used in a context where
a value is to be moved, or because autoderef selects a method taking `self` (or
`Box<self>`) as a parameter. In both cases, when evaluating whether to call
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sort of getting fixed with arbitrary self types, right? Or is this specifically about the Box<self> syntax?

These approaches would, however, make diverging implementations (that is,
implementations for which the behaviour of the various deref traits is
unintuitive because they don't actually refer to the same underlying value) more
difficult to write, and we want to avoid that. And they would not fundamentally
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean easier to write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry. See my above comment that if we did this we may actually need to enforce that they match.

Copy link
Contributor

@pythonesque pythonesque May 14, 2018

Choose a reason for hiding this comment

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

@alercah

The behaviour of ignoring DerefMove when Target : Copy for Deref would be complicated if the Target types were different; that would possibly create scenarios where a generic smart pointer would fail for a non-Copy Target because DerefMove is ignored but Deref has a different Target.

Can you elaborate on what you mean by "fail"? I assume all that would happen is that the Deref target and implementation would be used on autoderef, which seems no different than what would happen if the targets were the same.

This would also complicate autoderef and in particular its relationship with overload resolution, probably unbearably so.

Wouldn't it always be unambiguous which implementation to use in any given context, in the same way that Copy itself is? I may be missing something. Or is the problem not about Copy, but that the two types could have methods with the same name, and you couldn't tell which one to call or prioritize one over the other? If that's your concern I definitely see why you would want the types to match.

`DerefMut`, then the first `b.consume()` becomes illegal because `deref_mut`
becomes preferred and `b` is immutable. By requiring that `DerefMove :
DerefMut`, this edge case cannot be encountered: this code will always be
illegal unless `b` is made mutable.
Copy link
Contributor

Choose a reason for hiding this comment

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

I sort of understand why this is a bad edge case, but as was pointed out above there are useful situations where you can't feasibly implement DerefMut; however, this would still be a bit surprising in those cases. I think what this edge case actually suggests is that Box gets it "right" mostly by accident; if it is important, the rules for mut bindings should be tweaked around DerefMove somehow (possibly that &mut methods on a target of DerefMove should require a mut binding, though I'm not sure how you justify that). In any case, allowing this edge case still seems preferable to me to ruling out DerefMove on !DerefMut types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behaviour of Box here is the same as the unique immutable reference that closures use to get around the difficulty of an immutable variable holding a mutable reference, which can be expressed in MIR but not in Rust. We could consider making deref_mut take &uniq self, if that type existed, and then this problem would not exist. Alternatively, simply dropping the notion of immutable variables would also solve this.

## Implementors

It's not clear to me which, if any, of the smart pointers above should implement
`DerefMove` other than `Box`. It requires discussion of whether inadvertently
Copy link
Contributor

Choose a reason for hiding this comment

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

Vec should likely have an implementation; I expect it can't right now because of sizedness issues. Actually, that suggests to me that allowing by-value DSTs is pretty important to making DerefMove work well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I will revise the RFC to discuss the interaction with by-value DSTs more, since that seems very fundamental.

Copy link

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

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

Some concerns that I had while reading this. Mainly other ways that Box's specialness can be observed.

conflict between current `Box` behaviour and the behaviour specified in this
RFC that isn't explicitly called out as being different, it should be considered
a bug in the RFC and either this RFC should be corrected or more discussion
should ensue.

Choose a reason for hiding this comment

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

Rust allows this:

struct A<T: ?Sized> {
    x: String,
    y: T,
}

fn foo(b: Box<A<dyn Send>>) {
    let r = (*b).x; // explicit deref for clarity
}

Choose a reason for hiding this comment

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

Also moving from b.0 doesn't drop the rest of *b until b is dropped.

struct D(i32);

impl Drop for D {
    fn drop(&mut self) {
        println!("drop({})", self.0);
    }
}

fn main() {
    let mut b = Box::new((D(0), D(1)));
    drop(b.0);
    println!("Hi!");
    b = Box::new((D(2), D(3))); // old value of b is dropped.
    println!("Hi again!");
}

Output:

drop(0)
Hi!
drop(1)
Hi again!
drop(2)
drop(3)

effect on Cargo packages.

# Rationale and alternatives
[alternatives]: #alternatives

Choose a reason for hiding this comment

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

A more comprehensive solution like #1646 should be mentioned as an alternative.

```

If the `Target` type is `Copy`, then `DerefMove` will not be used, and instead
`Deref` will be used and the resulting referenced value will be copied:

Choose a reason for hiding this comment

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

This wouldn't be sufficient:

fn foo(b: Box<(String, i32)>) {
    let r = (*b).1; // copy!
    drop(b);
}

@ssokolow
Copy link

ssokolow commented May 16, 2018

@bstrie

I find the DerefMut bound interesting because it means that the Deref traits will operate in exactly the opposite, er, direction from the Fn traits: DerefMove implies DerefMut implies Deref, whereas Fn implies FnMut implies FnOnce. Not sure if this is somehow profound or just an interesting symmetry. :)

It could just be that I'm very tired right now and can never get a proper grasp of covariance and contravariance as they apply to Rust to stick in my mind for long, even when I am fully alert, but I feel as if what I read in the Subtyping and Variance page of the nomicon should resolve the profound vs. interesting question as merely expected.

@mikeyhew
Copy link

I'm glad to see someone is pushing forward on this again.

As others have mentioned, I think we will want DerefMove to support DST someday. Since the deref_move method proposed here returns the target type by value, it can't work if Target: ?Sized, even with the Unsized Rvalues RFC, which does not support DST return values.

That being said, this version of deref_move works right now for Sized types, and since we don't have the language features yet that would make it work for DST (either &move references, or some way to return DSTs by-value), I think it's fine to use this version, and stabilize the effects of it for Box and whatever other types implement it, but keep the trait itself unstable so that it can be changed to support DST in the future.

@DDOtten
Copy link

DDOtten commented May 16, 2018

How about the name IntoDeref instead of DerefMove. We already have at least two kinds of names for taking ownership with things like IntoIter and FnOnce. I think it would be good use one of these for learnability.

@clarfonthey
Copy link
Contributor

Now that I think of it, I definitely agree with @DDOtten. The convention for Iterator should be clear enough that IntoDeref is the right choice.

@CryZe
Copy link

CryZe commented May 18, 2018 via email

@bjorn3
Copy link
Member

bjorn3 commented May 18, 2018

I vote for the name DerefOnce in parallel to FnOnce

Deref Fn
DerefMut FnMut
DerefOnce FnOnce

@CryZe
Copy link

CryZe commented May 18, 2018

Deref{Into/Move/Once} doesn't imply that you can only deref it a single time though, as it also implies DerefMut and transitively Deref. However FnOnce doesn't imply FnMut or Fn, and thus the trait as a trait bound itself, only allows you to call the function once. So while Once sounds nice on a surface level, the dependencies are quite a bit different.

@bjorn3
Copy link
Member

bjorn3 commented May 18, 2018

@CryZe hadn't thought about that.

@mikeyhew
Copy link

mikeyhew commented May 18, 2018

The name DerefInto also makes a bit more sense for this by-value version of DerefMove, compared to previous proposals involving &move references. Naming it DerefInto would allow us to add an &move version later and name it DerefMove. One could be implemented in terms of the other to avoid breaking backward compatibility:

// potential trait hierachy
// EDIT: the first version of this had DerefMove: DerefInto, and
//           `where Self: Sized` on the deref_into method. It turns
//           out that that doesn't work the way I expected — you still can't
//           implement DerefInto where Self: ?Sized or Self::Target: ?Sized

trait DerefInto: DerefMut {
    fn deref_into(self) -> Self::Target;
}

trait DerefMove {
    fn deref_move(&move self) -> &move Self::Target;
}

impl<T> DerefInto for T
where
    T: DerefMove
{
    fn deref_into(self) -> Self::Target where Self: Sized {
        self.deref_move().deref_into()
    }
}

EDIT: Here is a working demo for Box<T>, using Move<'a, T> syntax. I had to change the signature of deref_move to take a closure, to ensure that the return value of <Box as DerefMove>::deref_move lives long enough without leaking memory.
Gist: https://gist.github.com/mikeyhew/8ac8bbb77803ee239daeeffc7028a8b7
Playground: https://play.rust-lang.org/?version=nightly&mode=debug&gist=8ac8bbb77803ee239daeeffc7028a8b7

Thoughts?

@joshtriplett
Copy link
Member

Name bikeshedding aside (I can see a case for either DerefInto or DerefMove), this seems like a good idea. 👍

The ability to call self methods on a wrapper type seems reasonable.

@alercah
Copy link
Contributor Author

alercah commented Jun 5, 2018

I apologize I haven't come back to this. I got stumped by @matthewjasper's examples. After some more thinking today, I think it might be doable, but we would need something like &move in MIR.

@Ericson2314
Copy link
Contributor

I still think we'll need a &move to do DerefMove right. But temporary making an unstable DerefMove like this is a good first step. The compiler support for Box is really an abomination and we should strive to get rid of it.

@mikeyhew
Copy link

I didn't notice @matthewjasper's comments before. It looks like DerefInto wouldn't be backward compatible with Box because of the order in which things are dropped.

To get the right drop order, I'm pretty sure either a cleanup RAII type is needed, or deref_move needs to take a closure. Here's both versions using &move references:

trait DerefMove: DerefMut {
    type Cleanup;

    // explicit lifetimes for clarity
    fn deref_move<'a>(&'a move self) -> (&'a move Self::Target, Self::Cleanup);
}

trait DerefMove: DerefMut {
    // this also works with unsized rvalues:
    // `fn deref_move<F: FnOnce(Self::Target) -> O, O>(self, f: F) -> O`
    // however, it's a bit sketchy — see below
    fn deref_move<F, O>(&move self, f: F) -> O
    where
        F: FnOnce(&move Self::Target) -> O;
}

The closure version looks cool, but is kind of sketchy – what if the implementation doesn't call f and just returns Self::Target?

@alercah
Copy link
Contributor Author

alercah commented Jul 20, 2018

Yeah, trying to manage partial is pretty hacky. But it might be doable if we can accept some magic. I have an idea of making deref_move's ABI be magical and special, to work like a hypothetical &move might, using pass-by-pointer instead of pass-by-value. Some of these ideas come from this prior RFC.

At the language level:

  1. deref_move is, like drop, not callable explicitly. Also, like Drop, DerefMove prevents partial moves.
  2. As an exception to the above rule, it is legal to partially move out of self inside of deref_move
  3. You can partially move out of *b where b implements DerefMove. Subsequent accesses to the partially-moved object do not call deref_move again.
  4. self is not dropped at the end of deref_move.
  5. drop has special dropck rules where partial moves done in deref_move are considered to have still happened inside drop.
  6. When a value of a type implementing DerefMove is dropped, first, deref_move is called if it wasn't already. Regardless, its return value, or what remains of it, is dropped. drop is called, then any remaining fields are moved out of.

The idea here is that deref_move is basically "phase 1" of a bigger deref-and-drop operation which works out well. But it runs into recursion issues like those discussed in this thread.

As an unrelated note, I think I realized another problem with the proposal: how does Box<T> return a moved value from heap? I think that without additional compiler support magic it would be required to manually copy the data onto the stack, which is not at all ideal. So we need some sort of &move-like magic somewhere to let you get a T from a value on the heap without copying from it. It might end up being deref_move specific, like you can return unsafe { *p } for p: NonNull<T> or something, and the compiler understands that this

@mikeyhew
Copy link

@alercah

an unrelated note, I think I realized another problem with the proposal: how does Box return a moved value from heap? I think that without additional compiler support magic it would be required to manually copy the data onto the stack, which is not at all ideal. So we need some sort of &move-like magic somewhere to let you get a T from a value on the heap without copying from it. It might end up being deref_move specific, like you can return unsafe { *p } for p: NonNull or something, and the compiler understands that this

Ya, I think what really needs to happen is &'a move T needs to take over from Box<T> and do all the magic. Then Box<T> will still have all the magical capabilities, it'll just be available to other types too, if they implement DerefMove.

@alexreg
Copy link

alexreg commented Aug 7, 2018

What's happening with this RFC lately? 😊

@alercah
Copy link
Contributor Author

alercah commented Aug 7, 2018

I am planning to write up an internals post asking if we want to work on this as it is, or just hold off for &move.

@alexreg
Copy link

alexreg commented Aug 7, 2018

@alercah Couldn't the two features interact with each other in a sane way? Anyway, fair enough.

@alercah
Copy link
Contributor Author

alercah commented Aug 7, 2018

As it is, we basically need a special-case of &move to make this work, as I discussed above. So it may not be worth implementing unless we're happy to implement &move generally.

@alexreg
Copy link

alexreg commented Aug 7, 2018

Okay, sure. Best to discuss it on internals/Discord then, and then come back and edit this RFC accordingly, I guess.

@alexreg
Copy link

alexreg commented Aug 20, 2018

What's the latest on this? I'd definitely be interested in pushing it towards acceptance, even if that means working on the code.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 20, 2018

It means &move. Now that dynamically-sized stack variables have landed on nightly, I think it would be an especially fun time to implement &move.

@alexreg
Copy link

alexreg commented Aug 20, 2018

@Ericson2314 Yeah, sounds good. Does that need a separate RFC, or can people start work on it now? (Indeed, do you have plans to?)

@qnighy
Copy link

qnighy commented Aug 21, 2018

FWIW I published an experimental crate for &move: https://crates.io/crates/refmove

@Ericson2314
Copy link
Contributor

@alexreg I'm not pushing for that right now. (Allocators and stdlib Cargo I think need me in particular more, without even going into Haskell and NixOS things). So happy to pass the baton on this one. Hopefully with NLL here, the type of passes we'd need are a lot closer to the status quo.

@alexreg
Copy link

alexreg commented Aug 21, 2018

@Ericson2314

Fair enough. Maybe @alercah would be more interested in moving forwards &move syntax, with some help?

(And at the risk of going a little off-topic... what's stdlib Cargo?)

@Ericson2314
Copy link
Contributor

Ericson2314 commented Aug 21, 2018

(The overall goal is making standard library crates...not special. Stdlib-aware Cargo (oops forgot the "-aware", sorry) is the Cargo aspect of that, meaning being able to . #1133 was my RFC that essentially died of old age. The plan I think (hope!) is to return to such things after the 2018 edition.)

@alercah
Copy link
Contributor Author

alercah commented Aug 21, 2018

I'm going to be focusing a bit on documentation in the next little while, I think, as the edition comes round. I'll think about picking this up afterward. In the meanwhile, I'm going to withdraw this PR.

@alercah alercah closed this Aug 21, 2018
@alexreg
Copy link

alexreg commented Oct 7, 2018

@alercah Considered reopening this lately? :-)

@LinuxMercedes
Copy link

This would be pretty handy to have for newtypes, as I just ran into a somewhat confusing error message. Example:

enum Action {
  DoThing(String),
  // etc.
}

struct Actions(Vec<Action>);

impl Deref for Actions {
  /* ... */
}

impl DerefMut for Actions {
  /* ... */
}

fn perform(actions: Actions) {
  for action in actions.into_iter() { // <-- error[E0507]: cannot move out of borrowed content
    // do stuff
  }
}

Of course, laid out like this it's perhaps obvious, but if these parts are scattered across a larger project and you're thinking about it as "well Actions is basically just a Vec, I should be able to consume it with into_iter()", it's a bit less obvious what's going on.

@alexreg
Copy link

alexreg commented Feb 27, 2019

@alercah With the 2018 edition release firmly out of the way now, have you considered picking this up again? :-)

@alercah
Copy link
Contributor Author

alercah commented Feb 27, 2019 via email

@alexreg
Copy link

alexreg commented Feb 28, 2019

Okay, does anyone else want to pick this up then? I can help.

@LinuxMercedes
Copy link

I'd be interested, although based on my other responsibilities I think it'll be about a month or so to put together a new RFC based on the comments here.

@alexreg
Copy link

alexreg commented Feb 28, 2019

@LinuxMercedes That's no problem. I have several things keeping me busy too. Feel free to take this and create a new RFC, making a new PR whenever you're ready. I'm available for chat on Discord/Zulip in the meanwhile, if you like.

@runiq
Copy link

runiq commented Jul 29, 2019

@LinuxMercedes Hey, friendly reminder, no pressure: have you had the chance to look at this issue again? :)

@LinuxMercedes
Copy link

hahahahaha no 🙍‍♀️

soon though!

@TotalKrill
Copy link

I am a tad bit sad, that my venture into Newtype to avoid Orphan Rule led me here. It was a rollercoaster ride, where every solution, had a problem, with another solution, but sadly. This seems to be where it ended.

@Progdrasil
Copy link

Same as @TotalKrill here, this would be especially useful for functions consuming NewTypes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet