mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

lint nested patterns and slice patterns in needless_borrowed_reference

Browse Source
This commit is contained in:
Alex Macleod 2022-10-01 21:54:22 +00:00
parent 64243c6f99
commit 52a68dc097
8 changed files with 267 additions and 89 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
clippy_lints/src/manual_clamp.rs
View File

@ -324,7 +324,7 @@ fn check<'tcx>(
outer_arg: &'tcx Expr<'tcx>,
span: Span,
) -> Option<ClampSuggestion<'tcx>> {
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 check<'tcx>(
/// # ;
/// ```
fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<ClampSuggestion<'tcx>> {
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

4
clippy_lints/src/map_unit_fn.rs
View File

@ -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 {

2
clippy_lints/src/methods/manual_ok_or.rs
View File

@ -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 }
}

119
clippy_lints/src/needless_borrowed_ref.rs
View File

@ -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 @@
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<String>, b: &Option<String>) {
/// match (a, b) {
/// (None, &ref c) | (&ref c, None) => (),
/// (&Some(ref c), _) => (),
/// };
/// }
/// ```
///
/// ### Example
/// ```rust
/// let mut v = Vec::<String>::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::<String>::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 @@ fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {
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 {
if matches!(pat.kind, PatKind::Or(_)) {
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,
);
});
}
// 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,
);
},
);
}
},
_ => {},
}
}
}

18
src/docs/needless_borrowed_reference.txt
View File

@ -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<String>, b: &Option<String>) {
match (a, b) {
(None, &ref c) | (&ref c, None) => (),
(&Some(ref c), _) => (),
};
}
```
### Example
```
let mut v = Vec::<String>::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::<String>::new();
v.iter_mut().filter(|a| a.is_empty());
if let [first, second] = v.as_slice() {}
```

41
tests/ui/needless_borrowed_ref.fixed
View File

@ -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<u8>) {
let mut v = Vec::<String>::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<u8>) {
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
}

41
tests/ui/needless_borrowed_ref.rs
View File

@ -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<u8>) {
let mut v = Vec::<String>::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<u8>) {
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
}

121
tests/ui/needless_borrowed_ref.stderr
View File

@ -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