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

pattern binding **** is named the same as one of the variants of the type SHOULD BE ERROR #104966

Closed
zhang-ray opened this issue Nov 27, 2022 · 7 comments · Fixed by #104154
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@zhang-ray
Copy link

Given the following code:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=df522ed9839ada9d5c66830e8de12f7c

#[derive(Debug)]
pub enum BusinessType {
    First,
    Second,
    Third,
}

fn my_tricky_func(business_type: &BusinessType) {
    match &business_type {
        First => {
            println!("First");
        }
        Second => {
            println!("Second");
        }
        Third => {
            println!("Third");
        }
    }
}

fn my_ok_func(business_type: &BusinessType) {
    match &business_type {
        BusinessType::First => {
            println!("First");
        }
        BusinessType::Second => {
            println!("Second");
        }
        BusinessType::Third => {
            println!("Third");
        }
    }
}

fn main() {
    println!("\n\n↓↓↓ trying my_tricky_func");
    my_tricky_func(&BusinessType::Third);
    my_tricky_func(&BusinessType::Second);
    my_tricky_func(&BusinessType::First);

    println!("\n\n↓↓↓ trying my_ok_func");
    my_ok_func(&BusinessType::Third);
    my_ok_func(&BusinessType::Second);
    my_ok_func(&BusinessType::First);
}

The current output is:

Compiling playground v0.0.1 (/playground)
warning[[E0170]](https://doc.rust-lang.org/stable/error-index.html#E0170): pattern binding `First` is named the same as one of the variants of the type `BusinessType`
  --> src/main.rs:10:9
   |
10 |         First => {
   |         ^^^^^ help: to match on the variant, qualify the path: `BusinessType::First`
   |
   = note: `#[warn(bindings_with_variant_name)]` on by default

warning[[E0170]](https://doc.rust-lang.org/stable/error-index.html#E0170): pattern binding `Second` is named the same as one of the variants of the type `BusinessType`
  --> src/main.rs:13:9
   |
13 |         Second => {
   |         ^^^^^^ help: to match on the variant, qualify the path: `BusinessType::Second`

warning[[E0170]](https://doc.rust-lang.org/stable/error-index.html#E0170): pattern binding `Third` is named the same as one of the variants of the type `BusinessType`
  --> src/main.rs:16:9
   |
16 |         Third => {
   |         ^^^^^ help: to match on the variant, qualify the path: `BusinessType::Third`

warning: unreachable pattern
  --> src/main.rs:13:9
   |
10 |         First => {
   |         ----- matches any value
...
13 |         Second => {
   |         ^^^^^^ unreachable pattern
   |
   = note: `#[warn(unreachable_patterns)]` on by default

warning: unreachable pattern
  --> src/main.rs:16:9
   |
10 |         First => {
   |         ----- matches any value
...
16 |         Third => {
   |         ^^^^^ unreachable pattern

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

warning: unused variable: `Second`
  --> src/main.rs:13:9
   |
13 |         Second => {
   |         ^^^^^^ help: if this is intentional, prefix it with an underscore: `_Second`

warning: unused variable: `Third`
  --> src/main.rs:16:9
   |
16 |         Third => {
   |         ^^^^^ help: if this is intentional, prefix it with an underscore: `_Third`

warning: variable `First` should have a snake case name
  --> src/main.rs:10:9
   |
10 |         First => {
   |         ^^^^^ help: convert the identifier to snake case (notice the capitalization): `first`
   |
   = note: `#[warn(non_snake_case)]` on by default

warning: variable `Second` should have a snake case name
  --> src/main.rs:13:9
   |
13 |         Second => {
   |         ^^^^^^ help: convert the identifier to snake case (notice the capitalization): `second`

warning: variable `Third` should have a snake case name
  --> src/main.rs:16:9
   |
16 |         Third => {
   |         ^^^^^ help: convert the identifier to snake case: `third`

For more information about this error, try `rustc --explain E0170`.
warning: `playground` (bin "playground") generated 11 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 0.60s
     Running `target/debug/playground`

Standard Output



↓↓↓ trying my_tricky_func
First
First
First


↓↓↓ trying my_ok_func
Third
Second
First

Ideally the output should look like:


Compiling playground v0.0.1 (/playground)
error: should be like this:`BusinessType::First`
  --> src/main.rs:10:9
   |
10 |         First => {
   |         ^^^^^ help: to match on the variant, qualify the path: `BusinessType::First`
   |
   = note: `#[warn(bindings_with_variant_name)]` on by default

@zhang-ray zhang-ray added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 27, 2022
@workingjubilee
Copy link
Member

This is technically valid code due to Rust's existing rules. Issuing a true "hard error" is dubious, because it would be arbitrarily blocking some identifiers from shadowing others in the grammar, which is inconsistent with other rules regarding shadowing: namely, that it is permitted in Rust.

However, the relevant lint will already be upgraded to deny-by-default as of #104154

@zhang-ray
Copy link
Author

This is technically valid code due to Rust's existing rules. Issuing a true "hard error" is dubious, because it would be arbitrarily blocking some identifiers from shadowing others in the grammar, which is inconsistent with other rules regarding shadowing: namely, that it is permitted in Rust.

However, the relevant lint will already be upgraded to deny-by-default as of #104154

Hello Jubilee, thank you for your information. But I thought it should be raised to HUGE warning at least. This "feature" of rust is much inconstant with C/C++/Golang rule which would make lots of trouble by programmers from C/C++/Golang... ^_^

@scottmcm scottmcm linked a pull request Nov 27, 2022 that will close this issue
@workingjubilee
Copy link
Member

Hmm. There are many such footguns if you're expecting a 1:1 match of syntax and semantics with another language, unfortunately, as e.g. this is valid Rust:

let x = 5;
let x = 4;
println!("{}", x);

This is invalid C++:

auto x = 5;
auto x = 4;
std::cout << x;

Obviously, we intend to deny the specific case you mention, but it did emit 11 warnings (4 unique)... if the compiler emits 100 warnings, it's technically valid Rust, but that doesn't mean the compiler thought it was a good idea to compile that code. Just that, as far as rustc can tell, it didn't violate memory safety or type soundness. I am curious: is "issuing 11 warnings" somehow not equal to issuing a "huge warning" in your mind?

@Noratrieb
Copy link
Member

But I thought it should be raised to HUGE warning at least

The lint will get turned into deny-by-default (error but you can allow it if you want), which qualifies as a huge warning for me :)

@zhang-ray
Copy link
Author

But I thought it should be raised to HUGE warning at least

The lint will get turned into deny-by-default (error but you can allow it if you want), which qualifies as a huge warning for me :)

I thought deny-by-default is quite suitable for most of cases...

@zhang-ray
Copy link
Author

Hmm. There are many such footguns if you're expecting a 1:1 match of syntax and semantics with another language, unfortunately, as e.g. this is valid Rust:

let x = 5;
let x = 4;
println!("{}", x);

This is invalid C++:

auto x = 5;
auto x = 4;
std::cout << x;

Obviously, we intend to deny the specific case you mention, but it did emit 11 warnings (4 unique)... if the compiler emits 100 warnings, it's technically valid Rust, but that doesn't mean the compiler thought it was a good idea to compile that code. Just that, as far as rustc can tell, it didn't violate memory safety or type soundness. I am curious: is "issuing 11 warnings" somehow not equal to issuing a "huge warning" in your mind?

Hello Jubilee, I know rust is not 1:1 matching indeed. As for this case, I regarding one pare of curly braces behind. What's more, I often use and LOVE this feature of Rust for keeping code clean.

In practice, there are so many warning would be generated and no time to eliminate them... For example

  1. JSON serialization/deserialization struct that members are camel case, which should match to our business need(JSON elements should be camel case);
  2. UEFI (non-OS) projects and OS kernel projects would generate lots of unsafe/compatible warnings at once...

So that, the "issuing 11 warnings" will be concealed by the other many warnings in practice....

@Noratrieb
Copy link
Member

As for the serde struct case, you should use #[serde(rename_all = "camelCase")] on your structs and keep the names snake case in Rust (or alternatively if you don't like that you should at least allow() the warning).
For the unsafe code, you should fix it as your code is probably wrong as the warning indicates. (If you need help doing that you can ask on https://users.rust-lang.org or in the #dark-arts in the Rust Community Discord)

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 T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants