rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.93k stars 12.53k forks source link

[Stabilization] Future APIs #59725

Closed withoutboats closed 5 years ago

withoutboats commented 5 years ago

Feature name: futures_api Stabilization target: 1.36.0 Tracking issue: #59113 Related RFCs:

I propose that we stabilize the futures_api feature, making the Future trait available on stable Rust. This is an important step in stabilizing the async/await feature and providing a stable, ergonomic, zero-cost abstraction for async IO in Rust.

The futures API was first introduced as a part of the std library prior to 1.0. It was removed from std shortly after 1.0, and was developed outside of std in an external crate called futures, first released in 2016. Since that time, the API has undergone significant evolution.

All the APIs being stabilized are exposed through both core and std.

The future Module

We shall stabilize these items in the future module:

We do not stabilize the other items in this module, which are implementation details of async/await as it currently exists that are not intended to be stabilized.

The task Module

We shall stabilize these items in the task module:

Notice: Late Changes to the API

We have decided to merge the future-proofing changes proposed in #59119 to leave room for some potential extensions to the API after stabilization. See further discussion on that issue.

Notes on Futures

The poll-based model

Unlike other languages, the Future API in Rust uses a poll based execution model. This follows a back and forth cycle involving an executor (which is responsible for executing futures) and a reactor (which is responsible for managing IO events):

Eventually, the future returns ready instead of pending, indicating that the future has completed.

Pinning

The Future trait takes self by Pin<&mut Self>. This is based on the pinning APIs stabilized in 1.33. This contract allows implementers to assume that once a future is being polled, it will not be moved again. The primary benefit of this is that async items can have borrows across await points, desugared into self-referential fields of the anonymous future type.

Changes proposed in this stabilization report

std has unsafe constructors following both names, from_raw is more specific than new_unchecked (its a constructor taking the "raw" type which is possibly invalid, asserting that this instance is valid).

The most common waker implementation is to be an arc of the task which re-enqueues itself when the waker is woken; to implement this by reference, you must clone the arc and put it on the queue. But most uses of wake drop the waker as soon as they call wake. This results in an unnecessary atomic reference increment and decrement; instead we now provide by a by-value and by-reference implementation, so users can use the form most optimal for their situation.

Moderation note

The futures APIs have been discussed at enormous length over the past 3 years. Every aspect of the API has been debated, reviewed and considered by the relevant teams and the Rust community as a whole. When posting to this thread, please make a good faith effort to review the history and see if your concern or proposal has been posted before, and how and why it was resolved.

withoutboats commented 5 years ago

@rfcbot fcp merge

rfcbot commented 5 years ago

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ghost commented 5 years ago

Just a suggestion for a small change to the waker API that seems to be unaddressed: add a "wake and drop Waker" method, i.e. wake() variant that consumes self. Or change the wake() method so that it consumes self.

This is the most common pattern in practice - we typically don't want to reuse the waker after waking the task. Currently, in most executors, wake() will increment the refcount of the task and drop(waker) will decrement the refcount, which means every time we wake a task, we're wasting two atomic instructions.

I'm also okay with simply changing wake(&self) to wake(self). In that case, waker.clone().wake() will do the same thing waker.wake() currently does (typically also with the same performance characteristics).

See also this comment by @cramertj: https://github.com/aturon/rfcs/pull/16#issuecomment-474584607

But if anyone thinks this is a too controversial/non-trivial/bikesheddable change, don't worry about it since we can easily add it anytime later.

withoutboats commented 5 years ago

It can be done backward compatibly but the thing is it would require adding another constructor to RawWaker that takes the extra fn pointer, which doesn't seem great. I'd be in favor of solving this now, since I don't think its very controversial.

There are use cases where it would be more expensive to go by value, so we shouldn't just change the signature of wake without providing an alternative. If we consider this optimization worth supporting, I would propose this change:

jethrogb commented 5 years ago

The future Module

We shall stabilize these items in the future module:

  • The std::future module itself
  • The std::future::Future trait and both of its associated types (Output and poll)

Do you mean “associated items”?

The task Module

We shall stabilize these items in the future module:

Do you mean “task module”?

cramertj commented 5 years ago

+1-- thanks for bringing that up, @stjepang, I was also about to mention that change. I agree with @withoutboats's proposed solution.

Centril commented 5 years ago

Another thing to note is that we need a #[rustc_const_allow_fn_pointer_only_for_raw_wakers] mechanism specifically for some of the APIs that are being stabilized to make it all work. See discussion in https://github.com/rust-lang/rust/pull/59119#discussion_r264841751 for an elaboration.

Basically we need a way on stable Rust to pass values of fn pointer types into const fns. The aforementioned attribute hack will be used to allow this for RawWakerVTable::new. However, this does not stabilize the ability to define such functions on stable Rust. This is not necessary for the purposes of the futures API.

This has not been implemented yet but will be done so adjacent to merging the stabilization PR (hopefully before, and ideally it should not need beta backporting since we have some days to get it merged before master=>beta promotion).

@oli-obk @cramertj Can one of you do the impl / review? (also cc me on that PR)


The change to add wake_by_ref seems great to me.


@rfcbot reviewed

cramertj commented 5 years ago

Another thing probably worth calling out is the list of Try impls for Poll. They're all designed to bubble up errors as easily as possible inside Future and Stream implementations, but they do not propagate Pending-- that is done with the futures::ready! macro. It's useful to have these as two separate steps, since it's relatively common in manual future implementations to want one but not the other.

There's an impl for Poll<Result<T, E>> and an impl for Poll<Option<Result<T, E>>> (that propagates errors but not Pending or None).

These can be found here.

eddyb commented 5 years ago

It just crossed my mind that the RawWaker (and the associated vtable) can be made safe to construct: https://github.com/rust-lang/rust/pull/59739#discussion_r272793941

Does anyone know if that had been discussed before? Or even tried and found "subtly impossible (atm)"? Because I'd rather avoid adding a stable API involving *const ().

Centril commented 5 years ago

So... based on @eddyb's idea and discussions with them as well as with @Nemo157, I'd like to propose a last-last minute change to the API, namely:

extern {
    type Private;
}

pub struct RawWaker {
    data: NonNull<Private>,
    vtable: &'static RawWakerVTable<Private>,
}

pub struct RawWakerVTable<T: ?Sized = ()> {
    clone: for<'a> fn(&'a T) -> RawWaker,
    wake: unsafe fn(NonNull<T>),
    wake_by_ref: for<'a> fn(&'a T),
    drop: unsafe fn(NonNull<T>),
}

impl RawWaker {
    pub fn new<T>(data: NonNull<T>, vtable: &'static RawWakerVTable<T>) -> Self {
        ...
    }
}

impl<T> RawWakerVTable<T> {
    #[rustc_promotable]
    #[rustc_allow_const_fn_ptr]
    pub const fn new(
        clone: for<'a> fn(&'a T) -> RawWaker,
        wake: unsafe fn(NonNull<T>),
        wake_by_ref: for<'a> fn(&'a T),
        drop: unsafe fn(NonNull<T>),
    ) -> Self {
        ...
    }
}

No other public facing changes are made.

An implementation exists in a playground (aside from #[rustc_allow_const_fn_ptr] which is implemented in https://github.com/rust-lang/rust/pull/59739/commits/9b348288605b56e9a4a2fcfd9dcf8302d1956109).

The rationale for doing so is as @eddyb put it:

[...] it's a neat middle-ground between full-on traits and type-unsafe *const ()

In particular, we move from NonNull and to T instead of (). Moreover, two function pointers stop being unsafe.

eddyb commented 5 years ago

Note that if you just change *const () to *const T, users don't have to change because they can just rely on T being inferred to (), so that's one compromise you could make.

carllerche commented 5 years ago

FWIW, w/ raw pointers, it is possible to store additional data in used pointer bits, rendering the pointer invalid. This would be fine if the invalid pointer is passed to a vtable fn as a raw pointer, but passing it as a ref would forbid this use case.

Centril commented 5 years ago

@carllerche So then to support that use case you'd want?:

pub struct RawWakerVTable<T: ?Sized = ()> {
    clone: unsafe fn(NonNull<T>) -> RawWaker,
    wake: unsafe fn(NonNull<T>),
    wake_by_ref: unsafe fn(NonNull<T>),
    drop: unsafe fn(NonNull<T>),
}

(NonNull<T> should work since it is a #[repr(transparent)] newtype to a *const T)

The change to T still buys us some type safety. The for<'a> fn(&'a T) variants can probably be added later as an additional associated function on RawWakerVTable.

carllerche commented 5 years ago

@centril I don't have an opinion on the details as long as the use cases are supported (non valid pointers and forwards compatibility).

Centril commented 5 years ago

@carllerche Fair :) Thanks for the input!

Matthias247 commented 5 years ago

Why NonNull? For something like a global executor one doesn't need the pointer data, and it could simply stay unused/null. Of course it could also be set to a dummy value, but what would be the rationale for this?

Centril commented 5 years ago

@Matthias247 Well the thinking was that you wouldn't want to pass in null to this so it would be more type-safe to use NonNull. I'm happy to use *const T instead.

Really, the bigger point was to move from () to T and let RawWakerVTable be generic. I think that is more type-safe, ergonomic (since you obviate the need for one cast), and since it is generic it also offers better code reuse.

withoutboats commented 5 years ago

@eddyb @Centril I am not in favor of pursuing this change to the futures API.

First, the only additional check you're encoding in the type system is that when you construct a RawWaker the data pointer you pass has the same type as the arguments to your vtable function. This will not actually prevent you from having to do unsafe casts, since you want is not actually a *const but actually usually an Arc (or in niche use cases not an arc, but also not a raw pointer). This type is also likely to be arbitrary, for that reason, rather than a real meaningful type - I would probably just keep using (). This would also complicate the APIs for use cases which swap out the wake method but use an underlying waker. I really don't see that this would provide much benefit in preventing bugs when using this API; it wold essentially just be a hoop to jump without checking anything meaningful.

Also, this is a niche use case (creating a waker) and even in that niche use case there is a much more ergonomic and type safe API we eventually want to add for the majority of those use cases, which would look roughly like this:

trait Wake {
   fn wake(self: Arc<Self>);

   // default impl overrideable if you dont need ownership
   fn wake_by_ref(self: &Arc<Self>) {
      self.clone().wake()
   }
}

impl Waker {
    pub fn new(wake: Arc<dyn Wake>) -> Waker { ... }
}

This is blocked on a few other things (&Arc as a valid receiver, changes to the std/core infrastructure so we can add std-only methods to core types), so we decided to stick with the RawWaker API for the moment. But in the long term, RawWaker will be a niche within a niche: constructing wakers that don't conform to the simple majoritarian implementation of just being an arc'd trait object. These are things like embedded executors and the intermediate executors in concurrency primitives like futures-unordered.

So this seems like it would complicate the API without actually benefiting the targeted users, who are also a small minority of users. The () is not the challenging unsafe bit of the waker API.

Centril commented 5 years ago

This will not actually prevent you from having to do unsafe casts, since you want is not actually a *const but actually usually an Arc (or in niche use cases not an arc, but also not a raw pointer).

It won't, this is true. However, if we take e.g. @Nemo157's embrio-rs then we can move from:

// Leaving out `wake_by_ref` here... it would ofc be included in a real implementation.

static EMBRIO_WAKER_RAW_WAKER_VTABLE: RawWakerVTable = RawWakerVTable {
    clone: |data| unsafe { (*(data as *const EmbrioWaker)).raw_waker() },
    wake: |data| unsafe { (*(data as *const EmbrioWaker)).wake() } },
    drop,
};

    pub(crate) fn raw_waker(&'static self) -> RawWaker {
        RawWaker::new(
            self as *const _ as *const (),
            &EMBRIO_WAKER_RAW_WAKER_VTABLE,
        )
    }

into:

static EMBRIO_WAKER_RAW_WAKER_VTABLE: RawWakerVTable<EmbrioWaker> = RawWakerVTable {
    clone: |data| unsafe { (*data).raw_waker() },
    wake: |data| unsafe { (*data).wake() } },
    drop,
};

    pub(crate) fn raw_waker(&'static self) -> RawWaker {
        RawWaker::new(
            self as *const _,
            &EMBRIO_WAKER_RAW_WAKER_VTABLE,
        )
    }

This seems to me strictly better, ergonomic, and readable since it has fewer casts.

This type is also likely to be arbitrary, for that reason, rather than a real meaningful type - I would probably just keep using ().

The use of EmbrioWaker above does not seem arbitrary to me. It seems meaningful. However, if you want to keep using (), you can of course do that.

This would also complicate the APIs for use cases which swap out the wake method but use an underlying waker. I really don't see that this would provide much benefit in preventing bugs when using this API; it wold essentially just be a hoop to jump without checking anything meaningful.

As the generic parameter T is defaulted to () there is zero extra complication and drawbacks to those that don't need more than (). It is not true that you would need to jump through hoops. Rather, this is a net reduction in hoops that need to be jumped through for all users.

(To be clear, the version I'm talking about here is:

pub struct RawWakerVTable<T: ?Sized = ()> {
    clone: unsafe fn(*const T) -> RawWaker,
    wake: unsafe fn(*const T),
    wake_by_ref: unsafe fn(*const T),
    drop: unsafe fn(*const T),
}

)

But in the long term, RawWaker will be a niche within a niche: constructing wakers that don't conform to the simple majoritarian implementation of just being an arc'd trait object.

I understand this point but does that matter? Even within the niche of the niche it provides an improvement without any cost to those that only ever want (). Why should we intentionally pessimize an API because it is niche?

Nemo157 commented 5 years ago

This will not actually prevent you from having to do unsafe casts, since you want is not actually a *const but actually usually an Arc (or in niche use cases not an arc, but also not a raw pointer).

In most cases isn't the pointer being passed around actually the Arc pointer? @Centril's latest version would allow removing casts completely and just using Arc::into_raw/Arc::from_raw for the internal conversion (e.g. here).

Centril commented 5 years ago

@rfcbot concern RawWakerVTable-if-generic-would-be-better-even-if-only-for-niche-uses

cramertj commented 5 years ago

If the pointer is being passed around that way (in an Arc), it can use the even safer ArcWake trait. The point of the RawWakerVTable was to provide more fine-grained access in edge-case scenarios, and I don't think making it generic buys us anything in those cases (and, in fact, can make things harder by requiring functions to be generic that otherwise would not be).

The common case will already be addressed primarily through entirely safe traits. Adding extra guardrails to the unsafe escape hatch isn't necessary here.

nikomatsakis commented 5 years ago

I am ambivalent here. I see the benefits of making RawWaker incrementally more type-safe and (at least in some cases) ergonomic, but I also feel like the overriding concern here is getting futures out the door, so that the rest of the ecosystem can start to build on them.

EDIT: To be clear, for this reason, I'd probably be inclined to move forward with the API as it is, so as to avoid further delay.

Centril commented 5 years ago

and I don't think making it generic buys us anything in those cases

I cannot square this with the obvious improvement I saw to EMBRIO_WAKER_RAW_WAKER_VTABLE.

(and, in fact, can make things harder by requiring functions to be generic that otherwise would not be).

How? The generic parameter is defaulted to T = () so if you just write RawWakerVTable without applying any type arguments you get back to where we are right now.

withoutboats commented 5 years ago

I think that the problem is not that this API proposal is necessarily net worse, but that its most optimistic net benefit is not better enough to justify delaying shipping to consider its impact fully and reach consensus on this change. I think its very important to keep in mind that shipping these features to users is in itself a priority for the project & that changes with only small possible upsides should not block shipping.

EDIT: essentially we reach a point of diminishing returns in which we are putting too many project resources into considering decisions with too little impact

oli-obk commented 5 years ago

How? The generic parameter is defaulted to T = () so if you just write RawWakerVTable without applying any type arguments you get back to where we are right now.

Isn't not having generic parameters forward compatible to adding defaulted generic parameters? Thus we can in fact delay this discussion to after stabilization.

Centril commented 5 years ago

Isn't not having generic parameters forward compatible to adding defaulted generic parameters? Thus we can in fact delay this discussion to after stabilization.

It's not. At minimum, the act of adding a generic parameter impacts turbofish which makes it a breaking change to add new type parameters to RawWakerVTable::new

I think that the problem is not that this API proposal is necessarily net worse, but that its most optimistic net benefit is not better enough to justify delaying shipping to consider its impact fully and reach consensus on this change. I think its very important to keep in mind that shipping these features to users is in itself a priority for the project & that changes with only small possible upsides should not block shipping.

If you don't think there's a net worsening of the API but that the net benefit is not large enough (but I do), then I see little reason not to do it. I think disagreeing to this change only to speed up stabilization will in fact be counterproductive to the goal of shipping faster.

Moreover, the proposal was just changed to include wake_by_ref so "consider its impact fully" applies to that too.

nikomatsakis commented 5 years ago

@Centril

I think disagreeing to this change only to speed up stabilization will in fact be counterproductive to the goal of shipping faster.

Is the premise here that it is counterproductive because you are blocking stabilization? I don't think that's particularly fair. In general, leaving the API as is is obviously less work -- no PRs have to land or commits have to be pushed. Moreover, the API has been "vetted" as is, and last minute changes always run some risk of surprising interactions we did not foresee (even if the risk is minimal here, due to the fact that the old API can be emulated).

I think @withoutboats's meta point is quite valid. At this stage in the stabilization process, I think we should be quite... judicious in our use of blocking concerns, particularly for those of us who have not been closely involved in the whole futures discussion until this point. I have not been following that closely but I know that the whole setup around Waker and RawWaker underwent a lot of work to keep it simple -- I feel like the best time for this suggestion would have been in that period, and it feels reasonable to me to say "we're not inclined to change it now unless the improvement is of high importance" (based on prior comments, I think everyone agrees that this ultimately a niche concern).

oli-obk commented 5 years ago

At minimum, the act of adding a generic parameter impacts turbofish which makes it a breaking change to add new type parameters to RawWakerVTable::new

I don't understand what situation would cause turbofish to fail to work if you start out with zero generic parameters and add a new one. (Some experiments in https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9ff8beb5c56f54fd57da801897b70728)

Can you give an example that fails?

nikomatsakis commented 5 years ago

I don't think adding a type parameter is 100% back compatible. Consider this example (playground):

struct Foo<T = ()> {
    x: T
}

fn main() {
    let x = Foo { x: Default::default() };
}

This will fail to compile, but it would compile if we convert T to ().

jethrogb commented 5 years ago

@nikomatsakis In that example you're not using the type's public API.

nikomatsakis commented 5 years ago

Perhaps, I'm not sure what the 'public API' for raw-waker-vtable is here. I don't care to debate the technical details, as I think the high order bit here is the procedural one -- that we should be trying to avoid endless iteration and moving towards shipping. The fact that the people most closely involved here (@withoutboats, @cramertj, @stjepang, etc) seem inclined to leave things as they are seems good enough for me.

withoutboats commented 5 years ago

Merging #59733, added to the stabilization report that we're adding the wake_by_ref method I mentioned earlier based on @stjepang's suggested optimization.

pietroalbini commented 5 years ago

We briefly discussed the stabilization at the release team meeting, and everyone agreed we'd prefer not to backport the stabilization to 1.35.0, hoping it will land in time to be included in 1.36.0.

nikomatsakis commented 5 years ago

So I haven't seen a lot of progress on this thread. At this point, it seems like the best thing for us to do is to discuss this in the @rust-lang/lang meeting this Thursday. You can find details about the meeting here, including a Zoom link.

cramertj commented 5 years ago

@nikomatsakis sorry, can you clarify what progress you expected to see here? There's already a stabilization PR out for review. Are you talking about discussing @Centril's concern?

nikomatsakis commented 5 years ago

I was referring to Centril's concern, yes.

withoutboats commented 5 years ago

If you don't think there's a net worsening of the API but that the net benefit is not large enough (but I do), then I see little reason not to do it.

I'm not convinced its net worse - and not trying to convince you of that - I think there is a range of outcomes, some positive and some negative, and that range doesn't justify blocking at this stage. I do believe it could be a net worse user experience if it does not provide enough benefit and makes the API more complicated to get a high level understanding of.

withoutboats commented 5 years ago

@rfcbot poll T-libs Should we move forward on stabilization without spending more time investigating the change to make the rawwaker vtable type generic?

Since the blocking concern is a libs API change, I thought it would be a good idea to ask other members of the libs team explicitly about their opinions.

Libs team: there's a proposal to change the API of the RawWaker to make it generic, potentially providing some additional type safety to people implementing a waker through the raw waker interface. Others (myself included) think this is not worth pursuing at this stage of the design process, because it doesn't have enough clear motivation to delay stabilization or devote further engineering resources to. I want to get your opinion.

I would interpret a checked box in response to this poll as saying you do not think we should block stabilizing futures on investigating this proposed API change. If you think we should, please leave a comment expanding on that opinion.

rfcbot commented 5 years ago

Team member @withoutboats has asked teams: T-libs, for consensus on:

Should we move forward on stabilization without spending more time investigating the change to make the rawwaker vtable type generic?

sfackler commented 5 years ago

Just stabilize the damn thing.

nikomatsakis commented 5 years ago

As I've stated previously, when it comes to "stylistic" or ergonomic considerations of this kind, and at this late phase of development, I'm inclined to give precedence to those who have been most closely involved in the design ('active working group members') in terms of when we have reached the point of diminishing returns.

However, I wanted to raise what I think may be a silent consideration here (perhaps it's just me, though). In particular, I see the appeal of adding generics to RawWaker and friends, as it can lead to an API that more directly expresses its expectations (indeed, we do not expect a *() pointer), which in turn can mean fewer casts. However, I can also see that that it makes the API more complex, in the sense that the type now becomes generic.

I don't find that this points me in any particular direction -- it's often tough to decide whether making the definition more complex ultimately makes it more or less easy to understand/use. I just thought it was maybe a worthwhile distinction to keep in mind.

cramertj commented 5 years ago

@Centril and I discussed this further. Here's a rough summary of what was covered:

I (@cramertj) feel like a lot of work went into getting consensus around the current API, and maintaining consensus has been a struggle. Every time a new even slightly controversial proposed change is introduced, consensus breaks and there are significant delays while we attempt to re-establish. Because of this, I think it's important not to introduce new design proposals that we haven't clearly identified as uncontroversial. New consensus-breaking proposals further delay the stabilization of futures_api and wear on community morale (and my own personal happiness in life :wink:).

On the specific issue at hand, I think there are specific areas in which a generic RawWakerVTable would be beneficial and provide better ergonomics than what we have today. However, I also believe there are other places where it would be harder to work with than what we have today, and would require an intermediate step to erase the generic parameters / replace with () in order to be able to share waking functions or RawWakerVTables between multiple types. I'd be more interested in a generic constructor (new function) which yielded a non-generic RawWakerVTable, as this solves the problem of having to manually type-erase. However, this solution could also be added backwards-compatibly at a later date.

Additionally, I anticipate that the total number of users of this API will never exceed two digits-- it's the low-level API underneath the low-level API: the futures crate uses the RawWaker API to build a very slightly higher-level API that is much safer and will be the one used by the majority of executor implementations such as the one I maintain for Fuchsia. Writing an executor is already a somewhat niche usecase, and writing new abstractions for writing executors is even more niche. I don't believe that the proposed change adds enough value to justify the cost of making further breaking changes to the API prior to stabilization.

@Centril pointed out that the change would have small but materially impactful benefits in real world code today, such as the wakers in the embrio-rs executor. I agree with this. However, I also think that (1) this use-case is quite niche already and (2) as the futures crate expands its no-std support and offers more utilites for building no-std-compatible executors, the embrio-rs code could benefit from using those higher-level abstractions and avoiding RawWaker entirely.

I (@cramertj) agree that the generic RawWakerVTable provides an improvement to the embrio-rs code. However, I also believe that such code is rare, and that the fix could potentially complicate other code relying on erased RawWakerVTables (requiring conversion to RawWakerVTable<()>). Most importantly, though, we've finally reached a hard-won consensus on the std::{future, task} API, and I further changes need to offer not only unambiguous value, but value that outweighs the cost of further delays to stabilization and the work necessary to regain consensus.

Centril commented 5 years ago

Based on ^---

@rfcbot resolved RawWakerVTable-if-generic-would-be-better-even-if-only-for-niche-uses

eddyb commented 5 years ago

As for me, as I remarked in https://github.com/rust-lang/rust/pull/59739#discussion_r272793941, I failed to notice any of this "raw" stuff was exposed publicly, otherwise I might've proposed the additional type safety, and I understand that I am too late now.

I've done "manual vtables" before¹, so this should've been more obvious to me, but I guess I was stuck on "we'll have some hacks in core because Rc/Arc aren't in it, but everyone will use std and implement a trait", which seems to have stopped being the plan a long time ago, but everything about futures is hundreds of comments and GitHub simply does not scale to that (which is ridiculous in 2019 IMO but what can we do...) so I couldn't keep track of most of it.

¹including with safe (skolemizing) unpacking (thanks to generative lifetimes) - I might be able to make an "erased type existential" wrapper once GATs land in Rust, that'd be fun!

withoutboats commented 5 years ago

@eddyb I do hope to see the safe and ergonomic Arc-based Wake trait in std someday, but its blocked on other features and not a prerequisite for stabilizing futures.

rfcbot commented 5 years ago

:bell: This is now entering its final comment period, as per the review above. :bell:

alexcrichton commented 5 years ago

@withoutboats or @cramertj, would one of y'all be up for preparing a stabilization PR for the standard library? While I believe we need to hold off r+ until the FCP has lapsed, we can be ready to r+ once FCP has lapsed ahead of time :)

Centril commented 5 years ago

@alexcrichton There's already one up, https://github.com/rust-lang/rust/pull/59739.

pnkfelix commented 5 years ago

@rfcbot concern async-fn-should-not-be-allowed

make sure that whatever you do to stabilize these APIs, it does not let #60069 leak out of nightly.