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

Consider merging this crate and pin-utils #58

Closed
Aaron1011 opened this issue Sep 1, 2019 · 11 comments
Closed

Consider merging this crate and pin-utils #58

Aaron1011 opened this issue Sep 1, 2019 · 11 comments
Labels
A-stack-pinning Area: Stack pinning (note: the current pin-project does not provide support for this) C-enhancement Category: A new feature or an improvement for an existing one

Comments

@Aaron1011
Copy link
Collaborator

Aaron1011 commented Sep 1, 2019

The pin-utils crate provides three macros: pin_mut, unsafe_pinned, and unsafe_unpinned.

The #[pin_project] attribute provided by this crate is strictly more powerful than unsafe_pinned and unsafe_unpinned - it ensures safety by default, but lets you opt-out via unsafe to get the unchecked behavior of unsafe_pinned and unsafe_unpinned.

The only thing this crate is missing is the pin_mut macro, which is a small macro_rules! macro.

I think it would be useful for there to be a single crate which handles all of these pinning operations. Currently, users that want both safe pin projections and stack pinning need to use two separate crates.

@taiki-e
Copy link
Owner

taiki-e commented Sep 1, 2019

I think it would be useful for there to be a single crate which handles all of these pinning operations. Currently, users that want both safe pin projections and stack pinning need to use two separate crates.

I agree with you. I submitted rust-lang/futures-rs#1686 for this, but, we can consider redefining in pin-project.

@taiki-e
Copy link
Owner

taiki-e commented Sep 1, 2019

cc @Nemo157 Any thoughts on this?

@Nemo157
Copy link

Nemo157 commented Sep 1, 2019

I actually find pin_mut! quite annoying to use in practice because of the extra binding needed (I fall into the “less having to think of names is better” camp, so commonly the thing that needs pinning is something that I wouldn’t normally bind to a variable). When I last wrote some code that involved a lot of pinning I threw together this crate as an experiment in better ergonomics, but I didn’t get around to actually publishing it.

But, I do agree that it would be good to have a single crate exporting all utilities that you need when working with Pin.

@taiki-e
Copy link
Owner

taiki-e commented Sep 1, 2019

I actually find pin_mut! quite annoying to use in practice because of the extra binding needed (I fall into the “less having to think of names is better” camp, so commonly the thing that needs pinning is something that I wouldn’t normally bind to a variable).

Yeah, I also feel pin_mut! is not ergonomic (nevertheless, I added it to futures 😁.).

When I last wrote some code that involved a lot of pinning I threw together this crate as an experiment in better ergonomics, but I didn’t get around to actually publishing it.

As long as you look at the code in the tests directory, it looks like it's easy to use...

@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label Sep 23, 2019
@taiki-e
Copy link
Owner

taiki-e commented Sep 23, 2019

The ergonomic API for stack pinning has not been established yet, so if adding these support to pin-project at this time, I think it needs to add unergonomic pin_mut to keep stability.

And if we want to experiment with ergonomic things, I think it should be done in another library.

@LucioFranco
Copy link
Sponsor

Can we not add pin_mut at a later time or what ever the "ergo" api will be. Don't think I've needed pin_mut where I have pin-project and not futures-util?

@Nemo157
Copy link

Nemo157 commented Sep 23, 2019

Small update to my comment above, I've now published ergo-pin. My primary reason for writing that was while experimenting with pinning collections and pinned iterators (to allow using generators to create an iterator), so that was a usecase where I could have been using pin-project (although wasn't) and be completely outside the futures ecosystem.

@taiki-e taiki-e added A-meta and removed A-meta labels Sep 25, 2019
@taiki-e
Copy link
Owner

taiki-e commented Sep 25, 2019

Actually, futures_util::pin_mut/futures::pin_mut are sufficient for my use-case and I'm also interested in trying ergo-pin, but I don't have a strong opinion about whether to do this on pin-project itself.

@taiki-e taiki-e added feedback-wanted A-stack-pinning Area: Stack pinning (note: the current pin-project does not provide support for this) labels Sep 25, 2019
@taiki-e
Copy link
Owner

taiki-e commented May 18, 2020

I'm still torn about this. Here are some updates and my opinions (TL;DR: I feel there are one advantage and some relatively minor drawbacks):

  • I'm now a maintainer of pin-utils.
  • pin-utils will only provide stack-pinning util in the future (Deprecate unsafe pin projection macros rust-lang/pin-utils#29)
  • There are two pin-project crates, and the decision here is also propagated to lite.
  • Adding a futures-util dependency for use only a stack pinning utility is not good. (However, as pin-project and dependencies require relatively long compile times, "add pin-project dependency for use only a stack pinning utility" may not happen very often.)
  • I think after removed the project attributes (Deprecate #[project] attributes in favor of the naming of projected-type #225) should allow the API to be considered stable enough (i.e., v1.0). Not so when adding stack pinning API.
  • pin-utils is a really small crate that won't impact compile time with or without merging. (Depending on the situation, but using pin-project's stack pinning utility instead of pin-utils should increase overall compile time because pin-project is only compiled after proc-macro is compiled. But I feel some users are more sensitive to the increasing number of dependencies than the actual compile time.)
  • It's convenient to have all pin related utilities in one place.
  • If we don't add this, the name of pin-project is descriptive about what this crate does.

@Aaron1011
Copy link
Collaborator Author

Aaron1011 commented Jun 15, 2020

I've become relatively indifferent about adding pin_mut to this crate. However, I think it would be a good idea to reach a decision one way or the other, as this is the only remaining open issue 😄

@taiki-e
Copy link
Owner

taiki-e commented Jul 8, 2020

I'm going closing this because the benefits of doing this don't seem to outweigh the drawbacks I said in the comment above, at this time.

If anyone can explain more about the benefits of doing this, we can reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stack-pinning Area: Stack pinning (note: the current pin-project does not provide support for this) C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

No branches or pull requests

4 participants