Skip to content

Commit

Permalink
add manual_find lint for function return case
Browse files Browse the repository at this point in the history
  • Loading branch information
ebobrow committed May 7, 2022
1 parent 6206086 commit fb48453
Show file tree
Hide file tree
Showing 10 changed files with 619 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_complexity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
189 changes: 189 additions & 0 deletions clippy_lints/src/loops/manual_find.rs
Original file line number Diff line number Diff line change
@@ -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<Ident> {
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<Ident>,
}

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
}
34 changes: 34 additions & 0 deletions clippy_lints/src/loops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<i32>) -> Option<i32> {
/// for el in arr {
/// if el == 1 {
/// return Some(el);
/// }
/// }
/// None
/// }
/// ```
/// Use instead:
/// ```rust
/// fn example(arr: Vec<i32>) -> Option<i32> {
/// 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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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<'_>) {
Expand Down
10 changes: 4 additions & 6 deletions clippy_lints/src/non_expressive_names.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit fb48453

Please sign in to comment.