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

Why does Pin::set take self by value? #57339

Closed
HadrienG2 opened this issue Jan 4, 2019 · 10 comments
Closed

Why does Pin::set take self by value? #57339

HadrienG2 opened this issue Jan 4, 2019 · 10 comments

Comments

@HadrienG2
Copy link

HadrienG2 commented Jan 4, 2019

Looking at https://doc.rust-lang.org/nightly/std/pin/struct.Pin.html, I see that Pin::set takes self by value. This is unlike the API that was proposed in the tracking issue #55766, which would take &mut self instead, and I could not find a rationale for this discrepancy in the tracking issue.

Is this intended? If not, you may want to fix this before the Pin stabilization actually hits beta/stable 😉

@cramertj
Copy link
Member

cramertj commented Jan 4, 2019

This is what I implemented and what I intended when I implemented it, but I didn't notice the discrepancy with @withoutboats's stabilization report. If they feel that this should be changed, it seems worth discussing, otherwise I'm comfortable moving forward with it as-is. The choice here is whether or not to build-in reborrowing into the method or to require a call to .as_mut() to reborrow the Pin.

@HadrienG2
Copy link
Author

HadrienG2 commented Jan 4, 2019

Just to make sure I understand the implications, does the following code behave as I expect?

use pin_utils::pin_mut;

fn main() {
    let x = Foo { ... };
    pin_mut!(x);
    x.set(Foo { ... })
    // Pin<&mut x> does not implement Copy because &mut _ is (obviously) not copyable
    // So x has been consumed and there is no way to access the underlying value anymore...
}

And if so, what is the intended use case for Pin::set()?

@HadrienG2
Copy link
Author

HadrienG2 commented Jan 4, 2019

Oh, wait, I think I understand your earlier remark about as_mut(). Since &mut T implements DerefMut<Target=T>, this should work:

let x = Foo { ... };
pin_mut!(x);
let x2 = x.as_mut();
x2.set(Foo { ... });
// At this point, x2 has been destroyed, but the old x borrow becomes usable again

Coming from a world of pointers and references, it decidedly feels odd to need to manually reborrow like this in order to avoid losing access to the &mut by merely writing into it. But maybe I misunderstood something.

@cramertj
Copy link
Member

cramertj commented Jan 4, 2019

It does feel odd to need to manually reborrow, and its unfortunate that Pin<&mut T> requires this, unlike &mut T which does it automatically. However, I hope this restriction will be lifted in the future.

@HadrienG2
Copy link
Author

I see, if the plan is to enable reborrow semantics later on it does make more sense !

@withoutboats
Copy link
Contributor

Is there a reason you would not want reborrow semantics? The current API is technically the most "broad," but if what it enables is useless (I can't think of a use), we could just backport a change to the API.

@withoutboats
Copy link
Contributor

(The discrepancy with the stabilization report was a mistake on my part I believe; I don't remember noting this and I didn't list it as a change to be made before stabilizing)

@HadrienG2
Copy link
Author

Sorry, I worded my last comment very poorly. What I meant is, if the plan is to eventually give Pin<&mut T> special semantics as a self type, so that self: Pin<&mut T> does not consume the Pin (like &mut self does not consume the reference), then I understand the design of this API better.

@withoutboats
Copy link
Contributor

I was responding to the issue, not your last comment. I think it would be strictly better to make this change while we still can.

@cramertj
Copy link
Member

cramertj commented Jan 7, 2019

@withoutboats I don't have any strong feelings either way. I'll open a PR to change and we can see if anyone opposes or otherwise merge it.

bors added a commit that referenced this issue Jan 9, 2019
Reborrow Pin<P> using &mut in `Pin::set`

Fixes #57339.

This makes it possible to call `.set` multiple times without
using `.as_mut()` first to reborrow the pointer.

r? @withoutboats
cc @rust-lang/libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants