Skip to content

stabilize pin #403

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

Closed
wants to merge 1 commit into from
Closed

stabilize pin #403

wants to merge 1 commit into from

Conversation

yoshuawuyts
Copy link
Contributor

@yoshuawuyts yoshuawuyts commented Oct 28, 2019

Removes the "unstable" marker from async_std::pin. Even in the absence of #258, there's great value in being able to import everything required for implementing Future from async_std. This may not seem like a big deal at first glance, but it removes a cognitive speed bump when implementing futures, which makes async-std more intuitive to use.

Thanks!

Proposed

use async_std::future::Future;
use async_std::task::{Context, Poll};
use async_std::pin::Pin;

impl Future for MyStruct {
    type Output = ();
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        Poll::Ready(())
    }
}

Current

use std::pin::Pin;

use async_std::future::Future;
use async_std::task::{Context, Poll};

impl Future for MyStruct {
    type Output = ();
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        Poll::Ready(())
    }
}

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Oct 28, 2019
@ghost
Copy link

ghost commented Oct 28, 2019

it removes a cognitive speed bump when implementing futures, which makes async-std more intuitive to use.

My personal experience is the opposite - I never found async_std::pin useful and always instinctively reach for std::pin instead. But that's just my very biased experience.

From a pedagogical standpoint, I worry that having async_std::pin might create confusion to some extent. Rust users will hear about std::pin::Pin and learn about it from the standard library, the async book, other async projects, and so on, and might be confused as to why we have the same module and wonder how it differs from the one in std.

While I don't have a particularly strong opinion and would rather do nothing than stabilize this, I'd like more convincing motivation first.

To be concrete, something like the following needs to happen for me to 👍 this PR:

  • Someone complains about having to import std::pin and wondering why there isn't async_std::pin. At least 1 user who is not us needs to complain.

  • We open an issue gathering user feedback and then stabilize the module if people want it and it fits their mental model well.

Any opinions? It would very helpful to hear more than 2 opinions on this.

@skade
Copy link
Collaborator

skade commented Oct 28, 2019

I would prefer to not expose API that might couple people to async-std without need. This might lead to a situation where people import async_std::pin and nothing else, gluing them to async-std without need.

@yoshuawuyts
Copy link
Contributor Author

Pedagogy

From a pedagogical standpoint, I worry that having async_std::pin might create confusion to some extent.

I think this touches on a more general issue of re-exports. We may need to be more clear on what we're re-exporting, and what we're defining ourselves. Similarly we may need to clarify what's new in async-std, and what's merely an adaptation of std APIs.

I'm not worried that we're introducing too much API surface if it's a direct re-export of std. In fact, I think we should err on re-exporting rather than not in case of doubt. Within reason of course.


Acceptance criteria

To be concrete, something like the following needs to happen for me to +1 this PR [user opens issue, or we put out a poll]

We haven't been really designing APIs this way so far, and I don't think we should start now. User testing in general is useful, but this seems like something so minor that the effort is probably not worth it.

For example: nobody was asking for future::select, but we found that having it was part of a complete story for futures concurrency. Similar for a re-export such as net::IpV4Addr. In this case Pin is a component required for implementing Future, and similar to some of our other re-exports I think it'd make sense for us to export it.


Design direction

Reflecting a bit further on where I'm coming from: I'm approaching this as: "If it's part of std, why wouldn't we add this?". I feel others might be approaching this more as: "Why would we add this?".

I think being liberal with our API additions has worked out well for us so far. Very few of the APIs in Stream have been well-motivated beyond "std has them". But working towards parity with std has provided a cohesive experience, and also sets us up well to meet the expectations of people who're new to async-std.

This different from APIs that we're designing, and adding to async-std. Each new addition has to be thoroughly motivated, and sometimes take weeks or months to design. I think the balance beyond "liberal std parity, conservative novel APIs" is the right path, and adding pin::Pin is very much along those lines.

@ghost
Copy link

ghost commented Oct 29, 2019

I'm not worried that we're introducing too much API surface if it's a direct re-export of std. In fact, I think we should err on re-exporting rather than not in case of doubt. Within reason of course.

So the reason why I'm worried about the async_std::pin re-export and not about re-exports like async_std::net::IpV4Addr is the following.

IpV4Addr is a helper type and the focus in the net module is on the "central" types like TcpStream, TcpListener, and UdpStream. Everything else in the module (like IpV4Addr) is noise. So async_std::net is asyncified std::net + its helper types.

I feel hesitant about async_std::pin because it's a whole new module and provides nothing in addition to std::pin. It's not asyncified std::pin. However, if it was just a helper type within std::task, I'd have no problem with re-exporting it into async_std::task.

There is another problem here, which is that Pin is not exactly as simple as IpV4Addr. It also comes with the trait std::marker::Unpin and the marker type std::marker::PhantomPinned, making it span two modules. I feel having just async_std::pin without the marker module makes it look incomplete.

I'm just saying that Pin is more tricky than other small types and am worried stabilizing it like this might be a mistake. I might realize I'm wrong on that later, but am worried enough to not be comfortable with approving the PR right now. What would reassure me is merely more than 1 voice saying this is a good idea (preferably a user rather than a part of the team), which I don't think is much to ask for. It can be on Discord. :)

We haven't been really designing APIs this way so far, and I don't think we should start now. User testing in general is useful, but this seems like something so minor that the effort is probably not worth it.

My approach has been to accept almost anything behind unstable and then stabilize if it proves useful. This re-export didn't get much use and I don't see it as minor because it's a top-level module and is incomplete without async_std::marker right now.

I think being liberal with our API additions has worked out well for us so far. Very few of the APIs in Stream have been well-motivated beyond "std has them". But working towards parity with std has provided a cohesive experience, and also sets us up well to meet the expectations of people who're new to async-std.

The key difference for me is that Stream is asyncified Iterator so if Iterator has a method, then Stream should have it too, which is why we don't have to feel hesitant.

Pin is different. It is somewhat similar to types Waker, Context, and Poll, but given that it also comes with Unpin and PhantomPinned also makes it more complicated.

@yoshuawuyts
Copy link
Contributor Author

t also comes with the trait std::marker::Unpin and the marker type std::marker::PhantomPinned, making it span two modules. I feel having just async_std::pin without the marker module makes it look incomplete.

I think this is a pretty compelling argument to not do this quite yet. Agreed, let's hold off for now.

@yoshuawuyts yoshuawuyts deleted the stabilize-pin branch October 29, 2019 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants