unreachable_patterns lint due to min_exhaustive_patterns

[kpreid]

Code

fn foo() -> Result<(), core::convert::Infallible> {
    todo!()
}

fn main() {
    match foo() {
        Ok(()) => {},
        Err(e) => match e {}
    }
}

Current output

warning: unreachable pattern
 --> src/lib.rs:8:9
  |
8 |         Err(e) => match e {}
  |         ^^^^^^
  |
  = note: this pattern matches no values because `Infallible` is uninhabited
  = note: `#[warn(unreachable_patterns)]` on by default

Desired output

Until the 2024 edition, or the code's MSRV is ≥ 1.82, no warning.

Rationale and extra context

Deleting the Err(e) => match e {} arm will produce code which does not warn on nightly/1.82, but which does not compile on 1.80. This means that users which wish to keep a warning-free-ish nightly (or beta, eventually) build will have to #[allow(unreachable_patterns)] from now until 1.82 is released, or perhaps even until their next MSRV increase.

Deferring the warning until an edition or explicit MSRV increase seems to me to be a good way to avoid churn in the form of #[allow]s. Such #[allow]s are particularly dangerous because unreachable_patterns is also the lint that detects binding patterns that were intended to be matching against constants instead, so declaring it on a wide scope might hide serious bugs, especially if (as is likely) they're not promptly removed upon the release of 1.82.

Rust Version

1.82.0-nightly (2024-08-11 41dd149fd6a6a06795fc)

Anything else?

I noticed this due to my CI against nightly, but comments on the stabilization PR #122792 (comment) mentioned it first. I'm filing this issue to make sure it is not overlooked by being only visible in that comment thread.

[compiler-errors]

@Nadrieril: Do you mind preparing a PR to split out these new cases from the unreachable_patterns, so we can mark it as allow for a few versions (or at least so people can allow it separately from the unreachable pats lint)? Perhaps call it impossible_pattern or something?

[compiler-errors]

Such #[allow]s are particularly dangerous because unreachable_patterns is also the lint that detects binding patterns that were intended to be matching against constants instead

This is a great point IMO, and feels like enough justification to warrant splitting this lint out.

[traviscross]

@rustbot labels +T-lang +I-lang-nominated

Nominating to handle the question of whether bumping the lint to warn-by-default should be done now, done never, done in a few versions, or done in an edition-dependent way (and then maybe done in all editions later).

[Nadrieril]

Yep, will prepare a PR when I get a moment

[scottmcm]

Perhaps call it impossible_pattern or something?

Can we make sure this is part of a group, so that macros that intentionally allow(unreachable_patterns) because it depends how it's instantiated will still have it allowed?

TBH my instinct here is that people should be allowing it on the arm specifically, rather than globally, if they feel the need.

(Or the latest-rust CI can allow it while the MSRV-rust CI can still deny the warning.)

[joshtriplett]

FWIW, I don't think we should treat the introduction of impossible_patterns as a prerequisite for landing this, on the basis that (as @scottmcm noted) you can #[allow(unreachable_patterns)] specifically on the one match arm, which shouldn't cause harm.

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in our triage meeting today. We didn't feel that separating out this lint was necessary because, among other reasons, the allow could be placed directly on the match arm for anyone who wants to preserve MSRV. If T-compiler does want to separate out this lint, that's fine by us as long as unreachable_patterns then becomes a lint group that includes impossible_patterns.

We also discussed how this could be a good use case for cfg(rust_version) (tracking issue).

[scottmcm]

Such #[allow]s are particularly dangerous because unreachable_patterns is also the lint that detects binding patterns that were intended to be matching against constants instead

I think this is overstated, because doing that also gives a naming lint (because the constant is SCREAMING instead of lower) and an unused-variable lint (because you didn't use the binding). And, helpfully, those apply even if the arm is the last one, or in an if let, where unreachable doesn't.

And in some cases you get specific deny-by-default lints like

error[E0170]: pattern binding `Less` is named the same as one of the variants of the type `std::cmp::Ordering`
 --> src/lib.rs:3:9
  |
3 |         Less => {},
  |         ^^^^ help: to match on the variant, qualify the path: `std::cmp::Ordering::Less`
  |
  = note: `#[deny(bindings_with_variant_name)]` on by default

So if you can find a heuristic that we can make a new deny-by-default lint against "gee, that sure looks like you meant to match a constant there" for, please propose it. I love approving "hey, we know that case and it's not what you wanted" lints :)

[compiler-errors]

I still think that this should be split. Just because users can put an #[allow(unreachable_patterns)] on precisely the match arms that now trigger this lint does not enforce that they do so. They may very well put it on the top of their module like they do with other lints, and this is a footgun like we pointed out above.

[compiler-errors]

I find the unreachable patterns lint to be very high signal towards there being an underlying problem with the code, and impossible patterns to be useful for signaling "hey, this code is dead" but not necessarily at the same severity level as the former, especially because we required users to include these dead arms previously.

I find it kind of a shame to lump these two together in the mean time, as users churn to remove these arms now that pattern exhaustiveness takes never into account, but a lint group is fine I guess.

[Nadrieril]

I'm also in favor of delaying the lint. The ability to omit the arms is the heart of the feature, we can add the lint later for discoverability.

[Nadrieril]

I have a PR up: #129103

[traviscross]

Our feeling, among those on the call, was that we do want this lint to fire, and that this is the purpose of lints, even though, yes, we're unfortunately going from requiring it to warning about it.

Given that, and that we today discussed and discarded the idea of doing this over an edition, I doubt we're going to have appetite for that.

Could we perhaps make this lint machine-applicable in some targeted way (i.e. that would distinguish these no-op cases from cases that might indicate a bug)? I known that doesn't address the MSRV thing, but it would make the migration easier otherwise.

[Nadrieril]

The proposed --msrv flag rust-lang/compiler-team#772 would be a great solution here: we'd only warn if the crate's msrv allows the arm to be omitted.

[joshtriplett]

--msrv would be great, but isn't going to ship and be wired up in time to help with this.

allow(unreachable_patterns) on the arm will work, and allow(impossible_patterns) will work if we ship that for 1.82.

Could we ad a note to the lint message along the lines of "if you need this match arm for compatibility with older Rust, add allow(impossible_patterns) on the match arm"?

[joshtriplett]

Renominating this based on seeing feedback about impact.

[clarfonthey]

I think that an MSRV flag for this would be ideal because even this is just a subset of impossible patterns, and presumably more will be added in the future. I don't think that being forced to separate the lint into a different name every time a new case gets added is a good idea, but I would accept as a reasonable compromise changing what is implicitly allowed by the lint with an MSRV flag.

[joshtriplett]

I think that an MSRV flag for this would be ideal because even this is just a subset of impossible patterns, and presumably more will be added in the future. I don't think that being forced to separate the lint into a different name every time a new case gets added is a good idea, but I would accept as a reasonable compromise changing what is implicitly allowed by the lint with an MSRV flag.

I would love to have the --msrv flag for this and many other purposes, but that's going to take a while to 1) add, 2) stabilize, and 3) wire into Cargo to pass automatically. We need at least an interim mitigation for users of stable 1.82.

[clarfonthey]

I mean, truthfully, I think that the simplest immediate mitigation is to not trigger this lint for impossible patterns. This allows omitting them but won't complain if you include them. Once a better mitigation is in place (like an MSRV-aware linter) that can be fixed.

I don't think that separating out impossible patterns as its own lint category is a good idea, since like I said, that pattern will be extended in the future to include more patterns than are noticed right now.

[cuviper]

I think that mitigation would be ok, but I suspect that distinguishing the cases that shouldn't trigger here is about the same amount of work as creating a new lint for those cases. It's less work downstream though if there's nothing you need to allow (for now).

[Nadrieril]

New factor to take into account: in some (rare) cases, because of type inference the arm cannot in fact be removed in edition 2021: #129352

[joshtriplett]

@clarfonthey wrote:

I mean, truthfully, I think that the simplest immediate mitigation is to not trigger this lint for impossible patterns. This allows omitting them but won't complain if you include them. Once a better mitigation is in place (like an MSRV-aware linter) that can be fixed.

That's a really good point. We have the goal of eventually removing those branches, but I think our original decision about linting here was based on not having the possibility of MSRV-aware linting in the future, so we wouldn't have been able to do any better. Given the possibility of --msrv, and...

@Nadrieril wrte:

New factor to take into account: in some (rare) cases, because of type inference the arm cannot in fact be removed in edition 2021: #129352

...this, I think we should revert this decision and avoid linting on this for now.

[Nadrieril]

I have a PR ready if y'all want to FCP this in time for the beta fork (August 30th, tho we can of course always backport).

EDIT: I can't count, that's too short. Still, PR is there and we can discuss details.

[tgross35]

I have a PR ready

This link seems to point to this issue. Did you mean #129103?

[Nadrieril]

whoops, indeed 🙏

[joshtriplett]

I have a PR ready if y'all want to FCP this in time for the beta fork (August 30th, tho we can of course always backport).

EDIT: I can't count, that's too short. Still, PR is there and we can discuss details.

I think if we get consensus and we're in FCP, we can reasonably get this merged in before the deadline even though we don't have a full 10 days until then.

[glandium]

Another factor:

fn foo(x: std::os::raw::c_ulong) -> Option<u32> {
    let x: u32 = match x.try_into() {
        Ok(session) => session,
        Err(_) => return None,
    };
    Some(x)
}

(don't mind the fact that this could be written differently, the real code this is derived from does need the match)

This emits the unreachable pattern warning... on Windows targets only.

[glandium]

Another case:

enum Foo {
#[cfg(feature = "bar")]
Bar
}

enum Qux {
    A,
    Foo(Foo)
}

fn foo(x: Qux) -> bool {
    match x {
        Qux::A => true,
        Qux::Foo(_) => false,
    }
}

The "right" fix here would be to add #[cfg(feature = "bar")] to the Qux::Foo branch, but it breaks the build on rustc < 1.82. Yes, you can use #[allow(unreachable_patterns)] but then, the compiler won't make you revisit this when you bump your MSRV.

[glandium]

This emits the unreachable pattern warning... on Windows targets only.

and 32-bits targets.

[apiraino]

reopening to follow backport. I assume T-lang has approved the changes to close this (RFC here)

@rustbot label -I-lang-nominated