Futures-rs: Replace `Never` with `!`

[davll]

Recently, [features(never_type)] is stabilized in rustc master branch. PR Link

And I remember that Never type in futures is a temporary replacement for !. Now ! type is stabilized and shall we proceed to replace Never with !?

[aturon]

Unfortunately this would also bump up the version of Rust required to use futures, so we probably won't make this change right away.

[carllerche]

Isn't that OK to do on a breaking release?

[aturon]

@carllerche My concern is not so much about breakage as that (1) futures is intended to be a foundational library and (2) this change doesn't buy us very much. I'd prefer not to impose stronger requirements on rustc versions unless there's a clear need for it.

[jimmycuadra]

As a counterpoint, futures is a library with a lot of visibility and lots of people will be looking at it as an example of library design in Rust, and so it could be a good idea to use it to showcase idiomatic Rust, which I'd expect to include shedding old baggage when the language offers a proper route to take to avoid old hacks. In other words, when people ask, "Why would you need the never type?" you can point to a high-profile real-world example here in futures.

Furthermore, supporting pre-never-type rustc seems like a conflicting goal given that futures 1.0 and the larger Rust async stack is being closely tied to the Rust 2018 era.

[davll]

I agree, futures should be not only a fundamental library for asynchronous programming but also a showcase of rust syntax. And futures 0.2 has room for improvements toward 1.0 IMO, so we should not constrain it on outdated things before 1.0 release.

As a compromise proposal, we can let futures 0.3 get ! instead of Never and make Never as an optional feature for older compilers.

[stbuehler]

The stabilized feature isn't released yet afaict, and it will probably take some weeks until 1.0.26.

Apart from that I disagree with the "don't want to use latest stable features because it would bump the required Rust version" (although I'd very much like to see https://github.com/rust-lang/cargo/issues/2751; if a "non-breaking" crate update suddenly requires a new rust version you'll have a very bad time finding working crate versions on older rustc). Also I'm pretty sure over the year there will be a lot of features in rust that futures will want to use ASAP.

But if you really want to go that way, please note that the current Never type cannot be replaced by ! without breakage. See https://github.com/stbuehler/futures-rs/commit/hide-never-impl-for-std-never-alias for a possible patch.

And afaict it will be impossible to replace without breakage if crates (like this one) use things like #[deny(unreachable_code)] - my conclusion is that it is very bad style to make such warning an error in a non-developer build.

[Nemo157]

And afaict it will be impossible to replace without breakage if crates (like this one) use things like #[deny(unreachable_code)]

Luckily cargo disables the ability for dependencies to deny lints, so even if futures has #[deny(unreachable_code)], that will become #[warn(unreachable_code)] when compiling an application that depends on futures.

[ngg]

I think it is a bad idea to make Never two different types depending on the "nightly" feature as done in https://github.com/stbuehler/futures-rs/commit/hide-never-impl-for-std-never-alias. It would make the non-nightly and nightly versions slightly incompatible with each other, like proc-macro2 0.2.x did. Adding an extra feature should not break compatibility (if different dependencies depend on different features of the same crate then the union of the feature sets will be used). If there are really no cases where changing to nightly could break any valid code then ok of course.

[stbuehler]

I admit I'm not completely sure what the nightly feature is about; I took it as a flag to see what future versions could look like, and the point of using it was to demonstrate how replacing the Never type with an alias to ! could look like. I don't care if it is actually committed that way.

I don't think the nightly feature should be treated as a normal feature, where you would assume it is stable like the rest of the API and so on. And the name clearly suggests you'll need a nightly rust build to use it, so it should be clear there are no stability guarantees.

But actually using nightly in my "example" should be stable regarding the interface: code that compiles without nightly should compile with nightly too (just not the other way round) - after all that was the point of it, to make Never replaceable without breaking the API.

[ngg]

But it wouldn't be fully compatible unfortunately. For example the following code would be perfectly valid (and possibly sane for a crate using some custom error handling) for the non-nightly version but would fail to compile due to conflicting implementations on nightly.

struct MyError;

impl From<Never> for MyError {
    fn from(_: Never) -> Self {
        unreachable!()
    }
}

impl From<!> for MyError {
    fn from(_: !) -> Self {
        unreachable!()
    }
}

IMHO the nightly feature should be handled as a normal feature. For example let's assume you want to use async/await in your crate so you enable the nightly feature on futures but you also have some dependencies that only use normal futures (and not nightly). In this case cargo will compile in the nightly feature which can break the crates you depend on. This is exactly what happened with https://github.com/alexcrichton/proc-macro2/issues/67.

[stbuehler]

Good point (about the conflicting impls). I see no way around that - do you?

[ngg]

@stbuehler Not really.

I think this change should be somehow tied to the Rust 2018 edition, with a new major release (like 0.3 or 1.0), in which case it should not be a problem to increase min rustc version. (It would be great to require that for async/await support on stable too). Though I think it would be not that bad to require new stable compiler when bumping to 0.2, I can see why some people are against it.

[ngg]

This should have the same resolution as https://github.com/rust-lang/rust/pull/49039. (It doesn't seem to be decided yet)

[davll]

It seems that the issue is superseded by the approach of future-0.3 and thus the issue can be closed now.