rust-lang / rust

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

`unreachable_patterns` fires on match arm that can't be removed. #129352

Open Dirbaio opened 3 months ago

Dirbaio commented 3 months ago

playground

use core::future::Future;

async fn select<A: Future, B: Future>(_: A, _: B) -> Result<A::Output, B::Output> {
    todo!()
}

pub async fn wtf() -> ! {
    let a = async { loop {} };
    let b = async { loop {} };
    match select(a, b).await {
        Ok(x) => x,
        Err(x) => x,
    }
}

warns with unreachable_patterns on both arms

warning: unreachable pattern
  --> src/lib.rs:11:9
   |
11 |         Ok(x) => x,
   |         ^^^^^
   |
   = note: this pattern matches no values because `Result<!, !>` is uninhabited
   = note: `#[warn(unreachable_patterns)]` on by default

but removing either doesn't actually compile

error[E0004]: non-exhaustive patterns: `Ok(_)` not covered
   --> src/lib.rs:10:11
    |
10  |     match select(a, b).await {
    |           ^^^^^^^^^^^^^^^^^^ pattern `Ok(_)` not covered
    |
note: `Result<(), !>` defined here
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:527:1
    |
527 | pub enum Result<T, E> {
    | ^^^^^^^^^^^^^^^^^^^^^
...
531 |     Ok(#[stable(feature = "rust1", since = "1.0.0")] T),
    |     -- not covered
    = note: the matched value is of type `Result<(), !>`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
11  ~         Err(x) => x,
12  ~         Ok(_) => todo!(),

So this code seems "unfixable" with the new lint.

What's happening is the match arm is unreachable, but it still has the "side effect" of making the async block be inferred to Future<Output=!> instead of Future<Output=()>. So, if you remove it the future now gets inferred to return (), and the arm is not unreachable anymore.

In edition 2024 it Just Works if you write match select(a, b).await {}, because the fallback is ! instead of ().

This seems a bit hard to mitigate in Edition 2021, it seems more like an unfortunate combination of the lint and the inference fallback more than a bug. I'm opening the issue anyway to share it, just in case anyone has an idea.

Noratrieb commented 3 months ago

cc @Nadrieril

WaffleLapkin commented 3 months ago

The way to fix this would be to specify an uninhabited type which isn't never type (and thus doesn't experience never type fallback). i.e. doing an identity::<Result<Infallible, Infallible>> or loop {} as Infallible. The problem is that both of these run into unreachable code lint.

I think the only way to fix the warning is to specify the type of the futures, either directly:

fn ascribe_future<R>(f: impl Future<Output = R>) -> impl Future<Output = R> { f }

let a = ascribe_future::<Infallible>(async { loop {} });
let b = ascribe_future::<Infallible>(async { loop {} });

or in the select:

async fn select<A: Future<Output = RA>, B: Future<Output = RB>, RA, RB>(_: A, _: B) -> Result<A::Output, B::Output> {
    todo!()
}

match select::<_, _, Infallible, Infallible>(a, b).await {}

Neither is particularly nice and the interaction with the old never type fallback is very unfortunate, but yeah :(

Nadrieril commented 3 months ago

Well that's fun :D Yeah, to mitigate we'd have to make pattern exhaustiveness influence type inference I think, which... yeah.

traviscross commented 2 months ago

The code above can be fixed to not warn with:

//@ edition: 2021
use core::future::Future;

async fn select<A: Future, B: Future>(_: A, _: B) -> Result<A::Output, B::Output> {
    loop {}
}

pub async fn f() -> ! {
    let a = async { loop {} };
    let b = async { loop {} };
    match select(a, b).await {
        _ => unreachable!(), //[2021]~ <--- No warning.
    }
}

Playground link

This does warn in Rust 2024 as one might expect and as is desirable.

traviscross commented 2 months ago

Here's a minimization of the original issue:

pub fn g<T>(_: fn() -> T) -> Option<T> { loop {} }
pub fn f() -> ! {
    match g(|| loop {}) {
        Some(x) => x,
        //[2021]~^ WARN unreachable pattern
        None => unreachable!(),
    }
}

Playground link

(There's some similarity to the never type hack here, both in the minimization and in the original.)

As in the original, removing the arm works in Rust 2024 but not in Rust 2021. However, in Rust 2021 we can write, warning-free, either:

pub fn g<T>(_: fn() -> T) -> Option<T> { loop {} }
pub fn f() -> ! {
    match g(|| loop {}) {
        Some(()) => unreachable!(),
        None => unreachable!(),
    }
}

Playground link

...or:

pub fn g<T>(_: fn() -> T) -> Option<T> { loop {} }
pub fn f() -> ! {
    match g(|| loop {}) {
        _ => unreachable!(),
    }
}

Playground link

Dirbaio commented 2 months ago

it's undesirable to have to write unreachable! though. i'd rather have the compiler prove unreachability for me, than trust my word and get a panic if i'm wrong.

traviscross commented 2 months ago

Sure. That's not the point being made here.

Dirbaio commented 2 months ago

What is the point being made, then?

traviscross commented 2 months ago

Some had believed there was no reasonable way to fix this code in Rust 2021 other than by allowing the warning or us choosing to not warn on this at all. The fix shown is reasonable, is an improvement to the code (in my opinion), and works in Rust 2021.

To your point, sure, it'd be even better if, all else being equal, the branch could just be removed in Rust 2021 (without doing the things @WaffleLapkin suggested) as it can be in Rust 2024. But that's a separate matter.

Dirbaio commented 2 months ago

The fix shown is reasonable, is an improvement to the code (in my opinion),

The fix makes the code worse, as I said. Before mistakes would manifest as compilation errors, with unreachable!() they manifest as runtime panics.

Dirbaio commented 2 months ago

This is the code where I ran into this originally, if you're curious.

The two futures are supposed to never return. I could've simply written join(tx_fut, rx_fut).await; unreachable!(), but that would've caused a panic if e.g. a future was accidentally made to return in a future refactor. The whole point of the match is to offload the unreachability check to the compiler, so if a future is accidentally changed to return it'll fail to compile, not panic at runtime. Adding unreachable!() inside the match defeats the point.

traviscross commented 2 months ago

Thanks. OK, I see now what you were trying to enforce there.

traviscross commented 2 months ago

Reflecting on this, there are I think two separate issues:

  1. How to write the code in Rust 2021 in a way that doesn't warn.
  2. The best way to make the static assertion that is trying to be made here.

It seems kind of an accident that this approach to the static assertion works at all without warnings. Consider, e.g., that this spiritually-identical code warns on stable Rust:

fn select<A, B>(_: impl FnOnce() -> A, _: impl FnOnce() -> B) -> Result<A, B> {
    todo!()
}

pub fn f() -> ! {
    let a = || loop {};
    let b = || loop {};
    match select(a, b) {
 // ^^^^^ WARN unreachable expression
        Ok(x) | Err(x) => x,
    }
}

Playground link

That adding in an .await, as in the original example, hides this from the lint doesn't seem fundamental.

That warning would push most people to instead write:

pub fn f() -> ! {
    let a = || loop {};
    let b = || loop {};
    _ = select(a, b);
    unreachable!()
}

Playground link

...that of course loses the static assertion you want to express. At which point, I'd probably just express that static assertion directly:

fn assert_output<T>(_: &impl FnOnce() -> T) {}

pub fn f() -> ! {
    let a = || loop {};
    let b = || loop {};
    assert_output::<Infallible>(&a); //~ <---
    assert_output::<Infallible>(&b); //~ <---
    _ = select(a, b);
    unreachable!()
}

Playground link

We can apply that same transformation to the original example:

async fn select<A: Future, B: Future>(_: A, _: B) -> Result<A::Output, B::Output> {
    todo!()
}

fn assert_output<T>(_: &impl Future<Output = T>) {}

pub async fn f() -> ! {
    let a = async { loop {} };
    let b = async { loop {} };
    assert_output::<Infallible>(&a);
    assert_output::<Infallible>(&b);
    _ = select(a, b).await;
    unreachable!()
}

Playground link

This approach still works even if the match isn't dead, e.g., this is warning-free in all editions:

pub fn f() -> ! {
    let a = || loop {};
    let b = || ();
    assert_output::<Infallible>(&a);
    match select(a, b)() {
        Err(()) => todo!(),
    }
}

Playground link

jannic commented 2 months ago

The warning is similarly annoying in cases like this:

fn returns_result() -> Result<(), Infallible> {
    Ok(())
}

fn main() {
    match returns_result() {
        Ok(_) => println!("ok!"),
        Err(_) => println!("err!"),
    }
}

https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=afc918547e8bfc948713353daf0bb12d

Of course, here the Err branch can just be removed in rust 1.82.0 (beta). But that won't work with current stable. What's the best transition strategy to make both versions compile the code without warnings? Just ignore the warning?

Nadrieril commented 2 months ago

We'll be reverting the lint change for 1.82.0 and deciding how to proceed further once that's done (likely with a sublint for the empty pattern case)