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

Deny multiple catch all arms #86677

Closed
andrewbanchich opened this issue Jun 27, 2021 · 8 comments
Closed

Deny multiple catch all arms #86677

andrewbanchich opened this issue Jun 27, 2021 · 8 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-patterns Relating to patterns and pattern matching C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@andrewbanchich
Copy link
Contributor

andrewbanchich commented Jun 27, 2021

Currently the compiler only issues warnings for cases like this:

enum Foo {
    A,
    B
}

fn main() {
    let foo = Foo::B;
    
    match foo {
        A => println!("A"),
        B => println!("B"),
    };
}

which prints A since it's interpreting both A and B arms as catch alls.

I can't think of any possible scenario where one could use multiple catch all arms for anything, so it may be beneficial to deny compiling this altogether.

@marmeladema
Copy link
Contributor

I've ended up encountering this error too and most of the time it's because I forgot to specify the enum name. Maybe a help note could be issued if the catch all variable name matches the name of one of the variant because it's most likely a mistake.

@andrewbanchich
Copy link
Contributor Author

andrewbanchich commented Jun 27, 2021

@marmeladema I think many of us have 😊

I was thinking having it be a hard deny instead would prevent issues for those who have disabled warnings (which I've seen happen).

@Aaron1011
Copy link
Member

Rust currently emits several warnings for this code:

warning[E0170]: pattern binding `A` is named the same as one of the variants of the type `Foo`
  --> src/main.rs:10:9
   |
10 |         A => println!("A"),
   |         ^ help: to match on the variant, qualify the path: `Foo::A`
   |
   = note: `#[warn(bindings_with_variant_name)]` on by default

warning[E0170]: pattern binding `B` is named the same as one of the variants of the type `Foo`
  --> src/main.rs:11:9
   |
11 |         B => println!("B"),
   |         ^ help: to match on the variant, qualify the path: `Foo::B`

warning: unreachable pattern
  --> src/main.rs:11:9
   |
10 |         A => println!("A"),
   |         - matches any value
11 |         B => println!("B"),
   |         ^ unreachable pattern
   |
   = note: `#[warn(unreachable_patterns)]` on by default

warning: unused variable: `A`
  --> src/main.rs:10:9
   |
10 |         A => println!("A"),
   |         ^ help: if this is intentional, prefix it with an underscore: `_A`
   |
   = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `B`
  --> src/main.rs:11:9
   |
11 |         B => println!("B"),
   |         ^ help: if this is intentional, prefix it with an underscore: `_B`

warning: variant is never constructed: `A`
 --> src/main.rs:2:5
  |
2 |     A,
  |     ^
  |
  = note: `#[warn(dead_code)]` on by default

warning: variable `A` should have a snake case name
  --> src/main.rs:10:9
   |
10 |         A => println!("A"),
   |         ^ help: convert the identifier to snake case: `a`
   |
   = note: `#[warn(non_snake_case)]` on by default

warning: variable `B` should have a snake case name
  --> src/main.rs:11:9
   |
11 |         B => println!("B"),
   |         ^ help: convert the identifier to snake case: `b`

warning: 8 warnings emitted

One reason for allowing this is to support macros - a macro might build up a match from different pieces of user input, and consequently end up with multiple catch-all arms.

I was thinking having it be a hard deny instead would prevent issues for those who have disabled warnings (which I've seen happen).

Disabling all warnings is a pretty bad idea, as demonstrated by this issue 😄 . If you run across any projects that have blanket suppressions of a warning, I'd encourage them to open an issue to help us improve the usefulness of that warning.

@andrewbanchich
Copy link
Contributor Author

I agree disabling warnings is bad. I don't know enough about the macro use case to weigh in, but if that's a legit case for this feel free to close the issue.

@inquisitivecrystal
Copy link
Contributor

I feel like this could be a good case for a deny by default lint? It's something that you could want to do in macros, but the chance that you actually want to do it is so low that it might be better if rustc assumed you didn't.

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Jun 28, 2021

@rustbot label A-diagnostics A-lint A-patterns C-enhancement T-compiler T-lang

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-patterns Relating to patterns and pattern matching C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Jun 28, 2021
@leonardo-m
Copy link

I'm glad that Reddit thread gave birth to this issue :)

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 28, 2021
@Mark-Simulacrum
Copy link
Member

The lint is now deny by default as of #104154, so I think we can probably close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-patterns Relating to patterns and pattern matching C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants