Skip to content

expose std::pin #203

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

Merged
merged 5 commits into from
Sep 17, 2019
Merged

expose std::pin #203

merged 5 commits into from
Sep 17, 2019

Conversation

yoshuawuyts
Copy link
Contributor

This is important when defining / calling futures, so it makes sense for us to also export this.

But also given recent user feedback on the confusion on pinning, I'd like to open up the possibility to experiment with providing better pinning facilities such as pin-project or pin_mut behind flags. I'm not sure if we could, or even should. But I want to allow us to have that conversation and test things out (even if it's just in floating patches.)

Thanks!

@ghost
Copy link

ghost commented Sep 16, 2019

The module is not behind the unstable flag :)

@yoshuawuyts
Copy link
Contributor Author

The module is not behind the unstable flag :)

Oh yeah accurate; I was thinking pin::Pin is fine exactly the way it is because it's part of std and we already use it. Any addition to this module would be behind an unstable flag.

@ghost
Copy link

ghost commented Sep 16, 2019

I'm not that sure we should expose std::pin yet and would still prefer having the whole module behind unstable.

This is important when defining / calling futures, so it makes sense for us to also export this.

Defining and calling futures is something users should rarely do (ideally, never!) - after all, the promise of async/await is that we don't write futures by hand anymore. And even if we still do write them by hand from time to time today, I expect that to be less and less the case over time.

Another point is that pinning is not that much related to async programming, and is an orthogonal feature that has more to do with generators. But I admit that, practically speaking, pinning is very closely tied to futures right now.

There are also std::marker::Unpin and std::marker::PhantomPinned, without which the "pinning story" would be incomplete. Imagine someone having the following imports in their code:

use std::marker::PhantomPinned;
use async_std::pin::Pin;

Why is Pin here in async_std but PhantomPinned is not? I worry that might be confusing.

@yoshuawuyts
Copy link
Contributor Author

@stjepang those are all valid points, and I agree. I'll put the whole module behind an unstable flag!

Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
Signed-off-by: Yoshua Wuyts <[email protected]>
@yoshuawuyts
Copy link
Contributor Author

all feedback implemented. bors r+

@taiki-e
Copy link
Contributor

taiki-e commented Sep 17, 2019

@yoshuawuyts: It seems bors could not recognize your comment as a command.

https://bors.tech/documentation/

Also, the command will be recognized if, and only if, the word “bors” is at the beginning of a line.

@yoshuawuyts
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Sep 17, 2019
203: expose std::pin r=yoshuawuyts a=yoshuawuyts

This is important when defining / calling futures, so it makes sense for us to also export this.

But also given recent user feedback on the confusion on pinning, I'd like to open up the possibility to experiment with providing better pinning facilities  such as [`pin-project`](https://github.com/taiki-e/pin-project) or [`pin_mut`](https://docs.rs/pin-utils/0.1.0-alpha.4/pin_utils/macro.pin_mut.html) behind flags. I'm not sure if we could, or even should. But I want to allow us to have that conversation and test things out (even if it's just in floating patches.)

Thanks!

Co-authored-by: Yoshua Wuyts <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 17, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 39a1c2b into master Sep 17, 2019
@ghost ghost deleted the expose-pin branch September 17, 2019 09:29
@ghost
Copy link

ghost commented Sep 17, 2019

Would it make sense to experiment with pin_mut! or #[pin_project] in async-macros/async-attributes? That feels to like a more appropriate place to me than async-std since those macros are not really "async version of Pin".

I'd love to collect all kinds of nice stuff there like #[test], #[bench], #[main], #[pin_project], #[throws], #[async_trait], #[for_await], etc. And we would be able to iterate quickly without worrying about stability - breaking changes are perfectly fine because these attributes are just syntax sugar.

But anyways, just thinking out aloud - let's keep async_std::pin as it is :)

@yoshuawuyts yoshuawuyts mentioned this pull request Sep 17, 2019
6 tasks
@yoshuawuyts
Copy link
Contributor Author

@stjepang Hell yes!

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

Successfully merging this pull request may close these issues.

2 participants