Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants