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

Change the argument type of project method back to self: Pin<&mut Self> #89

Closed
taiki-e opened this issue Sep 11, 2019 · 12 comments · Fixed by #90
Closed

Change the argument type of project method back to self: Pin<&mut Self> #89

taiki-e opened this issue Sep 11, 2019 · 12 comments · Fixed by #90
Assignees
Labels
A-pin-projection Area: #[pin_project] breaking-change This proposes a breaking change
Milestone

Comments

@taiki-e
Copy link
Owner

taiki-e commented Sep 11, 2019

The signature of the generated project method changes as follows.

before:

fn project(self: &mut Pin<&mut Self>) -> ProjectedType;

after:

fn project(self: Pin<&mut Self>) -> ProjectedType;

The advantage of the current approach is that you can call the project method multiple times without .as_mut().

However, there are drawbacks such as requiring a different method to avoid lifetime issues (#65), and not being able to call the method without importing trait (#21 (comment)).

Also, by reverting this, the code generated by the macro will be simpler.

Related: #47 (comment)

PR: #90

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 11, 2019

I thought the effects of doing this.

  1. The generated code is reduced by about 10-15 + number of fields/variants lines per #[pin_project].

  2. As it does not depend on stabilization of self: &mut Pin<&mut Self> and resolves Provide way to import projection trait #80, blockers of 0.4 release are gone. (Tracking issue for pin-project 0.4 #21 (comment))

  3. This should not significantly increase the code that we need to write. (half of the change is the deletion of mut)

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 11, 2019

cc @Aaron1011 @Nemo157 @jonhoo @seanmonstar @LucioFranco Any thoughts on this?

@LucioFranco
Copy link

So having just released like 10 of the tower crates that depend on alpha.11, not too excited about this haha.

On a real note, would it make sense to wait to see if &mut Pin<..> will make it into 1.39 then make a decision from there?

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 15, 2019

@LucioFranco

On a real note, would it make sense to wait to see if &mut Pin<..> will make it into 1.39 then make a decision from there?

This is certainly what I thought was the most realistic a week ago. However, most of the reason why the current pin-project is unstable is about self: &mut Pin<..> related issues (most other issues are either backward compatible or can be implemented while keeping backward compatibility), and even if self: &mut Pin<..> is stable, the current complexity cannot be completely removed. So I'm not sure if it really makes sense to wait. (I am thinking of merging this and releasing beta. I believe we can stabilize the crate after making some non-breaking changes/adjustments.)

That being said, this is a fairly breaking change for existing alpha users, so I'd like to help with the update as much as possible (Specifically, I will write a few patches, but is there anything else I can do?).

@Nemo157
Copy link

Nemo157 commented Sep 16, 2019

In my current usage of pin-project (which I haven't tried updating to 0.4 yet) I also have a lot of my own self: Pin<&mut Self> methods. There's no way I'm going to do the same trait-indirect workaround to change these to self: &mut Pin<&mut Self>, so my methods are already pretty full of self.as_mut().foo(), so having to do it for project() as well isn't a big deal.

I do need the ability to return values bound to the lifetime of the input pin, switching that to be via project_into() wouldn't be a massive deal since they're all just get_pin_mut functions on wrappers, but that vs as_mut() feel like similarly sized downsides to me.

@RalfJung
Copy link
Contributor

How widely known do you feel it is that people have to call as_mut? Maybe it would make sense to add a section on that to the libstd docs, so that people have a better chance of learning about this? I can help review that PR.

@LucioFranco
Copy link

So I'm not sure if it really makes sense to wait.

Totally makes sense, I'm 👍 on board with this change then.

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 16, 2019

How widely known do you feel it is that people have to call as_mut?

I'm not sure about Pin::as_mut, but in the first place, I feel that many of the convenience features related to Pin API are not well known. (Pin::set (tower-rs/tower#323 (comment)), Option::as_pin_mut (tower-rs/tower#316 (comment)), pin_utils::pin_mut (hyperium/hyper#1923), etc...)

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 16, 2019

Maybe it would make sense to add a section on that to the libstd docs, so that people have a better chance of learning about this?

I'm not sure about Pin::as_mut, but in the first place, I feel that many of the convenience features related to Pin API are not well known.

Perhaps it is enough to add a description and an example to the method documentation. There is currently no example and there is no mention of the need to use as_mut in these situations.

@RalfJung
Copy link
Contributor

That might be a good start, yes.

@taiki-e
Copy link
Owner Author

taiki-e commented Sep 17, 2019

@RalfJung: I've filed rust-lang/rust#64529

tmandry added a commit to tmandry/rust that referenced this issue Sep 17, 2019
tmandry added a commit to tmandry/rust that referenced this issue Sep 17, 2019
tmandry added a commit to tmandry/rust that referenced this issue Sep 17, 2019
bors bot added a commit that referenced this issue Sep 18, 2019
90: Change the argument type of project method back to `self: Pin<&mut Self>` r=taiki-e a=taiki-e

Closes #89


Co-authored-by: Taiki Endo <[email protected]>
@taiki-e
Copy link
Owner Author

taiki-e commented Sep 18, 2019

I'm going to merge this and some of the existing issues and release beta within a few days.

@bors bors bot closed this as completed in ddc0122 Sep 18, 2019
@bors bors bot closed this as completed in #90 Sep 18, 2019
@taiki-e taiki-e added A-pin-projection Area: #[pin_project] breaking-change This proposes a breaking change labels Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pin-projection Area: #[pin_project] breaking-change This proposes a breaking change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants