Consider reversing `clippy::redundant_pattern`?

[Coder-256]

Power-users of Rust are certainly aware of the dilemma that in match arms, patterns like foo => ... can either match anything and bind to foo, or match a constant or enum case called foo. This can make code difficult to read, and often causes confusion about which option is actually happening.

Naming conventions don't fully solve the issue. For example, if you're refactoring and you accidentally move a constant named FOO out of scope, all of a sudden FOO => ... will be matched by assigning FOO as a variable, which may lead to confusing errors... or even incorrect code with just two misleading, spurious-looking warnings about naming conventions and an unused variable.

Also: many users commonly try to match something to the value of an existing variable like foo => ... instead of x if x == foo => .... When that happens, very unintuitively the match always succeeds and just replaces/shadows foo with whatever was matched. In fact, this post is inspired by one such victim on Reddit.

Due to this, IMHO it would be a lot clearer if we style-linted against direct binding and suggest that users switch to adding the extra few characters for @ _. It would make the code clearer to read, mitigate the constant-out-of-scope case, and make it easier for new users to figure out what went wrong if they attempt this common mistake. In other words, I'm arguing for reversing the clippy::redundant_pattern lint to suggest changing match arms from foo => ... to foo @ _ => ...; then, only constants and enum cases are meant to use the FOO => ... syntax.

Curious to hear what people think about this idea? Is there an already an RFC for this?

[chrefr]

I don't use clippy normally, but I find the direct binding syntax better. It is more clear, IMHO, and while there are certainly pitfalls with it I think they're more common with beginners (that tend to not invoke clippy, but that is a problem of its own) and once you learn that you're unlikely to fall into this trap. Especially given that this gives warnings (either naming conventions or unreachable pattern) in 99% of the cases. If you never ignore warnings it's very unlikely to sneak in.

That being said, it might be a good idea to have both, at least for beginners (and teach beginners to use clippy more). But I don't see how it will become useful unless it'll be the default.

Anyway, this post sounds like a better fit for internals.rust-lang.org.

[H2CO3]

I also like to use foo @ _ and usually silence any lints resulting from it.

[Yandros]

So, for constants defined outside of fn bodies (99.9% of them), you can also use self::CONSTANT to disambiguate the pattern-matching.

• In the future, using const { CONSTANT } could be another way to achieve it.

Together with the @ _, this leads to ways to disambiguate in either direction.

I've personally mused in the past about a hypothetical language where @ would be mandatory, order-reversed, and using let syntax. I believe this would lead to a language with pattern-matching semantics clearer, especially to those unacquainted with it:

let x = 42;
(let x, let y) = (42, 27); // <- the only case worse than current Rust, imho

match option {
    Some(CONSTANT) => { … },
    Some(let x) => { … },
    None => { … },
}

if option is Some(let x) {
    /* x in scope */
} else {
    // …
}

But back to Rust this requires the more sigily @ and that trailing _ due to grammar, yielding:

let x @ _ = 42;
let (x @ _, y @ _) = (42, 27);

match option {
    Some(self::CONSTANT) => { … },
    Some(x @ _) => { … },
    None => { … },
}

if let Some(x @ _) = option {

And as much as I am "a sucker for" language explicitness, I find this to be a bit too much: in practice the case convention lint —together with, in some cases, the unreachable arm lint— ought to be enough: people can get bitten by refactorings, that's true, but that's the very thing lints are for, as I see it.

• The Reddit example is interesting since it managed to trigger no lint, and definitely one which would have benefited from your mandatory @ _ rule, or my imaginary let-in-patterns syntax, but to be honest, their match was missing a fallback branch, so they were in a two-wrongs kind of case, where indeed lints can start to suffer.

  For instance, if using if let q = c { which seems like it would have better expressed their implicit _ => {} rule, we do get back another lint to help us with it:

  warning: irrefutable `if let` pattern
   --> src/lib.rs:8:8
    |
  8 |     if let q = c {
    |        ^^^^^^^^^
    |
    = note: `#[warn(irrefutable_let_patterns)]` on by default
    = note: this pattern will always match, so the `if let` is useless
    = help: consider replacing the `if let` with a `let`
  

Granted, it's a bit sad that we have to rely on three orthogonal lints (or even more, if using clippy), to catch this pitfall of the language, but at the end of the day, when put together, they do the job, and in exchange, the code can remain more lightweight

---

So my personal rule of thumb is:

• follow case conventions as much as possible, and pay special attention to lints triggering around pattern-matching;

• in the rare instances where case convention is not followed (I do that a lot when writing proc-macro code, to make the quote! invocations order of magnitudes more readable), resort to @ _-disambiguation since the case-convention lint has to be disabled:

danielhenrymantilla/ext-trait.rs/blob/f1dd86b79cd0db3587b43891d4582ea5bc64a197/src/proc_macros/mod.rs#L112-L118

[H2CO3]

@Yandros

I've personally mused in the past about a hypothetical language where @ would be mandatory, order-reversed, and using let syntax

Swift does something similar FYI: associated values of pattern-matched enum variants are extracted using let in exactly that position.

[afetisov]

@Coder-256

Due to this, IMHO it would be a lot clearer if we style-linted against direct binding and suggest that users switch to adding the extra few characters for @ _.

This would be unreasonably verbose if you demand it in fields of enums and structs (which may very well be unnamed tuple fields, thus falling prey to the same errors). If you enforce the lint only for toplevel catch-all pattern, then it would just be inconsistent and confusing, and wouldn't catch most of the possible errors.

For this reason I am against this change. On the other hand, the naming convention and unreachable pattern lints could be improved and taught to recongize this error directly. There may also be some value in adding a lint against creating patterns with the same name as an existing variable, since that could be an attempt to compare the values.

@Coder-256

For example, if you're refactoring and you accidentally move a constant named FOO out of scope, all of a sudden FOO => ... will be matched by assigning FOO as a variable, which may lead to confusing errors... or even incorrect code with just two misleading, spurious-looking warnings about naming conventions and an unused variable.

Yes, you would get a naming convention lint. I think that's enough. That's the reason most people would recommend to keep your code entirely warning-free, even if it looks unnecessary.

Again, the lint could be improved to suggest that you may be missing a visible constant, rather than just say you violate naming conventions.

[scottmcm]

@Yandros

Granted, it's a bit sad that we have to rely on three orthogonal lints (or even more, if using clippy), to catch this pitfall of the language, but at the end of the day, when put together, they do the job, and in exchange, the code can remain more lightweight

One thing I'd like to see here is stronger lints for the more obviously incorrect cases. Yes, you often get like 4 warnings if you do the match wrong, but also none of them are errors.

For example, match a.cmp(b) { Less => ..., Equal => ..., Greater => ... } should give a specific "you forgot to import the variants" error, because those bindings exactly match the variant names. That's never right, so it should get more than a warning.

(And adding lints like that is reversible, so it just takes PRs -- no need for RFCs.)

[Coder-256]

@afetisov

Yes, you would get a naming convention lint. I think that's enough. That's the reason most people would recommend to keep your code entirely warning-free, even if it looks unnecessary.

Again, the lint could be improved to suggest that you may be missing a visible constant, rather than just say you violate naming conventions.

Agreed, currently I imagine most users see the warning about naming conventions and just rename their variable; then the warning disappears but the code is even more incorrect. IMHO the "unused variable" warning can be a bit confusing; perhaps it should have some help text about matching constants vs. binding variables.

@Yandros

if option is Some(let x) {

I actually really really like @Yandros' solution. It hadn't occurred to me that my original solution should technically apply to all let bindings, but this fixes the problem elegantly. I guess the first and main concern that comes to mind is whether this would pass as an RFC, so it might sadly continue to be hypothetical but I'd definitely love to be convinced otherwise! Maybe if you were going to write Rust from scratch again, this would be the smart way to do it.

Anyway, I agree that a good angle of attack for the original problem would be better diagnostics.

[chrefr]

What if we will lint only when the binding name is the same as some variable in scope?

[Coder-256]

@chrefr

What if we will lint only when the binding name is the same as some variable in scope?

In this case be wary of e.g. if let Some(foo) = foo, if let Ok(foo) = foo, etc. which are arguably valid. Maybe add an exception if the original expression is a just an identifier with the same name as the new binding? Or contained somewhere in the expression? I do think this lint is a good idea but it could be annoying in the case of intentional, valid shadowing.