Create 0000-unreachable-patterns-error.md

[davemilter]

No description provided.

[TimNN]

Rendered.

One potential situation where I imagine multiple matching patterns could be useful is in macro-generated code. However given that the warning is implemented as a lint it seems reasonable to make the lint deny by default (if the current rust stability guidelines allow that), which macro authors can selectively disable.

[petrochenkov]

It was an error, it was made a warning just recently, in Rust 1.16.
See rust-lang/rust@bcdbe94 and rust-lang/rust#38069 for more details.

[mgattozzi]

I don't think this should be turned into an error. An unreachable pattern is generally indicative of bad code smell but it's not something that would cause a problem with the execution. It's unreachable but that's not a problem really. We make Non-Exhaustive patterns an error because in this case what happens if there's a match and nothing to handle it? The execution flow would fail to work properly. I can see value in this but it seems like it's better off as a warning lint as is and not an error as it will not cause the code to fail to work. Clippy for instance lints against bad code like this but doesn't make it an error. If you really want these to be an error you can just put #![deny(warnings)] at the top of lib.rs or main.rs

[davemilter]

If you really want these to be an error you can just put #![deny(warnings)] at the top of lib.rs or main.rs

Actually it is not as simple as it looks like. There is no way to switch on/off this for whole crate,
there is [[bin]], integration tests also seems to be treated as external crates, may be other stuff?

[davemilter]

An unreachable pattern is generally indicative of bad code smell but it's not something that would cause a problem with the execution

It causes execution of branches that not expected to execute,
of course this question of terminology, but for it is cause problem during execution.

And it often enough issue, for example: https://stackoverflow.com/search?q=%5Brust%5D+unreachable+pattern

[steveklabnik]

There is no way to switch on/off this for whole crate,

To be clear, this does switch it on/off for the whole crate; you're talking about packages, which have multiple crates in them.

[oli-obk]

How about we improve the warnings in the presence of patterns where there's a local variable of the same name? We already have this for enum variants.

[Dushistov]

@oli-obk

How about we improve the warnings in the presence of patterns where there's a local >variable of the same name? We already have this for enum variants.

I like rust because of it is solid as a rock to prevent errors.
You do something wrong =(with huge probability) you got compile time error.
That is why I agree with @davemilter let make it consistent:
missed something to match = error,
match something twice = error.

That's why if choice is between some corner corner case (rust-lang/rust#38069 ) and "solid as a rock" compile time check, then choose "solid as a rock" and for corner case use some corner work around.

[Dushistov]

/cc @canndrew

So what about reverting of rust-lang/rust#38069 ? I think consistent semantic of language is more importnat then handling of empty enum case.

[oli-obk]

I like rust because of it is solid as a rock to prevent errors.

I don't differentiate much between errors and warnings. For me any diagnostic output means I cannot publish as it is. I even try to be warning free while developing (or at least only have fewer than 5 warnings).

I think consistent semantic of language is more importnat then handling of empty enum case.

That PR was made out of a concrete reason (consistency!). Please read the referenced issue that the PR fixes. There's a lot of discussion both ways, but it was decided that this is the best solution.

If we make it a hard error again, we can't improve the rules in the compiler. Think about this case:

match some_u8 {
    0...100 => {},
    101...255 => {},
    _ => {}, // today this branch is necessary, due to a limitation
}

In the future, that branch will turn into a warning, because it's statically known to be unreachable, but it's not an error right now. If we made it an error, we'd break perfectly valid code in the future. Making the compiler smarter should not result in previously working user code breaking. Have a look at #1229 where this exact same issue was discussed. If we make the compiler smarter, it might detect that you wrote 42 / (false as i32), which will panic at runtime. The reasoning for only producing a warning and not an error has been discussed there and applies to this RFC, too.

missed something to match = error,
match something twice = error.

As it has been said before, if there's no clear winner, let the users decide. And you can already decide with deny(unreachable_pattern). If you want this project wide or even user wide, I suggest a rust.toml similar to clippy.toml, which will allow you to turn lints on and off on a project or user wide basis.

[canndrew]

@Dushistov it will always need to be possible to turn this error off for the sake of macro-generated code. It would be nice if we could make it error-by-default again though - and force people to use #[allow(unreachable_patterns)] if they need to allow it - but there are still improvements to the pattern-matching code that need to be rolled-out first. @oli-obk gave one example, though the original motivating example for the change was to support matches that involve uninhabited types like ! and enum Void {}.

[kaukas]

Please excuse my superficial knowledge.

In my opinion this seems equivalent to the unused imports error in Go. It might be great if you are about to release your changes but really annoying if you're just poking around. Unlike Go, Rust does have warnings so perhaps this should stay as a warning.

[retep998]

Having an unreachable pattern is a lot worse than having an unused import, because of Rust's decision to make consts in patterns and bindings in patterns use the same syntax! This means that the behavior of FOO => ... depends on whether FOO is a constant and if someone neglects to pay attention to that warning their code could very easily be doing the wrong thing! An unused import meanwhile does absolutely nothing, so if it is ignored nothing will be broken.

[jjpe]

From my own experience with Rust development, having "unreachable patterns" be a warning is quite useful: Sometimes, during development, I find myself temporarily needing to switch off some branches every now and again. If this were turned into an error again that would become impossible.

Note: om not talking about releasing any code with unreachable code in it; This is merely during development itself.

Also, since it was an error and now is a warning, I think there should be a strong bias against changing it again, especially if there's no revolutionary new reason or use case driving the change. Flip flopping on any feature or bug seems like a very bad idea to me, in general.

[aturon]

Thanks for the RFC! However, as @canndrew points out, there are good reasons to keep this within the lint system, and we hope to move it to error-by-default in the future (which I believe will largely address the aims of this RFC).

As such, I'm going to go ahead and close.