From 52a68dc097cc8064a7464598bca959f2706aab18 Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Sat, 1 Oct 2022 21:54:22 +0000 Subject: [PATCH] lint nested patterns and slice patterns in `needless_borrowed_reference` --- clippy_lints/src/manual_clamp.rs | 6 +- clippy_lints/src/map_unit_fn.rs | 4 +- clippy_lints/src/methods/manual_ok_or.rs | 2 +- clippy_lints/src/needless_borrowed_ref.rs | 123 ++++++++++++++-------- src/docs/needless_borrowed_reference.txt | 18 +--- tests/ui/needless_borrowed_ref.fixed | 41 ++++++-- tests/ui/needless_borrowed_ref.rs | 41 ++++++-- tests/ui/needless_borrowed_ref.stderr | 121 ++++++++++++++++++++- 8 files changed, 267 insertions(+), 89 deletions(-) diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs index ac5c24ee604..ece4df95505 100644 --- a/clippy_lints/src/manual_clamp.rs +++ b/clippy_lints/src/manual_clamp.rs @@ -324,7 +324,7 @@ fn is_call_max_min_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) outer_arg: &'tcx Expr<'tcx>, span: Span, ) -> Option> { - if let ExprKind::Call(inner_fn, &[ref first, ref second]) = &inner_call.kind + if let ExprKind::Call(inner_fn, [first, second]) = &inner_call.kind && let Some(inner_seg) = segment(cx, inner_fn) && let Some(outer_seg) = segment(cx, outer_fn) { @@ -377,9 +377,7 @@ fn is_call_max_min_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) /// # ; /// ``` fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { - if let ExprKind::Match(value, &[ref first_arm, ref second_arm, ref last_arm], rustc_hir::MatchSource::Normal) = - &expr.kind - { + if let ExprKind::Match(value, [first_arm, second_arm, last_arm], rustc_hir::MatchSource::Normal) = &expr.kind { // Find possible min/max branches let minmax_values = |a: &'tcx Arm<'tcx>| { if let PatKind::Binding(_, var_hir_id, _, None) = &a.pat.kind diff --git a/clippy_lints/src/map_unit_fn.rs b/clippy_lints/src/map_unit_fn.rs index df5684541e9..32da37a862d 100644 --- a/clippy_lints/src/map_unit_fn.rs +++ b/clippy_lints/src/map_unit_fn.rs @@ -131,12 +131,12 @@ fn reduce_unit_expression<'a>(cx: &LateContext<'_>, expr: &'a hir::Expr<'_>) -> }, hir::ExprKind::Block(block, _) => { match (block.stmts, block.expr.as_ref()) { - (&[], Some(inner_expr)) => { + ([], Some(inner_expr)) => { // If block only contains an expression, // reduce `{ X }` to `X` reduce_unit_expression(cx, inner_expr) }, - (&[ref inner_stmt], None) => { + ([inner_stmt], None) => { // If block only contains statements, // reduce `{ X; }` to `X` or `X;` match inner_stmt.kind { diff --git a/clippy_lints/src/methods/manual_ok_or.rs b/clippy_lints/src/methods/manual_ok_or.rs index c5c0ace7729..d9fb4382181 100644 --- a/clippy_lints/src/methods/manual_ok_or.rs +++ b/clippy_lints/src/methods/manual_ok_or.rs @@ -55,7 +55,7 @@ fn is_ok_wrapping(cx: &LateContext<'_>, map_expr: &Expr<'_>) -> bool { if let ExprKind::Closure(&Closure { body, .. }) = map_expr.kind; let body = cx.tcx.hir().body(body); if let PatKind::Binding(_, param_id, ..) = body.params[0].pat.kind; - if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, &[ref ok_arg]) = body.value.kind; + if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, [ok_arg]) = body.value.kind; if is_lang_ctor(cx, ok_path, ResultOk); then { path_to_local_id(ok_arg, param_id) } else { false } } diff --git a/clippy_lints/src/needless_borrowed_ref.rs b/clippy_lints/src/needless_borrowed_ref.rs index b8855e5adbf..10c3ff026b6 100644 --- a/clippy_lints/src/needless_borrowed_ref.rs +++ b/clippy_lints/src/needless_borrowed_ref.rs @@ -1,6 +1,4 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::snippet_with_applicability; -use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{BindingAnnotation, Mutability, Node, Pat, PatKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -8,36 +6,26 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { /// ### What it does - /// Checks for bindings that destructure a reference and borrow the inner + /// Checks for bindings that needlessly destructure a reference and borrow the inner /// value with `&ref`. /// /// ### Why is this bad? /// This pattern has no effect in almost all cases. /// - /// ### Known problems - /// In some cases, `&ref` is needed to avoid a lifetime mismatch error. - /// Example: - /// ```rust - /// fn foo(a: &Option, b: &Option) { - /// match (a, b) { - /// (None, &ref c) | (&ref c, None) => (), - /// (&Some(ref c), _) => (), - /// }; - /// } - /// ``` - /// /// ### Example /// ```rust /// let mut v = Vec::::new(); - /// # #[allow(unused)] /// v.iter_mut().filter(|&ref a| a.is_empty()); + /// + /// if let &[ref first, ref second] = v.as_slice() {} /// ``` /// /// Use instead: /// ```rust /// let mut v = Vec::::new(); - /// # #[allow(unused)] /// v.iter_mut().filter(|a| a.is_empty()); + /// + /// if let [first, second] = v.as_slice() {} /// ``` #[clippy::version = "pre 1.29.0"] pub NEEDLESS_BORROWED_REFERENCE, @@ -54,34 +42,83 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef { return; } - if_chain! { - // Only lint immutable refs, because `&mut ref T` may be useful. - if let PatKind::Ref(sub_pat, Mutability::Not) = pat.kind; + // Do not lint patterns that are part of an OR `|` pattern, the binding mode must match in all arms + for (_, node) in cx.tcx.hir().parent_iter(pat.hir_id) { + let Node::Pat(pat) = node else { break }; - // Check sub_pat got a `ref` keyword (excluding `ref mut`). - if let PatKind::Binding(BindingAnnotation::REF, .., spanned_name, _) = sub_pat.kind; - 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", - |diag| { - let hint = snippet_with_applicability(cx, spanned_name.span, "..", &mut applicability).into_owned(); - diag.span_suggestion( - pat.span, - "try removing the `&ref` part and just keep", - hint, - applicability, - ); - }); + if matches!(pat.kind, PatKind::Or(_)) { + return; } } + + // Only lint immutable refs, because `&mut ref T` may be useful. + let PatKind::Ref(sub_pat, Mutability::Not) = pat.kind else { return }; + + match sub_pat.kind { + // Check sub_pat got a `ref` keyword (excluding `ref mut`). + PatKind::Binding(BindingAnnotation::REF, _, ident, None) => { + span_lint_and_then( + cx, + NEEDLESS_BORROWED_REFERENCE, + pat.span, + "this pattern takes a reference on something that is being dereferenced", + |diag| { + // `&ref ident` + // ^^^^^ + let span = pat.span.until(ident.span); + diag.span_suggestion_verbose( + span, + "try removing the `&ref` part", + String::new(), + Applicability::MachineApplicable, + ); + }, + ); + }, + // Slices where each element is `ref`: `&[ref a, ref b, ..., ref z]` + PatKind::Slice( + before, + None + | Some(Pat { + kind: PatKind::Wild, .. + }), + after, + ) => { + let mut suggestions = Vec::new(); + + for element_pat in itertools::chain(before, after) { + if let PatKind::Binding(BindingAnnotation::REF, _, ident, None) = element_pat.kind { + // `&[..., ref ident, ...]` + // ^^^^ + let span = element_pat.span.until(ident.span); + suggestions.push((span, String::new())); + } else { + return; + } + } + + if !suggestions.is_empty() { + span_lint_and_then( + cx, + NEEDLESS_BORROWED_REFERENCE, + pat.span, + "dereferencing a slice pattern where every element takes a reference", + |diag| { + // `&[...]` + // ^ + let span = pat.span.until(sub_pat.span); + suggestions.push((span, String::new())); + + diag.multipart_suggestion( + "try removing the `&` and `ref` parts", + suggestions, + Applicability::MachineApplicable, + ); + }, + ); + } + }, + _ => {}, + } } } diff --git a/src/docs/needless_borrowed_reference.txt b/src/docs/needless_borrowed_reference.txt index 55faa0cf571..152459ba1c9 100644 --- a/src/docs/needless_borrowed_reference.txt +++ b/src/docs/needless_borrowed_reference.txt @@ -1,30 +1,22 @@ ### What it does -Checks for bindings that destructure a reference and borrow the inner +Checks for bindings that needlessly destructure a reference and borrow the inner value with `&ref`. ### Why is this bad? This pattern has no effect in almost all cases. -### Known problems -In some cases, `&ref` is needed to avoid a lifetime mismatch error. -Example: -``` -fn foo(a: &Option, b: &Option) { - match (a, b) { - (None, &ref c) | (&ref c, None) => (), - (&Some(ref c), _) => (), - }; -} -``` - ### Example ``` let mut v = Vec::::new(); v.iter_mut().filter(|&ref a| a.is_empty()); + +if let &[ref first, ref second] = v.as_slice() {} ``` Use instead: ``` let mut v = Vec::::new(); v.iter_mut().filter(|a| a.is_empty()); + +if let [first, second] = v.as_slice() {} ``` \ No newline at end of file diff --git a/tests/ui/needless_borrowed_ref.fixed b/tests/ui/needless_borrowed_ref.fixed index a0937a2c5f6..bcb4eb2dd48 100644 --- a/tests/ui/needless_borrowed_ref.fixed +++ b/tests/ui/needless_borrowed_ref.fixed @@ -1,17 +1,38 @@ // run-rustfix -#[warn(clippy::needless_borrowed_reference)] -#[allow(unused_variables)] -fn main() { +#![warn(clippy::needless_borrowed_reference)] +#![allow(unused, clippy::needless_borrow)] + +fn main() {} + +fn should_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec) { 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 - } + if let Some(v) = thingy {} + + if let &[a, ref b] = slice_of_refs {} + + let [a, ..] = &array; + let [a, b, ..] = &array; + + if let [a, b] = slice {} + if let [a, b] = &vec[..] {} + + if let [a, b, ..] = slice {} + if let [a, .., b] = slice {} + if let [.., a, b] = slice {} +} + +fn should_not_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec) { + if let [ref a] = slice {} + if let &[ref a, b] = slice {} + if let &[ref a, .., b] = slice {} + + // must not be removed as variables must be bound consistently across | patterns + if let (&[ref a], _) | ([], ref a) = (slice_of_refs, &1u8) {} let mut var2 = 5; let thingy2 = Some(&mut var2); @@ -28,17 +49,15 @@ fn main() { } } -#[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' + // lifetime mismatch error if there is no '&ref' before `feature(nll)` stabilization in 1.63 + (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // ^ 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 500ac448f0d..f6de1a6d83d 100644 --- a/tests/ui/needless_borrowed_ref.rs +++ b/tests/ui/needless_borrowed_ref.rs @@ -1,17 +1,38 @@ // run-rustfix -#[warn(clippy::needless_borrowed_reference)] -#[allow(unused_variables)] -fn main() { +#![warn(clippy::needless_borrowed_reference)] +#![allow(unused, clippy::needless_borrow)] + +fn main() {} + +fn should_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec) { let mut v = Vec::::new(); let _ = v.iter_mut().filter(|&ref a| a.is_empty()); - // ^ should be linted let var = 3; let thingy = Some(&var); - if let Some(&ref v) = thingy { - // ^ should be linted - } + if let Some(&ref v) = thingy {} + + if let &[&ref a, ref b] = slice_of_refs {} + + let &[ref a, ..] = &array; + let &[ref a, ref b, ..] = &array; + + if let &[ref a, ref b] = slice {} + if let &[ref a, ref b] = &vec[..] {} + + if let &[ref a, ref b, ..] = slice {} + if let &[ref a, .., ref b] = slice {} + if let &[.., ref a, ref b] = slice {} +} + +fn should_not_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec) { + if let [ref a] = slice {} + if let &[ref a, b] = slice {} + if let &[ref a, .., b] = slice {} + + // must not be removed as variables must be bound consistently across | patterns + if let (&[ref a], _) | ([], ref a) = (slice_of_refs, &1u8) {} let mut var2 = 5; let thingy2 = Some(&mut var2); @@ -28,17 +49,15 @@ fn main() { } } -#[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' + // lifetime mismatch error if there is no '&ref' before `feature(nll)` stabilization in 1.63 + (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // ^ and ^ should **not** be linted (&Animal::Dog(ref a), &Animal::Dog(_)) => (), // ^ should **not** be linted } diff --git a/tests/ui/needless_borrowed_ref.stderr b/tests/ui/needless_borrowed_ref.stderr index 0a5cfb3db0b..7453542e673 100644 --- a/tests/ui/needless_borrowed_ref.stderr +++ b/tests/ui/needless_borrowed_ref.stderr @@ -1,10 +1,123 @@ -error: this pattern takes a reference on something that is being de-referenced - --> $DIR/needless_borrowed_ref.rs:7:34 +error: this pattern takes a reference on something that is being dereferenced + --> $DIR/needless_borrowed_ref.rs:10: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` +help: try removing the `&ref` part + | +LL - let _ = v.iter_mut().filter(|&ref a| a.is_empty()); +LL + let _ = v.iter_mut().filter(|a| a.is_empty()); + | -error: aborting due to previous error +error: this pattern takes a reference on something that is being dereferenced + --> $DIR/needless_borrowed_ref.rs:14:17 + | +LL | if let Some(&ref v) = thingy {} + | ^^^^^^ + | +help: try removing the `&ref` part + | +LL - if let Some(&ref v) = thingy {} +LL + if let Some(v) = thingy {} + | + +error: this pattern takes a reference on something that is being dereferenced + --> $DIR/needless_borrowed_ref.rs:16:14 + | +LL | if let &[&ref a, ref b] = slice_of_refs {} + | ^^^^^^ + | +help: try removing the `&ref` part + | +LL - if let &[&ref a, ref b] = slice_of_refs {} +LL + if let &[a, ref b] = slice_of_refs {} + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:18:9 + | +LL | let &[ref a, ..] = &array; + | ^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - let &[ref a, ..] = &array; +LL + let [a, ..] = &array; + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:19:9 + | +LL | let &[ref a, ref b, ..] = &array; + | ^^^^^^^^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - let &[ref a, ref b, ..] = &array; +LL + let [a, b, ..] = &array; + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:21:12 + | +LL | if let &[ref a, ref b] = slice {} + | ^^^^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - if let &[ref a, ref b] = slice {} +LL + if let [a, b] = slice {} + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:22:12 + | +LL | if let &[ref a, ref b] = &vec[..] {} + | ^^^^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - if let &[ref a, ref b] = &vec[..] {} +LL + if let [a, b] = &vec[..] {} + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:24:12 + | +LL | if let &[ref a, ref b, ..] = slice {} + | ^^^^^^^^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - if let &[ref a, ref b, ..] = slice {} +LL + if let [a, b, ..] = slice {} + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:25:12 + | +LL | if let &[ref a, .., ref b] = slice {} + | ^^^^^^^^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - if let &[ref a, .., ref b] = slice {} +LL + if let [a, .., b] = slice {} + | + +error: dereferencing a slice pattern where every element takes a reference + --> $DIR/needless_borrowed_ref.rs:26:12 + | +LL | if let &[.., ref a, ref b] = slice {} + | ^^^^^^^^^^^^^^^^^^^ + | +help: try removing the `&` and `ref` parts + | +LL - if let &[.., ref a, ref b] = slice {} +LL + if let [.., a, b] = slice {} + | + +error: aborting due to 10 previous errors