Auto merge of rust-lang#8649 - ebobrow:imperative_find, r=flip1995

add [`manual_find`] lint for function return case

part of the implementation discussed in rust-lang#7143

changelog: add [`manual_find`] lint for function return case

Author: bors

CHANGELOG.md

@@ -3520,6 +3520,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

clippy_lints/src/lib.register_all.rs

@@ -119,6 +119,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),

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),

clippy_lints/src/lib.register_lints.rs

@@ -237,6 +237,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,

clippy_lints/src/loops/manual_find.rs

@@ -0,0 +1,158 @@
+use super::utils::make_iterator_snippet;
+use super::MANUAL_FIND;
+use clippy_utils::{
+    diagnostics::span_lint_and_then, higher, is_lang_ctor, path_res, peel_blocks_with_stmt,
+    source::snippet_with_applicability, ty::implements_trait,
+};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{
+    def::Res, lang_items::LangItem, BindingAnnotation, Block, Expr, ExprKind, HirId, Node, Pat, PatKind, Stmt, StmtKind,
+};
+use rustc_lint::LateContext;
+use rustc_span::source_map::Span;
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    pat: &'tcx Pat<'_>,
+    arg: &'tcx Expr<'_>,
+    body: &'tcx Expr<'_>,
+    span: Span,
+    expr: &'tcx Expr<'_>,
+) {
+    let inner_expr = peel_blocks_with_stmt(body);
+    // 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(higher::If { cond, then, r#else: None, }) = higher::If::hir(inner_expr);
+        if let Some(binding_id) = get_binding(pat);
+        if let ExprKind::Block(block, _) = then.kind;
+        if let [stmt] = block.stmts;
+        if let StmtKind::Semi(semi) = stmt.kind;
+        if let ExprKind::Ret(Some(ret_value)) = semi.kind;
+        if let ExprKind::Call(Expr { kind: ExprKind::Path(ctor), .. }, [inner_ret]) = ret_value.kind;
+        if is_lang_ctor(cx, ctor, LangItem::OptionSome);
+        if path_res(cx, inner_ret) == Res::Local(binding_id);
+        if let Some((last_stmt, last_ret)) = last_stmt_and_ret(cx, expr);
+        then {
+            let mut applicability = Applicability::MachineApplicable;
+            let mut snippet = make_iterator_snippet(cx, arg, &mut applicability);
+            // Checks if `pat` is a single reference to a binding (`&x`)
+            let is_ref_to_binding =
+                matches!(pat.kind, PatKind::Ref(inner, _) if matches!(inner.kind, PatKind::Binding(..)));
+            // If `pat` is not a binding or a reference to a binding (`x` or `&x`)
+            // we need to map it to the binding returned by the function (i.e. `.map(|(x, _)| x)`)
+            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, inner_ret.span, "..", &mut applicability),
+                    )[..],
+                );
+            }
+            let ty = cx.typeck_results().expr_ty(inner_ret);
+            if cx.tcx.lang_items().copy_trait().map_or(false, |id| implements_trait(cx, ty, id, &[])) {
+                snippet.push_str(
+                    &format!(
+                        ".find(|{}{}| {})",
+                        "&".repeat(1 + usize::from(is_ref_to_binding)),
+                        snippet_with_applicability(cx, inner_ret.span, "..", &mut applicability),
+                        snippet_with_applicability(cx, cond.span, "..", &mut applicability),
+                    )[..],
+                );
+                if is_ref_to_binding {
+                    snippet.push_str(".copied()");
+                }
+            } else {
+                applicability = Applicability::MaybeIncorrect;
+                snippet.push_str(
+                    &format!(
+                        ".find(|{}| {})",
+                        snippet_with_applicability(cx, inner_ret.span, "..", &mut applicability),
+                        snippet_with_applicability(cx, cond.span, "..", &mut applicability),
+                    )[..],
+                );
+            }
+            // Extends to `last_stmt` to include semicolon in case of `return None;`
+            let lint_span = span.to(last_stmt.span).to(last_ret.span);
+            span_lint_and_then(
+                cx,
+                MANUAL_FIND,
+                lint_span,
+                "manual implementation of `Iterator::find`",
+                |diag| {
+                    if applicability == Applicability::MaybeIncorrect {
+                        diag.note("you may need to dereference some variables");
+                    }
+                    diag.span_suggestion(
+                        lint_span,
+                        "replace with an iterator",
+                        snippet,
+                        applicability,
+                    );
+                },
+            );
+        }
+    }
+}
+
+fn get_binding(pat: &Pat<'_>) -> Option<HirId> {
+    let mut hir_id = None;
+    let mut count = 0;
+    pat.each_binding(|annotation, id, _, _| {
+        count += 1;
+        if count > 1 {
+            hir_id = None;
+            return;
+        }
+        if let BindingAnnotation::Unannotated = annotation {
+            hir_id = Some(id);
+        }
+    });
+    hir_id
+}
+
+// Returns the last statement and last return if function fits format for lint
+fn last_stmt_and_ret<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'_>,
+) -> Option<(&'tcx Stmt<'tcx>, &'tcx Expr<'tcx>)> {
+    // Returns last non-return statement and the last return
+    fn extract<'tcx>(block: &Block<'tcx>) -> Option<(&'tcx Stmt<'tcx>, &'tcx Expr<'tcx>)> {
+        if let [.., last_stmt] = block.stmts {
+            if let Some(ret) = block.expr {
+                return Some((last_stmt, ret));
+            }
+            if_chain! {
+                if let [.., snd_last, _] = block.stmts;
+                if let StmtKind::Semi(last_expr) = last_stmt.kind;
+                if let ExprKind::Ret(Some(ret)) = last_expr.kind;
+                then {
+                    return Some((snd_last, ret));
+                }
+            }
+        }
+        None
+    }
+    let mut parent_iter = cx.tcx.hir().parent_iter(expr.hir_id);
+    if_chain! {
+        // This should be the loop
+        if let Some((node_hir, Node::Stmt(..))) = parent_iter.next();
+        // This should be the funciton body
+        if let Some((_, Node::Block(block))) = parent_iter.next();
+        if let Some((last_stmt, last_ret)) = extract(block);
+        if last_stmt.hir_id == node_hir;
+        if let ExprKind::Path(path) = &last_ret.kind;
+        if is_lang_ctor(cx, path, LangItem::OptionNone);
+        if let Some((_, Node::Expr(_block))) = parent_iter.next();
+        // This includes the function header
+        if let Some((_, func)) = parent_iter.next();
+        if func.fn_kind().is_some();
+        then {
+            Some((block.stmts.last().unwrap(), last_ret))
+        } else {
+            None
+        }
+    }
+}

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;
@@ -609,6 +610,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,
@@ -629,6 +661,7 @@ declare_lint_pass!(Loops => [
     SAME_ITEM_PUSH,
     SINGLE_ELEMENT_LOOP,
     MISSING_SPIN_LOOP,
+    MANUAL_FIND,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Loops {
@@ -703,6 +736,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, expr);
 }
 
 fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {

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]

tests/ui/manual_find.rs

@@ -0,0 +1,22 @@
+#![allow(unused)]
+#![warn(clippy::manual_find)]
+
+fn vec_string(strings: Vec<String>) -> Option<String> {
+    for s in strings {
+        if s == String::new() {
+            return Some(s);
+        }
+    }
+    None
+}
+
+fn tuple(arr: Vec<(String, i32)>) -> Option<String> {
+    for (s, _) in arr {
+        if s == String::new() {
+            return Some(s);
+        }
+    }
+    None
+}
+
+fn main() {}

tests/ui/manual_find.stderr

@@ -0,0 +1,29 @@
+error: manual implementation of `Iterator::find`
+  --> $DIR/manual_find.rs:5:5
+   |
+LL | /     for s in strings {
+LL | |         if s == String::new() {
+LL | |             return Some(s);
+LL | |         }
+LL | |     }
+LL | |     None
+   | |________^ help: replace with an iterator: `strings.into_iter().find(|s| s == String::new())`
+   |
+   = note: `-D clippy::manual-find` implied by `-D warnings`
+   = note: you may need to dereference some variables
+
+error: manual implementation of `Iterator::find`
+  --> $DIR/manual_find.rs:14:5
+   |
+LL | /     for (s, _) in arr {
+LL | |         if s == String::new() {
+LL | |             return Some(s);
+LL | |         }
+LL | |     }
+LL | |     None
+   | |________^ help: replace with an iterator: `arr.into_iter().map(|(s, _)| s).find(|s| s == String::new())`
+   |
+   = note: you may need to dereference some variables
+
+error: aborting due to 2 previous errors
+