From 7f822e742d96d250355ada92c52c5a1b755f73c1 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 25 Sep 2019 05:19:09 -0700 Subject: [PATCH] needless_borrowed_ref: fix false positive, make rustfixable --- clippy_lints/src/needless_borrowed_ref.rs | 17 +++++++-- tests/ui/needless_borrowed_ref.fixed | 45 +++++++++++++++++++++++ tests/ui/needless_borrowed_ref.rs | 2 + tests/ui/needless_borrowed_ref.stderr | 22 +---------- 4 files changed, 62 insertions(+), 24 deletions(-) create mode 100644 tests/ui/needless_borrowed_ref.fixed diff --git a/clippy_lints/src/needless_borrowed_ref.rs b/clippy_lints/src/needless_borrowed_ref.rs index 714c746f68d..9afa67dadd1 100644 --- a/clippy_lints/src/needless_borrowed_ref.rs +++ b/clippy_lints/src/needless_borrowed_ref.rs @@ -2,9 +2,9 @@ //! //! This lint is **warn** by default -use crate::utils::{snippet, span_lint_and_then}; +use crate::utils::{snippet_with_applicability, span_lint_and_then}; use if_chain::if_chain; -use rustc::hir::{BindingAnnotation, MutImmutable, Pat, PatKind}; +use rustc::hir::{BindingAnnotation, MutImmutable, Node, Pat, PatKind}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_errors::Applicability; @@ -65,16 +65,25 @@ fn check_pat(&mut self, cx: &LateContext<'a, 'tcx>, pat: &'tcx Pat) { // Check sub_pat got a `ref` keyword (excluding `ref mut`). if let PatKind::Binding(BindingAnnotation::Ref, .., spanned_name, _) = sub_pat.node; + let parent_id = cx.tcx.hir().get_parent_node(pat.hir_id); + if let Some(parent_node) = cx.tcx.hir().find(parent_id); then { + // do not recurse within patterns, as they may have other references + // XXXManishearth we can relax this constraint if we only check patterns + // with a single ref pattern inside them + if let Node::Pat(_) = parent_node { + return; + } + let mut applicability = Applicability::MachineApplicable; span_lint_and_then(cx, NEEDLESS_BORROWED_REFERENCE, pat.span, "this pattern takes a reference on something that is being de-referenced", |db| { - let hint = snippet(cx, spanned_name.span, "..").into_owned(); + let hint = snippet_with_applicability(cx, spanned_name.span, "..", &mut applicability).into_owned(); db.span_suggestion( pat.span, "try removing the `&ref` part and just keep", hint, - Applicability::MachineApplicable, // snippet + applicability, ); }); } diff --git a/tests/ui/needless_borrowed_ref.fixed b/tests/ui/needless_borrowed_ref.fixed new file mode 100644 index 00000000000..a0937a2c5f6 --- /dev/null +++ b/tests/ui/needless_borrowed_ref.fixed @@ -0,0 +1,45 @@ +// run-rustfix + +#[warn(clippy::needless_borrowed_reference)] +#[allow(unused_variables)] +fn main() { + let mut v = Vec::::new(); + let _ = v.iter_mut().filter(|a| a.is_empty()); + // ^ should be linted + + let var = 3; + let thingy = Some(&var); + if let Some(&ref v) = thingy { + // ^ should be linted + } + + let mut var2 = 5; + let thingy2 = Some(&mut var2); + if let Some(&mut ref mut v) = thingy2 { + // ^ should **not** be linted + // v is borrowed as mutable. + *v = 10; + } + if let Some(&mut ref v) = thingy2 { + // ^ should **not** be linted + // here, v is borrowed as immutable. + // can't do that: + //*v = 15; + } +} + +#[allow(dead_code)] +enum Animal { + Cat(u64), + Dog(u64), +} + +#[allow(unused_variables)] +#[allow(dead_code)] +fn foo(a: &Animal, b: &Animal) { + match (a, b) { + (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // lifetime mismatch error if there is no '&ref' + // ^ and ^ should **not** be linted + (&Animal::Dog(ref a), &Animal::Dog(_)) => (), // ^ should **not** be linted + } +} diff --git a/tests/ui/needless_borrowed_ref.rs b/tests/ui/needless_borrowed_ref.rs index c76f4de9b07..500ac448f0d 100644 --- a/tests/ui/needless_borrowed_ref.rs +++ b/tests/ui/needless_borrowed_ref.rs @@ -1,3 +1,5 @@ +// run-rustfix + #[warn(clippy::needless_borrowed_reference)] #[allow(unused_variables)] fn main() { diff --git a/tests/ui/needless_borrowed_ref.stderr b/tests/ui/needless_borrowed_ref.stderr index 1b8067f1d6d..0a5cfb3db0b 100644 --- a/tests/ui/needless_borrowed_ref.stderr +++ b/tests/ui/needless_borrowed_ref.stderr @@ -1,28 +1,10 @@ error: this pattern takes a reference on something that is being de-referenced - --> $DIR/needless_borrowed_ref.rs:5:34 + --> $DIR/needless_borrowed_ref.rs:7:34 | LL | let _ = v.iter_mut().filter(|&ref a| a.is_empty()); | ^^^^^^ help: try removing the `&ref` part and just keep: `a` | = note: `-D clippy::needless-borrowed-reference` implied by `-D warnings` -error: this pattern takes a reference on something that is being de-referenced - --> $DIR/needless_borrowed_ref.rs:10:17 - | -LL | if let Some(&ref v) = thingy { - | ^^^^^^ help: try removing the `&ref` part and just keep: `v` - -error: this pattern takes a reference on something that is being de-referenced - --> $DIR/needless_borrowed_ref.rs:39:27 - | -LL | (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // lifetime mismatch error if there is no '&ref' - | ^^^^^^ help: try removing the `&ref` part and just keep: `k` - -error: this pattern takes a reference on something that is being de-referenced - --> $DIR/needless_borrowed_ref.rs:39:38 - | -LL | (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // lifetime mismatch error if there is no '&ref' - | ^^^^^^ help: try removing the `&ref` part and just keep: `k` - -error: aborting due to 4 previous errors +error: aborting due to previous error