From this topic (that gradually got wild) it seems useful to introduce a lint about unused capture names. (Code in playground)
Upon seeing an unused capture name, search for similar enum variants, and give a "maybe you meant to match against enum variant SomeVariant instead of capturing?" + "If the name is not needed, consider replacing it with the wildcard _ or prefixing it with _"
I'm just asking, I don't know anything about the compiler source code, so I don't think I'll be the one implementing this. So I'm actually hoping someone else will do this.
See also
There is right now, if you don’t explicitly turn off unused variable warnings with #[allow(unused)]. Comment out the allows at the top and you get this output:
Compiling playground v0.0.1 (/playground)
warning: variant `MAGIC_LAMP` should have an upper camel case name
--> src/main.rs:8:5
|
8 | MAGIC_LAMP,
| ^^^^^^^^^^ help: convert the identifier to upper camel case: `MagicLamp`
|
= note: `#[warn(non_camel_case_types)]` (part of `#[warn(nonstandard_style)]`) on by default
warning: variant `DEATH_DOLL` should have an upper camel case name
--> src/main.rs:9:5
|
9 | DEATH_DOLL,
| ^^^^^^^^^^ help: convert the identifier to upper camel case: `DeathDoll`
warning: variant `SCROLL_OF_EXIT` should have an upper camel case name
--> src/main.rs:10:5
|
10 | SCROLL_OF_EXIT,
| ^^^^^^^^^^^^^^ help: convert the identifier to upper camel case: `ScrollOfExit`
warning: unnecessary parentheses around `match` scrutinee expression
--> src/main.rs:15:22
|
15 | return match (self) {
| ^ ^
|
= note: `#[warn(unused_parens)]` (part of `#[warn(unused)]`) on by default
help: remove these parentheses
|
15 - return match (self) {
15 + return match self {
|
warning: unused variable: `SCROLL_EXIT`
--> src/main.rs:21:13
|
21 | SCROLL_EXIT => "Scroll of Exit",
| ^^^^^^^^^^^
|
= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default
help: you might have meant to pattern match on the similarly named variant `SCROLL_OF_EXIT`
|
21 - SCROLL_EXIT => "Scroll of Exit",
21 + PlayerItemTypes::SCROLL_OF_EXIT => "Scroll of Exit",
|
help: if this is intentional, prefix it with an underscore
|
21 | _SCROLL_EXIT => "Scroll of Exit",
| +
warning: method `GetDisplayName` should have a snake case name
--> src/main.rs:14:12
|
14 | pub fn GetDisplayName(&self) -> &str {
| ^^^^^^^^^^^^^^ help: convert the identifier to snake case: `get_display_name`
|
= note: `#[warn(non_snake_case)]` (part of `#[warn(nonstandard_style)]`) on by default
warning: variable `SCROLL_EXIT` should have a snake case name
--> src/main.rs:21:13
|
21 | SCROLL_EXIT => "Scroll of Exit",
| ^^^^^^^^^^^ help: convert the identifier to snake case: `scroll_exit`
warning: `playground` (bin "playground") generated 7 warnings (run `cargo fix --bin "playground" -p playground` to apply 2 suggestions)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.00s
Running `target/debug/playground`
This was pointed out several times in the linked thread.
1 Like
Well... I think I wasn't clear enough. I think there should be a separate lint for unused capture names so one can write
#[allow(unused)]
#[deny(unused_match_captures)]
I like to use #[allow(unused)] because I don't like to have to be urged by warnings to clean up e.g. alternative solutions when I'm trying out (and possibly comparing) ideas. Furthermore, that lint mark is generated when doc examples are put into playground via the run button. So I personally consider it not a harmful thing, except for allowing the subtle bug of unused captures. And if I want to clean up my code at any time, I can comment out the #[allow(unused)], delete unused code then uncomment it again. So that's why I think a separate lint is handy.
that's terrible practice of using #[allow(unused)].
unused is a lint group, and it's a farly large group, which combines roughly 20 individual lints in current version now.
even I, who am extreme lazy and sloppy for prototyping code, have not used it much, especially in larger scopes such as module or crate levels. instead, I would rather disable individual lints, e.g. I would allow lints like dead_code, unreachable_code, unused_imports, because these are triggerred very often, and, generally, by temporary code.
but rarely do I allow unused_unsafe, unused_must_use, unused_variables, etc. instead, when I create a variable binding or struct field that I think might be used later but not immediately, I simply prefix it with an underscore, which is the standard practice.
1 Like
Get it, so what do you think about having the individual lint I mentioned?
If you absolutely must, disable the group then enable unused_variables again.
But really, the better answer is what people have already said: disable individual things, not the entire group. Maybe do things like #![cfg_attr(test, allow(dead_code))] if you want to be able to cargo test without those warnings, then see them once you cargo run --release, for example.
Also, follow the casing conventions, so you'll still get a warning about FOO => even if you disable all the unused warnings.
At some point if you're just not even trying to follow the advice -- which exists for a reason -- I think it's entirely justifiable for the compiler to stop helping.
TBH even then you'll still get various other warnings like if you match a.cmp(&b) { Less => ... } without having Ordering::Less in scope. For example, remove the use PlayerItemTypes::* from your playground example and you still get
error[E0170]: pattern binding `DEATH_DOLL` is named the same as one of the variants of the type `PlayerItemTypes`
--> src/main.rs:15:13
|
15 | DEATH_DOLL => "Death Doll",
| ^^^^^^^^^^ help: to match on the variant, qualify the path: `PlayerItemTypes::DEATH_DOLL`
|
= note: `#[deny(bindings_with_variant_name)]` on by default
But if you
- import
* and
- don't follow naming conventions and
- disable the naming lints and
- disable the unused lints
then yeah, you don't get a warning. But TBH, ¯\_(ツ)_/¯
1 Like
I think the issue is that you're mistakenly assuming (as seemingly many others do) that the unused lint group can be disabled without any danger to your code. After all, what possible damage could extra, unused code do? And so it's reasonable to be surprised that allow(unused) lets sneaky bugs by. Similar rationale for casing: it's purely aesthetic, right? Disabling purely aesthetic lints shouldn't allow bugs to get away.
But no, dead code is quite often a symptom of otherwise subtle bugs: an accidental early return or panic case, sloppily written match arms, mistyped variables, and so forth. Standardized and consistently varied casing can help loads by preventing mixups between similarly named items of different kinds.
By disabling those lints, you're not just saying "ignore my ugly code". You've really just told the compiler "stop bugging me, I've got this", in a very similar way to unsafe blocks. And just like with unsafe, you forfeit all right to expect the compiler to "just do the right thing". You quite specifically (and against much caution from the community) took that responsibility on your own shoulders, and must now bear the weight of your own mistakes.
1 Like