From fb484539d200873c81c715f7c833bd24bf8bdab2 Mon Sep 17 00:00:00 2001 From: Elliot Bobrow Date: Wed, 6 Apr 2022 09:28:49 -0700 Subject: [PATCH] add `manual_find` lint for function return case --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_complexity.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/loops/manual_find.rs | 189 ++++++++++++++++++++ clippy_lints/src/loops/mod.rs | 34 ++++ clippy_lints/src/non_expressive_names.rs | 10 +- tests/ui/manual_find.fixed | 112 ++++++++++++ tests/ui/manual_find.rs | 162 +++++++++++++++++ tests/ui/manual_find.stderr | 114 ++++++++++++ 10 files changed, 619 insertions(+), 6 deletions(-) create mode 100644 clippy_lints/src/loops/manual_find.rs create mode 100644 tests/ui/manual_find.fixed create mode 100644 tests/ui/manual_find.rs create mode 100644 tests/ui/manual_find.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index fd288285532e..0011f50da20b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3384,6 +3384,7 @@ Released 2018-09-13 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map +[`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map [`manual_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 132a46626762..844008254a8d 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -109,6 +109,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(loops::FOR_KV_MAP), LintId::of(loops::FOR_LOOPS_OVER_FALLIBLES), LintId::of(loops::ITER_NEXT_LOOP), + LintId::of(loops::MANUAL_FIND), LintId::of(loops::MANUAL_FLATTEN), LintId::of(loops::MANUAL_MEMCPY), LintId::of(loops::MISSING_SPIN_LOOP), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index a2ce69065f94..64f5b2df0043 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -21,6 +21,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(lifetimes::EXTRA_UNUSED_LIFETIMES), LintId::of(lifetimes::NEEDLESS_LIFETIMES), LintId::of(loops::EXPLICIT_COUNTER_LOOP), + LintId::of(loops::MANUAL_FIND), LintId::of(loops::MANUAL_FLATTEN), LintId::of(loops::SINGLE_ELEMENT_LOOP), LintId::of(loops::WHILE_LET_LOOP), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 21f1ef562b5a..d5ddb95d25f8 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -222,6 +222,7 @@ store.register_lints(&[ loops::FOR_KV_MAP, loops::FOR_LOOPS_OVER_FALLIBLES, loops::ITER_NEXT_LOOP, + loops::MANUAL_FIND, loops::MANUAL_FLATTEN, loops::MANUAL_MEMCPY, loops::MISSING_SPIN_LOOP, diff --git a/clippy_lints/src/loops/manual_find.rs b/clippy_lints/src/loops/manual_find.rs new file mode 100644 index 000000000000..5ad1aee001dc --- /dev/null +++ b/clippy_lints/src/loops/manual_find.rs @@ -0,0 +1,189 @@ +use super::utils::make_iterator_snippet; +use super::MANUAL_FIND; +use crate::rustc_hir::intravisit::Visitor; +use clippy_utils::{ + diagnostics::span_lint_and_sugg, + higher, peel_blocks_with_stmt, + source::snippet_with_applicability, + ty::is_type_diagnostic_item, + visitors::{expr_visitor, find_all_ret_expressions}, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{BindingAnnotation, Expr, ExprKind, ItemKind, Node, Pat, PatKind, QPath, StmtKind}; +use rustc_lint::LateContext; +use rustc_span::source_map::Span; +use rustc_span::sym; +use rustc_span::symbol::Ident; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + arg: &'tcx Expr<'_>, + body: &'tcx Expr<'_>, + span: Span, +) { + let inner_expr = peel_blocks_with_stmt(body); + if let Some(higher::If { + cond, + then, + r#else: None, + }) = higher::If::hir(inner_expr) + { + let mut pat_expr = None; + let mut ident_collector = PatIdentCollector::default(); + ident_collector.visit_pat(pat); + if let [ident] = &ident_collector.idents[..] { + if ident_in_expr(cx, *ident, cond) { + pat_expr = Some(*ident); + } + } + // Check for the specific case that the result is returned and optimize suggestion for that (more + // cases can be added later) + if_chain! { + if let Some(ident) = pat_expr; + if let ExprKind::Block(block, _) = then.kind; + if let [stmt] = block.stmts; // For now, only deal with the case w/out side effects/mapping + if let StmtKind::Semi(semi) = stmt.kind; + if let ExprKind::Ret(Some(ret_value)) = semi.kind; + let ret_ty = cx.typeck_results().expr_ty(ret_value); + if is_type_diagnostic_item(cx, ret_ty, sym::Option); + if let ExprKind::Call(_, [inner_ret]) = ret_value.kind; + if let Some(i) = ident_of(inner_ret); + if i == ident; + if let Node::Item(parent_fn) + = cx.tcx.hir().get_by_def_id(cx.tcx.hir().get_parent_item(body.hir_id)); + if let ItemKind::Fn(_, _, body_id) = parent_fn.kind; + let parent_fn = &cx.tcx.hir().body(body_id).value; + let ret_exprs = collect_ret_exprs(cx, parent_fn); + if ret_exprs.len() == 2; + // Some(binding) + if let ExprKind::Call(_, [binding]) = &ret_exprs[0].kind; + // None + if let ExprKind::Path(..) = &ret_exprs[1].kind; + then { + let mut applicability = Applicability::MachineApplicable; + let mut snippet = make_iterator_snippet(cx, arg, &mut applicability); + let is_ref_to_binding = + matches!(pat.kind, PatKind::Ref(inner, _) if matches!(inner.kind, PatKind::Binding(..))); + if !(matches!(pat.kind, PatKind::Binding(..)) || is_ref_to_binding) { + snippet.push_str( + &format!( + ".map(|{}| {})", + snippet_with_applicability(cx, pat.span, "..", &mut applicability), + snippet_with_applicability(cx, binding.span, "..", &mut applicability), + )[..], + ); + } + let mut borrows = 0; + match cond.kind { + ExprKind::Binary(_, l, r) => { + for side in [l, r] { + if !matches!( + side.kind, + ExprKind::MethodCall(_, [caller, ..], _) if ident_in_expr(cx, ident, caller) + ) && ident_in_expr(cx, ident, side) + { + borrows = 1; + } + } + }, + ExprKind::Call(..) => borrows = 1, + ExprKind::MethodCall(_, [caller, ..], _) => { + if !ident_in_expr(cx, ident, caller) { + borrows = 1; + } + }, + _ => {}, + } + if is_ref_to_binding { + borrows += 1; + } + snippet.push_str( + &format!( + ".find(|{}{}| {})", + "&".repeat(borrows), + snippet_with_applicability(cx, binding.span, "..", &mut applicability), + snippet_with_applicability(cx, cond.span, "..", &mut applicability), + )[..], + ); + if is_ref_to_binding { + snippet.push_str(".copied()"); + } + span_lint_and_sugg( + cx, + MANUAL_FIND, + span.with_hi(ret_exprs[1].span.hi()), + "manual implementation of Iterator::find", + "replace with an iterator", + snippet, + applicability, + ); + } + } + } +} + +fn ident_of(expr: &Expr<'_>) -> Option { + if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind { + if let [seg, ..] = path.segments { + return Some(seg.ident); + } + } + None +} + +fn ident_in_expr<'tcx>(cx: &LateContext<'tcx>, ident: Ident, expr: &'tcx Expr<'_>) -> bool { + let mut contains = false; + expr_visitor(cx, |e| { + contains |= ident_of(e).map_or(false, |i| i == ident); + !contains + }) + .visit_expr(expr); + contains +} + +#[derive(Default)] +struct PatIdentCollector { + idents: Vec, +} + +impl PatIdentCollector { + fn visit_pat(&mut self, pat: &Pat<'_>) { + match pat.kind { + PatKind::Binding(BindingAnnotation::Unannotated, _, ident, None) => self.idents.push(ident), + PatKind::Struct(_, fields, _) => { + for field in fields { + self.visit_pat(field.pat); + } + }, + PatKind::TupleStruct(_, pats, _) | PatKind::Or(pats) | PatKind::Tuple(pats, _) => { + for pat in pats { + self.visit_pat(pat); + } + }, + PatKind::Box(p) | PatKind::Ref(p, _) => self.visit_pat(p), + PatKind::Slice(pats_a, pat_b, pats_c) => { + for pat in pats_a { + self.visit_pat(pat); + } + if let Some(pat) = pat_b { + self.visit_pat(pat); + } + for pat in pats_c { + self.visit_pat(pat); + } + }, + _ => (), + } + } +} + +fn collect_ret_exprs<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Vec<&'tcx Expr<'tcx>> { + let mut ret_exprs = Vec::new(); + find_all_ret_expressions(cx, expr, |ret_expr| { + ret_exprs.push(ret_expr); + true + }); + ret_exprs +} diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index f029067d3671..770767a60582 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -5,6 +5,7 @@ mod explicit_iter_loop; mod for_kv_map; mod for_loops_over_fallibles; mod iter_next_loop; +mod manual_find; mod manual_flatten; mod manual_memcpy; mod missing_spin_loop; @@ -597,6 +598,37 @@ declare_clippy_lint! { "An empty busy waiting loop" } +declare_clippy_lint! { + /// ### What it does + /// Check for manual implementations of Iterator::find + /// + /// ### Why is this bad? + /// It doesn't affect performance, but using `find` is shorter and easier to read. + /// + /// ### Example + /// + /// ```rust + /// fn example(arr: Vec) -> Option { + /// for el in arr { + /// if el == 1 { + /// return Some(el); + /// } + /// } + /// None + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn example(arr: Vec) -> Option { + /// arr.into_iter().find(|&el| el == 1) + /// } + /// ``` + #[clippy::version = "1.61.0"] + pub MANUAL_FIND, + complexity, + "Manual implementation of Iterator::find" +} + declare_lint_pass!(Loops => [ MANUAL_MEMCPY, MANUAL_FLATTEN, @@ -617,6 +649,7 @@ declare_lint_pass!(Loops => [ SAME_ITEM_PUSH, SINGLE_ELEMENT_LOOP, MISSING_SPIN_LOOP, + MANUAL_FIND, ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -692,6 +725,7 @@ fn check_for_loop<'tcx>( single_element_loop::check(cx, pat, arg, body, expr); same_item_push::check(cx, pat, arg, body, expr); manual_flatten::check(cx, pat, arg, body, span); + manual_find::check(cx, pat, arg, body, span); } fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index 0d0c88b02c78..4a3edbb9d37c 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -159,12 +159,10 @@ impl<'a, 'tcx, 'b> Visitor<'tcx> for SimilarNamesNameVisitor<'a, 'tcx, 'b> { #[must_use] fn get_exemptions(interned_name: &str) -> Option<&'static [&'static str]> { - for &list in ALLOWED_TO_BE_SIMILAR { - if allowed_to_be_similar(interned_name, list) { - return Some(list); - } - } - None + ALLOWED_TO_BE_SIMILAR + .iter() + .find(|&&list| allowed_to_be_similar(interned_name, list)) + .copied() } #[must_use] diff --git a/tests/ui/manual_find.fixed b/tests/ui/manual_find.fixed new file mode 100644 index 000000000000..9d4cbb597fcd --- /dev/null +++ b/tests/ui/manual_find.fixed @@ -0,0 +1,112 @@ +// run-rustfix + +#![allow(unused)] +#![warn(clippy::manual_find)] + +use std::collections::HashMap; + +const ARRAY: &[u32; 5] = &[2, 7, 1, 9, 3]; + +fn lookup(n: u32) -> Option { + ARRAY.iter().find(|&&v| v == n).copied() +} + +fn with_pat(arr: Vec<(u32, u32)>) -> Option { + arr.into_iter().map(|(a, _)| a).find(|&a| a % 2 == 0) +} + +struct Data { + name: String, + is_true: bool, +} +fn with_struct(arr: Vec) -> Option { + arr.into_iter().find(|el| el.name.len() == 10) +} + +struct Tuple(usize, usize); +fn with_tuple_struct(arr: Vec) -> Option { + arr.into_iter().map(|Tuple(a, _)| a).find(|&a| a >= 3) +} + +struct A; +impl A { + fn should_keep(&self) -> bool { + true + } +} +fn with_method_call(arr: Vec) -> Option { + arr.into_iter().find(|el| el.should_keep()) +} + +fn with_closure(arr: Vec) -> Option { + let f = |el: u32| -> u32 { el + 10 }; + arr.into_iter().find(|&el| f(el) == 20) +} + +fn with_closure2(arr: HashMap) -> Option { + let f = |el: i32| -> bool { el == 10 }; + arr.values().find(|&&el| f(el)).copied() +} + +fn with_bool(arr: Vec) -> Option { + arr.into_iter().find(|el| el.is_true) +} + +fn with_side_effects(arr: Vec) -> Option { + for v in arr { + if v == 1 { + println!("side effect"); + return Some(v); + } + } + None +} + +fn with_else(arr: Vec) -> Option { + for el in arr { + if el % 2 == 0 { + return Some(el); + } else { + println!("{}", el); + } + } + None +} + +fn tuple_with_ref(v: [(u8, &u8); 3]) -> Option { + v.into_iter().map(|(_, &x)| x).find(|&x| x > 10) +} + +fn ref_to_tuple_with_ref(v: &[(u8, &u8)]) -> Option { + v.iter().map(|&(_, &x)| x).find(|&x| x > 10) +} + +// Not handled yet +fn mut_binding(v: Vec) -> Option { + for mut s in v { + if s.as_mut_str().len() > 1 { + return Some(s); + } + } + None +} + +fn subpattern(v: Vec<[u32; 32]>) -> Option<[u32; 32]> { + for a @ [first, ..] in v { + if a[12] == first { + return Some(a); + } + } + None +} + +fn two_bindings(v: Vec<(u8, u8)>) -> Option { + for (a, n) in v { + if a == n { + return Some(a); + } + } + None +} + +fn main() {} diff --git a/tests/ui/manual_find.rs b/tests/ui/manual_find.rs new file mode 100644 index 000000000000..3699cea29f2b --- /dev/null +++ b/tests/ui/manual_find.rs @@ -0,0 +1,162 @@ +// run-rustfix + +#![allow(unused)] +#![warn(clippy::manual_find)] + +use std::collections::HashMap; + +const ARRAY: &[u32; 5] = &[2, 7, 1, 9, 3]; + +fn lookup(n: u32) -> Option { + for &v in ARRAY { + if v == n { + return Some(v); + } + } + None +} + +fn with_pat(arr: Vec<(u32, u32)>) -> Option { + for (a, _) in arr { + if a % 2 == 0 { + return Some(a); + } + } + None +} + +struct Data { + name: String, + is_true: bool, +} +fn with_struct(arr: Vec) -> Option { + for el in arr { + if el.name.len() == 10 { + return Some(el); + } + } + None +} + +struct Tuple(usize, usize); +fn with_tuple_struct(arr: Vec) -> Option { + for Tuple(a, _) in arr { + if a >= 3 { + return Some(a); + } + } + None +} + +struct A; +impl A { + fn should_keep(&self) -> bool { + true + } +} +fn with_method_call(arr: Vec) -> Option { + for el in arr { + if el.should_keep() { + return Some(el); + } + } + None +} + +fn with_closure(arr: Vec) -> Option { + let f = |el: u32| -> u32 { el + 10 }; + for el in arr { + if f(el) == 20 { + return Some(el); + } + } + None +} + +fn with_closure2(arr: HashMap) -> Option { + let f = |el: i32| -> bool { el == 10 }; + for &el in arr.values() { + if f(el) { + return Some(el); + } + } + None +} + +fn with_bool(arr: Vec) -> Option { + for el in arr { + if el.is_true { + return Some(el); + } + } + None +} + +fn with_side_effects(arr: Vec) -> Option { + for v in arr { + if v == 1 { + println!("side effect"); + return Some(v); + } + } + None +} + +fn with_else(arr: Vec) -> Option { + for el in arr { + if el % 2 == 0 { + return Some(el); + } else { + println!("{}", el); + } + } + None +} + +fn tuple_with_ref(v: [(u8, &u8); 3]) -> Option { + for (_, &x) in v { + if x > 10 { + return Some(x); + } + } + None +} + +fn ref_to_tuple_with_ref(v: &[(u8, &u8)]) -> Option { + for &(_, &x) in v { + if x > 10 { + return Some(x); + } + } + None +} + +// Not handled yet +fn mut_binding(v: Vec) -> Option { + for mut s in v { + if s.as_mut_str().len() > 1 { + return Some(s); + } + } + None +} + +fn subpattern(v: Vec<[u32; 32]>) -> Option<[u32; 32]> { + for a @ [first, ..] in v { + if a[12] == first { + return Some(a); + } + } + None +} + +fn two_bindings(v: Vec<(u8, u8)>) -> Option { + for (a, n) in v { + if a == n { + return Some(a); + } + } + None +} + +fn main() {} diff --git a/tests/ui/manual_find.stderr b/tests/ui/manual_find.stderr new file mode 100644 index 000000000000..d4db69fbc1d2 --- /dev/null +++ b/tests/ui/manual_find.stderr @@ -0,0 +1,114 @@ +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:11:5 + | +LL | / for &v in ARRAY { +LL | | if v == n { +LL | | return Some(v); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `ARRAY.iter().find(|&&v| v == n).copied()` + | + = note: `-D clippy::manual-find` implied by `-D warnings` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:20:5 + | +LL | / for (a, _) in arr { +LL | | if a % 2 == 0 { +LL | | return Some(a); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.into_iter().map(|(a, _)| a).find(|&a| a % 2 == 0)` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:33:5 + | +LL | / for el in arr { +LL | | if el.name.len() == 10 { +LL | | return Some(el); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.into_iter().find(|el| el.name.len() == 10)` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:43:5 + | +LL | / for Tuple(a, _) in arr { +LL | | if a >= 3 { +LL | | return Some(a); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.into_iter().map(|Tuple(a, _)| a).find(|&a| a >= 3)` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:58:5 + | +LL | / for el in arr { +LL | | if el.should_keep() { +LL | | return Some(el); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.into_iter().find(|el| el.should_keep())` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:68:5 + | +LL | / for el in arr { +LL | | if f(el) == 20 { +LL | | return Some(el); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.into_iter().find(|&el| f(el) == 20)` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:78:5 + | +LL | / for &el in arr.values() { +LL | | if f(el) { +LL | | return Some(el); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.values().find(|&&el| f(el)).copied()` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:87:5 + | +LL | / for el in arr { +LL | | if el.is_true { +LL | | return Some(el); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `arr.into_iter().find(|el| el.is_true)` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:117:5 + | +LL | / for (_, &x) in v { +LL | | if x > 10 { +LL | | return Some(x); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `v.into_iter().map(|(_, &x)| x).find(|&x| x > 10)` + +error: manual implementation of Iterator::find + --> $DIR/manual_find.rs:126:5 + | +LL | / for &(_, &x) in v { +LL | | if x > 10 { +LL | | return Some(x); +LL | | } +LL | | } +LL | | None + | |________^ help: replace with an iterator: `v.iter().map(|&(_, &x)| x).find(|&x| x > 10)` + +error: aborting due to 10 previous errors +