Auto merge of - xFrednet:5552-false-positive-match-single-binding, r=ebroto

Fixing a false positive for the `match_single_binding` lint 

This is a fix for a false positive in the `match_single_binding` lint when using `#[cfg()]` on a branch. It is sadly a bit hacky but maybe the best solution as rust removes the other branch from the AST before we can even validate it. This fix looks at the code snippet itself and returns if it includes another thick arrow `=>` besides the one matching arm we found. This can again cause false negatives if someone has the following code:
```rust
match x {
    // => <-- Causes a false negative
    _ => 1,
}
```

I thought about making the code more complex and maybe validating against other things like the `#[cfg()]` macro but I believe that this is the best solution. This has basically switched the issue from a false positive to a false negative in a very specific case.

I'm happy to make some changes if you have any suggestions 🙃.

---
Fixes 

changelog: Fixed a false positive in the `match_single_binding` lint with `#[cfg()]` macro
This commit is contained in:
bors 2020-12-13 21:28:38 +00:00
commit d9241640e8
4 changed files with 93 additions and 3 deletions

View File

@ -4,8 +4,8 @@ use crate::utils::usage::is_unused;
use crate::utils::{
expr_block, get_arg_name, get_parent_expr, in_macro, indent_of, is_allowed, is_expn_of, is_refutable,
is_type_diagnostic_item, is_wild, match_qpath, match_type, match_var, meets_msrv, multispan_sugg, remove_blocks,
snippet, snippet_block, snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
span_lint_and_then,
snippet, snippet_block, snippet_opt, snippet_with_applicability, span_lint_and_help, span_lint_and_note,
span_lint_and_sugg, span_lint_and_then,
};
use crate::utils::{paths, search_same, SpanlessEq, SpanlessHash};
use if_chain::if_chain;
@ -1237,6 +1237,24 @@ fn check_match_single_binding<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[A
if in_macro(expr.span) || arms.len() != 1 || is_refutable(cx, arms[0].pat) {
return;
}
// HACK:
// This is a hack to deal with arms that are excluded by macros like `#[cfg]`. It is only used here
// to prevent false positives as there is currently no better way to detect if code was excluded by
// a macro. See PR #6435
if_chain! {
if let Some(match_snippet) = snippet_opt(cx, expr.span);
if let Some(arm_snippet) = snippet_opt(cx, arms[0].span);
if let Some(ex_snippet) = snippet_opt(cx, ex.span);
let rest_snippet = match_snippet.replace(&arm_snippet, "").replace(&ex_snippet, "");
if rest_snippet.contains("=>");
then {
// The code it self contains another thick arrow "=>"
// -> Either another arm or a comment
return;
}
}
let matched_vars = ex.span;
let bind_names = arms[0].pat.span;
let match_body = remove_blocks(&arms[0].body);

View File

@ -87,4 +87,32 @@ fn main() {
unwrapped
})
.collect::<Vec<u8>>();
// Ok
let x = 1;
match x {
#[cfg(disabled_feature)]
0 => println!("Disabled branch"),
_ => println!("Enabled branch"),
}
// Lint
let x = 1;
let y = 1;
println!("Single branch");
// Ok
let x = 1;
let y = 1;
match match y {
0 => 1,
_ => 2,
} {
#[cfg(disabled_feature)]
0 => println!("Array index start"),
_ => println!("Not an array index start"),
}
// False negative
let x = 1;
match x {
// =>
_ => println!("Not an array index start"),
}
}

View File

@ -99,4 +99,37 @@ fn main() {
unwrapped => unwrapped,
})
.collect::<Vec<u8>>();
// Ok
let x = 1;
match x {
#[cfg(disabled_feature)]
0 => println!("Disabled branch"),
_ => println!("Enabled branch"),
}
// Lint
let x = 1;
let y = 1;
match match y {
0 => 1,
_ => 2,
} {
_ => println!("Single branch"),
}
// Ok
let x = 1;
let y = 1;
match match y {
0 => 1,
_ => 2,
} {
#[cfg(disabled_feature)]
0 => println!("Array index start"),
_ => println!("Not an array index start"),
}
// False negative
let x = 1;
match x {
// =>
_ => println!("Not an array index start"),
}
}

View File

@ -167,5 +167,16 @@ LL | unwrapped
LL | })
|
error: aborting due to 11 previous errors
error: this match could be replaced by its body itself
--> $DIR/match_single_binding.rs:112:5
|
LL | / match match y {
LL | | 0 => 1,
LL | | _ => 2,
LL | | } {
LL | | _ => println!("Single branch"),
LL | | }
| |_____^ help: consider using the match body instead: `println!("Single branch");`
error: aborting due to 12 previous errors