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

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

Open
Dirbaio opened this issue Aug 21, 2024 · 15 comments
Open

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

Dirbaio opened this issue Aug 21, 2024 · 15 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. F-never_type `#![feature(never_type)]` L-unreachable_patterns Lint: unreachable_patterns T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Dirbaio
Copy link
Contributor

Dirbaio commented Aug 21, 2024

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.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 21, 2024
@jieyouxu jieyouxu added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. L-unreachable_patterns Lint: unreachable_patterns T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Aug 21, 2024
@Noratrieb
Copy link
Member

cc @Nadrieril

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 21, 2024
@WaffleLapkin
Copy link
Member

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
Copy link
Member

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

@traviscross
Copy link
Contributor

traviscross commented Aug 22, 2024

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

This comment was marked as resolved.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Aug 22, 2024

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

This comment was marked as resolved.

@Dirbaio

This comment was marked as resolved.

@traviscross

This comment was marked as resolved.

@Dirbaio

This comment was marked as resolved.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Aug 22, 2024

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
Copy link
Contributor

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

@traviscross
Copy link
Contributor

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
Copy link
Contributor

jannic commented Sep 9, 2024

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
Copy link
Member

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)

jannic added a commit to jannic/rp-hal that referenced this issue Sep 10, 2024
The warning happening in beta will be rolled back (for now) according to
rust-lang/rust#129352 (comment)

Until this change propagates to a beta release, we can live with the
warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. F-never_type `#![feature(never_type)]` L-unreachable_patterns Lint: unreachable_patterns T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants