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

futures-core 1.0 release #2207

Open
7 tasks
taiki-e opened this issue Sep 4, 2020 · 19 comments
Open
7 tasks

futures-core 1.0 release #2207

taiki-e opened this issue Sep 4, 2020 · 19 comments
Assignees

Comments

@taiki-e
Copy link
Member

taiki-e commented Sep 4, 2020

This is a proposal for the next major release of futures-core.
(I will open issues for other crates later. They are probably 0.4 instead of 1.0.)

About

I propose to further reduce the APIs of futures-core and only provide what is really stable.
This makes futures-core a shim crate that provides future/task-related APIs of the standard library.
Eventually (maybe after core::stream::Stream stabilized), only crates that need compatibility with older compilers will depend on futures-core, and other crates will not need to depend on futures-core.

APIs provided by this crate

The following two kinds of APIs are provided:

  • APIs already stable in the standard library:
    • In versions where it is stable, re-export from the standard library
    • In versions where it doesn't exist in the standard library or isn't stable yet, futures-core defines & provides the same (use build-script to detect it).
      This allows you to use items that are only available in the latest rustc, even if you are using an older version of rustc.
  • APIs not yet stable in the standard library:
    • If we know what (definition of items) will be added to the standard library, we can define and provide the same until it stabilizes.
    • However, basically do not add at this stage.
      • Stream trait and ready! macro are probably the only exceptions to this.
    • Once stable, we can re-export with later versions (use build-script to detect it).

Other APIs are provided by individual futures-* crate.

MSRV

APIs of this crate is really small, so it would be nice to be able to pin it to one of the following:

  • Rust 1.36 that future/task-api stabilized.
  • Rust 1.39 that async-await stabilized.

Every time the MSRV is increased, a new minor version is released.

Changes

Here is a list of proposed actual changes:

  • FusedFuture, FusedStream.
  • Type aliases (BoxFuture, LocalBoxFuture, BoxStream, LocalBoxStream); pending in Move type/trait aliases to futures-util #2344
    • Move to futures-util.
    • They are just type aliases of Pin<Box>ed future/stream.
  • Trait aliases (TryFuture, TryStream); pending in Move type/trait aliases to futures-util #2344
  • Stream trait
    • Once the std Stream trait is stable, futures_core::Stream is replaced with re-export from the standard library. (In the version that has stable std Stream, std Stream and futures_core::Stream is the same trait.)
      * Add next method from StreamExt.
      • This may depend on the decisions of @rust-lang/wg-async-foundations or other teams. (As much as possible, I'd like to avoid the situation where the API of Stream changes after futures-core 1.0 is released.)
    • Do not add other methods from StreamExt/TryStreamExt. There is no general agreement on those APIs. Especialy, async closure vs normal closure.
  • ready! macro
    • as-is
    • This is still unstable in the standard library, but I don't think the API itself will change.
    • The standard library's ready! macro maybe core::task::ready! instead of core::ready!, but futures-core expose this at top-revel.
      Because:
      • To do the same requires an unstable feature.
      • Unlike the standard library, futures-core provides only future/task-related items.
  • Wake trait
    • Currently named as ArcWake, and defined in futures-task.
    • Once core::task::Wake trait stable, re-export/define in futures-core.
    • From-impl may not be available in the old version, so provide conversion utilities (waker, waker_ref) in futures-task or futures-util.
  • Other utilities in std::{future, task, stream}
    • Once stable, re-exported/defined in futures-core.
    • And re-exported in futures-util. (Be careful with minimal-versions to avoid compile failure.)

EDIT1: Clarified futures_core::Stream changes when std Stream is stable. (originally only described in "about" section's "APIs not yet stable in the standard library")


cc @cramertj @Nemo157 @seanmonstar
FYI @rust-lang/wg-async-foundations

@yoshuawuyts
Copy link
Member

This all sounds great! One question I have is about the timing of a futures-core 1.0 release: would we want to wait for the Streams RFC to be accepted first? That way we know the decision on Stream::next and can ensure no changes occur?

@taiki-e
Copy link
Member Author

taiki-e commented Sep 4, 2020

Even after the RFC has been merged, it can be changed with good reason, so it is a very difficult question at which stage to determine that. (The risk cannot be zero.)
There are some cases where lang-team has "resolved" a part of unstable feature (e.g., Ok-wrapping for try blocks, .await syntax), but in either case it is not possible to fully guarantee that all APIs in Stream trait will not be changed before stabilization.

That said, "when the RFC is merged" is probably one of the reasonable timings.

@seanmonstar
Copy link
Contributor

This sounds great to me. In general I'd prefer to start chopping some things from futures, and this is a great start!

The only thing I'm unsure about is having trait Stream defined inside futures-core as v1.0, when it may soon be added to the standard library. It's possible for someone to implement futures_core::Stream on a type, and then implement std::stream::Stream for the same type. Then later making futures_core::Stream a re-export would then make it a conflict for the user.

@taiki-e
Copy link
Member Author

taiki-e commented Sep 8, 2020

@seanmonstar

The only thing I'm unsure about is having trait Stream defined inside futures-core as v1.0, when it may soon be added to the standard library. It's possible for someone to implement futures_core::Stream on a type, and then implement std::stream::Stream for the same type. Then later making futures_core::Stream a re-export would then make it a conflict for the user.

In this proposal, once the std Stream trait is stable, futures_core::Stream is replaced with re-export from the standard library. (In the version that has stable std Stream, std Stream and futures_core::Stream is the same trait.)

This idea originally mentioned in Async Interview #2:

Presuming that some trait from futures is moved wholesale into libstd (i.e., without any modification), then it is possible for us to simply issue a new minor version of the futures crate (and the appropriate subcrate). This new minor version would change from defining a trait (say, Stream) to re-exporting the version from std.

UPDATE: Updated issue description to add a description of this in the Stream trait section. (originally only described in "about" section's "APIs not yet stable in the standard library")

@Nemo157
Copy link
Member

Nemo157 commented Sep 9, 2020

I think to make it a forward-compatible update the build script must detect std::?::Stream from v1.0.0 and either re-export or fail to compile with it. Having a local Stream and making a future version start re-exporting it from std would be a breaking change (even if there is a patch release doing the re-exporting before std::?::Stream is stable).

Alternatively this could be a documented "unchecked" future-incompatibility so that it's not compiler checked like that, but users are told that simultaneously implementing futures-core::Stream and std::?::Stream is consider invalid.

@taiki-e
Copy link
Member Author

taiki-e commented Sep 9, 2020

Having a local Stream and making a future version start re-exporting it from std would be a breaking change (even if there is a patch release doing the re-exporting before std::?::Stream is stable).

Yes, you are right.

I think to make it a forward-compatible update the build script must detect std::?::Stream from v1.0.0 and either re-export or fail to compile with it.

Maybe a build-script like this could resolve the problem?

// build.rs
#[cfg_accessible(core::?::Stream)] // `cfg_accessible` isn't stable so in practice we probably use `autocfg`.
println!("cargo:rustc-cfg=std_stream_trait");

// lib.rs
#[cfg(std_stream_trait)]
pub use core::?::Stream;
#[cfg(not(std_stream_trait))]
pub trait Stream {
    // ...
}

@taiki-e
Copy link
Member Author

taiki-e commented Sep 9, 2020

Another concern with using build-scripts is their compatibility with non-cargo build systems that do not run build-scripts.
In this case, the trait imported from futures and the trait imported from std will always be different traits. (If using the build-script like said in the comment above.)

We may need to consider something like serde-rs/serde#1593 (support only the latest stable Rust on those build systems), but in any case, breakage may be unavoidable...

(fwiw, we can check if build-script is run by unconditionally emitting rustc-cfg in build-script.)

@withoutboats
Copy link

I don't understand why a build script would be needed if you're using cfg accessible.

@taiki-e
Copy link
Member Author

taiki-e commented Sep 9, 2020

cfg-accessible isn't stable yet, and a stable alternative requires build-script, so I've written the above example using build-script.
Once cfg-accessible is stable, we can do it without build-script, as you said. (However, in any case, build-script may be needed to support old compilers)

@taiki-e
Copy link
Member Author

taiki-e commented Sep 24, 2020

FYI: I've created a demo of how Stream re-export trick implemented: https://github.com/taiki-e/futures-compat-experiment

@taiki-e
Copy link
Member Author

taiki-e commented Feb 13, 2021

#2335

@taiki-e taiki-e self-assigned this Feb 13, 2021
@taiki-e taiki-e modified the milestones: futures-core-1.0, futures-0.4 Feb 13, 2021
@tmandry
Copy link
Member

tmandry commented Feb 17, 2021

I'd still consider it a possibility that Stream::next is added prior to stabilization, or even after (since it's a provided method). This might create weird situations with re-exporting if the futures-core version being used and the standard library version being used don't agree that next exists.

In particular, if futures-core lacked Stream::next it would no longer make code which depends on Stream::next backward-compatible with older versions of Rust, until the user upgraded to a version which included the method. But this situation is hard to detect for a maintainer who's using a newer version of Rust and is therefore getting a re-export.

Maybe that's an acceptable compromise?

@tmandry
Copy link
Member

tmandry commented Feb 17, 2021

I also have concerns around the naming of poll!: rust-lang/rust#81050 (comment)

@yoshuawuyts
Copy link
Member

Another item I think is worth discussing is the removal of FusedStream and FusedFuture. In particular FusedStream's usefulness extends beyond the select! {} macro, and has a clear counterpart in std in the form of FusedIterator. I'd like us to keep FusedStream, but with is_terminated removed as a required method so we can maintain parity between Iterator and Stream.

Perhaps this should be outside of the futures-core lib, but I wouldn't want to see it be removed altogether.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 19, 2021

I also have concerns around the naming of poll!: rust-lang/rust#81050 (comment)

@tmandry I understand your concerns, but basically, I don't think we really need to rename it. future::ready is rarely used in practice as I mentioned in the PR that stabilized it, and I think it will be virtually deprecated in favor of async fn/block once type_alias_impl_trait stabilizes.

Another item I think is worth discussing is the removal of FusedStream and FusedFuture. In particular FusedStream's usefulness extends beyond the select! {} macro, and has a clear counterpart in std in the form of FusedIterator. I'd like us to keep FusedStream, but with is_terminated removed as a required method so we can maintain parity between Iterator and Stream.

@yoshuawuyts As I've mentioned several times, Fused* doesn't work as expected without min_specialization. I think there is not much benefit to having them until the min_specialization is stable.

@Nemo157
Copy link
Member

Nemo157 commented Feb 19, 2021

futures::ready is still very useful when writing async IO implementations, I think we're a long way off having async-block defined IO types.

@yoshuawuyts
Copy link
Member

As I've mentioned several times, Fused* doesn't work as expected without min_specialization

@taiki-e I'm not familiar with the argument, most likely because I don't use select! {}. Would you mind repeating the argument here, or linking to some earlier location where you've made the argument?

If we're uncertain about FusedStream I don't feel strong enough to push in favor of its inclusion. But I do want to better understand your thinking on the topic, as it seems to differ from mine.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 19, 2021

I think we're a long way off having async-block defined IO types.

@Nemo157 Interesting. Do you think it's the timing when type_alias_impl_trait is stable? Or will it take longer?

@taiki-e
Copy link
Member Author

taiki-e commented Feb 19, 2021

@yoshuawuyts
I think #1989 and #1894 written by @Matthias247 have some great explanations.
The main issue is it needs to specify the trait requirement at the bounds level like select. (AFAIK, this is the biggest reason why current select is difficult to use) Something like autoref based specialization may work in some cases, but it's a hack I don't think it's guaranteed behavior...

One of the other problems is that it's difficult to propagate an implementation correctly from the underlying type without specialization. For example, in #2213, the current implementation is correct if the underlying types do not implement FusedFuture, but is incorrect if it is implemented.

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

No branches or pull requests

6 participants