Refactor check_for_loop_arg; rename manual_flatten's lint back to check_manual_flatten

3 files changed, 154 insertions, 144 deletions

diff --git a/clippy_lints/src/loops/for_loop_arg.rs b/clippy_lints/src/loops/for_loop_arg.rs
new file mode 100644
index 00000000000..b33e7183984
--- /dev/null
+++ b/clippy_lints/src/loops/for_loop_arg.rs
@@ -0,0 +1,149 @@
+use crate::utils::{
+    is_type_diagnostic_item, match_trait_method, match_type, paths, snippet, snippet_with_applicability, span_lint,
+    span_lint_and_help, span_lint_and_sugg,
+};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, Mutability, Pat};
+use rustc_lint::LateContext;
+use rustc_middle::ty::{self, Ty, TyS};
+use rustc_span::symbol::sym;
+
+pub(super) fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, expr: &Expr<'_>) {
+    let mut next_loop_linted = false; // whether or not ITER_NEXT_LOOP lint was used
+    if let ExprKind::MethodCall(ref method, _, ref args, _) = arg.kind {
+        // just the receiver, no arguments
+        if args.len() == 1 {
+            let method_name = &*method.ident.as_str();
+            // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
+            if method_name == "iter" || method_name == "iter_mut" {
+                if is_ref_iterable_type(cx, &args[0]) {
+                    lint_iter_method(cx, args, arg, method_name);
+                }
+            } else if method_name == "into_iter" && match_trait_method(cx, arg, &paths::INTO_ITERATOR) {
+                let receiver_ty = cx.typeck_results().expr_ty(&args[0]);
+                let receiver_ty_adjusted = cx.typeck_results().expr_ty_adjusted(&args[0]);
+                if TyS::same_type(receiver_ty, receiver_ty_adjusted) {
+                    let mut applicability = Applicability::MachineApplicable;
+                    let object = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
+                    span_lint_and_sugg(
+                        cx,
+                        super::EXPLICIT_INTO_ITER_LOOP,
+                        arg.span,
+                        "it is more concise to loop over containers instead of using explicit \
+                         iteration methods",
+                        "to write this more concisely, try",
+                        object.to_string(),
+                        applicability,
+                    );
+                } else {
+                    let ref_receiver_ty = cx.tcx.mk_ref(
+                        cx.tcx.lifetimes.re_erased,
+                        ty::TypeAndMut {
+                            ty: receiver_ty,
+                            mutbl: Mutability::Not,
+                        },
+                    );
+                    if TyS::same_type(receiver_ty_adjusted, ref_receiver_ty) {
+                        lint_iter_method(cx, args, arg, method_name)
+                    }
+                }
+            } else if method_name == "next" && match_trait_method(cx, arg, &paths::ITERATOR) {
+                span_lint(
+                    cx,
+                    super::ITER_NEXT_LOOP,
+                    expr.span,
+                    "you are iterating over `Iterator::next()` which is an Option; this will compile but is \
+                    probably not what you want",
+                );
+                next_loop_linted = true;
+            }
+        }
+    }
+    if !next_loop_linted {
+        check_arg_type(cx, pat, arg);
+    }
+}
+
+/// Checks for `for` loops over `Option`s and `Result`s.
+fn check_arg_type(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
+    let ty = cx.typeck_results().expr_ty(arg);
+    if is_type_diagnostic_item(cx, ty, sym::option_type) {
+        span_lint_and_help(
+            cx,
+            super::FOR_LOOPS_OVER_FALLIBLES,
+            arg.span,
+            &format!(
+                "for loop over `{0}`, which is an `Option`. This is more readably written as an \
+                `if let` statement",
+                snippet(cx, arg.span, "_")
+            ),
+            None,
+            &format!(
+                "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`",
+                snippet(cx, pat.span, "_"),
+                snippet(cx, arg.span, "_")
+            ),
+        );
+    } else if is_type_diagnostic_item(cx, ty, sym::result_type) {
+        span_lint_and_help(
+            cx,
+            super::FOR_LOOPS_OVER_FALLIBLES,
+            arg.span,
+            &format!(
+                "for loop over `{0}`, which is a `Result`. This is more readably written as an \
+                `if let` statement",
+                snippet(cx, arg.span, "_")
+            ),
+            None,
+            &format!(
+                "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`",
+                snippet(cx, pat.span, "_"),
+                snippet(cx, arg.span, "_")
+            ),
+        );
+    }
+}
+
+fn lint_iter_method(cx: &LateContext<'_>, args: &[Expr<'_>], arg: &Expr<'_>, method_name: &str) {
+    let mut applicability = Applicability::MachineApplicable;
+    let object = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
+    let muta = if method_name == "iter_mut" { "mut " } else { "" };
+    span_lint_and_sugg(
+        cx,
+        super::EXPLICIT_ITER_LOOP,
+        arg.span,
+        "it is more concise to loop over references to containers instead of using explicit \
+         iteration methods",
+        "to write this more concisely, try",
+        format!("&{}{}", muta, object),
+        applicability,
+    )
+}
+
+/// Returns `true` if the type of expr is one that provides `IntoIterator` impls
+/// for `&T` and `&mut T`, such as `Vec`.
+#[rustfmt::skip]
+fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
+    // no walk_ptrs_ty: calling iter() on a reference can make sense because it
+    // will allow further borrows afterwards
+    let ty = cx.typeck_results().expr_ty(e);
+    is_iterable_array(ty, cx) ||
+    is_type_diagnostic_item(cx, ty, sym::vec_type) ||
+    match_type(cx, ty, &paths::LINKED_LIST) ||
+    is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) ||
+    is_type_diagnostic_item(cx, ty, sym!(hashset_type)) ||
+    is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) ||
+    match_type(cx, ty, &paths::BINARY_HEAP) ||
+    match_type(cx, ty, &paths::BTREEMAP) ||
+    match_type(cx, ty, &paths::BTREESET)
+}
+
+fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
+    // IntoIterator is currently only implemented for array sizes <= 32 in rustc
+    match ty.kind() {
+        ty::Array(_, n) => n
+            .try_eval_usize(cx.tcx, cx.param_env)
+            .map_or(false, |val| (0..=32).contains(&val)),
+        _ => false,
+    }
+}
diff --git a/clippy_lints/src/loops/manual_flatten.rs b/clippy_lints/src/loops/manual_flatten.rs
index b5881a1b8c4..db90d2c0468 100644
--- a/clippy_lints/src/loops/manual_flatten.rs
+++ b/clippy_lints/src/loops/manual_flatten.rs
@@ -8,7 +8,7 @@ use rustc_span::source_map::Span;
 
 /// Check for unnecessary `if let` usage in a for loop where only the `Some` or `Ok` variant of the
 /// iterator element is used.
-pub(super) fn lint<'tcx>(
+pub(super) fn check_manual_flatten<'tcx>(
     cx: &LateContext<'tcx>,
     pat: &'tcx Pat<'_>,
     arg: &'tcx Expr<'_>,
diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs
index 18b7e96d42c..63eeb3607b6 100644
--- a/clippy_lints/src/loops/mod.rs
+++ b/clippy_lints/src/loops/mod.rs
@@ -1,3 +1,4 @@
+mod for_loop_arg;
 mod manual_flatten;
 mod utils;
 
@@ -27,7 +28,7 @@ use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
 use rustc_middle::middle::region;
-use rustc_middle::ty::{self, Ty, TyS};
+use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::{sym, Ident, Symbol};
@@ -861,12 +862,12 @@ fn check_for_loop<'tcx>(
         check_for_loop_range(cx, pat, arg, body, expr);
         check_for_loop_explicit_counter(cx, pat, arg, body, expr);
     }
-    check_for_loop_arg(cx, pat, arg, expr);
+    for_loop_arg::check_for_loop_arg(cx, pat, arg, expr);
     check_for_loop_over_map_kv(cx, pat, arg, body, expr);
     check_for_mut_range_bound(cx, arg, body);
     check_for_single_element_loop(cx, pat, arg, body, expr);
     detect_same_item_push(cx, pat, arg, body, expr);
-    manual_flatten::lint(cx, pat, arg, body, span);
+    manual_flatten::check_manual_flatten(cx, pat, arg, body, span);
 }
 
 // this function assumes the given expression is a `for` loop.
@@ -1682,118 +1683,6 @@ fn is_end_eq_array_len<'tcx>(
     false
 }
 
-fn lint_iter_method(cx: &LateContext<'_>, args: &[Expr<'_>], arg: &Expr<'_>, method_name: &str) {
-    let mut applicability = Applicability::MachineApplicable;
-    let object = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
-    let muta = if method_name == "iter_mut" { "mut " } else { "" };
-    span_lint_and_sugg(
-        cx,
-        EXPLICIT_ITER_LOOP,
-        arg.span,
-        "it is more concise to loop over references to containers instead of using explicit \
-         iteration methods",
-        "to write this more concisely, try",
-        format!("&{}{}", muta, object),
-        applicability,
-    )
-}
-
-fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, expr: &Expr<'_>) {
-    let mut next_loop_linted = false; // whether or not ITER_NEXT_LOOP lint was used
-    if let ExprKind::MethodCall(ref method, _, ref args, _) = arg.kind {
-        // just the receiver, no arguments
-        if args.len() == 1 {
-            let method_name = &*method.ident.as_str();
-            // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
-            if method_name == "iter" || method_name == "iter_mut" {
-                if is_ref_iterable_type(cx, &args[0]) {
-                    lint_iter_method(cx, args, arg, method_name);
-                }
-            } else if method_name == "into_iter" && match_trait_method(cx, arg, &paths::INTO_ITERATOR) {
-                let receiver_ty = cx.typeck_results().expr_ty(&args[0]);
-                let receiver_ty_adjusted = cx.typeck_results().expr_ty_adjusted(&args[0]);
-                if TyS::same_type(receiver_ty, receiver_ty_adjusted) {
-                    let mut applicability = Applicability::MachineApplicable;
-                    let object = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);
-                    span_lint_and_sugg(
-                        cx,
-                        EXPLICIT_INTO_ITER_LOOP,
-                        arg.span,
-                        "it is more concise to loop over containers instead of using explicit \
-                         iteration methods",
-                        "to write this more concisely, try",
-                        object.to_string(),
-                        applicability,
-                    );
-                } else {
-                    let ref_receiver_ty = cx.tcx.mk_ref(
-                        cx.tcx.lifetimes.re_erased,
-                        ty::TypeAndMut {
-                            ty: receiver_ty,
-                            mutbl: Mutability::Not,
-                        },
-                    );
-                    if TyS::same_type(receiver_ty_adjusted, ref_receiver_ty) {
-                        lint_iter_method(cx, args, arg, method_name)
-                    }
-                }
-            } else if method_name == "next" && match_trait_method(cx, arg, &paths::ITERATOR) {
-                span_lint(
-                    cx,
-                    ITER_NEXT_LOOP,
-                    expr.span,
-                    "you are iterating over `Iterator::next()` which is an Option; this will compile but is \
-                    probably not what you want",
-                );
-                next_loop_linted = true;
-            }
-        }
-    }
-    if !next_loop_linted {
-        check_arg_type(cx, pat, arg);
-    }
-}
-
-/// Checks for `for` loops over `Option`s and `Result`s.
-fn check_arg_type(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
-    let ty = cx.typeck_results().expr_ty(arg);
-    if is_type_diagnostic_item(cx, ty, sym::option_type) {
-        span_lint_and_help(
-            cx,
-            FOR_LOOPS_OVER_FALLIBLES,
-            arg.span,
-            &format!(
-                "for loop over `{0}`, which is an `Option`. This is more readably written as an \
-                `if let` statement",
-                snippet(cx, arg.span, "_")
-            ),
-            None,
-            &format!(
-                "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`",
-                snippet(cx, pat.span, "_"),
-                snippet(cx, arg.span, "_")
-            ),
-        );
-    } else if is_type_diagnostic_item(cx, ty, sym::result_type) {
-        span_lint_and_help(
-            cx,
-            FOR_LOOPS_OVER_FALLIBLES,
-            arg.span,
-            &format!(
-                "for loop over `{0}`, which is a `Result`. This is more readably written as an \
-                `if let` statement",
-                snippet(cx, arg.span, "_")
-            ),
-            None,
-            &format!(
-                "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`",
-                snippet(cx, pat.span, "_"),
-                snippet(cx, arg.span, "_")
-            ),
-        );
-    }
-}
-
 // To trigger the EXPLICIT_COUNTER_LOOP lint, a variable must be
 // incremented exactly once in the loop body, and initialized to zero
 // at the start of the loop.
@@ -2270,34 +2159,6 @@ impl<'tcx> Visitor<'tcx> for VarUsedAfterLoopVisitor {
     }
 }
 
-/// Returns `true` if the type of expr is one that provides `IntoIterator` impls
-/// for `&T` and `&mut T`, such as `Vec`.
-#[rustfmt::skip]
-fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
-    // no walk_ptrs_ty: calling iter() on a reference can make sense because it
-    // will allow further borrows afterwards
-    let ty = cx.typeck_results().expr_ty(e);
-    is_iterable_array(ty, cx) ||
-    is_type_diagnostic_item(cx, ty, sym::vec_type) ||
-    match_type(cx, ty, &paths::LINKED_LIST) ||
-    is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) ||
-    is_type_diagnostic_item(cx, ty, sym!(hashset_type)) ||
-    is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) ||
-    match_type(cx, ty, &paths::BINARY_HEAP) ||
-    match_type(cx, ty, &paths::BTREEMAP) ||
-    match_type(cx, ty, &paths::BTREESET)
-}
-
-fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
-    // IntoIterator is currently only implemented for array sizes <= 32 in rustc
-    match ty.kind() {
-        ty::Array(_, n) => n
-            .try_eval_usize(cx.tcx, cx.param_env)
-            .map_or(false, |val| (0..=32).contains(&val)),
-        _ => false,
-    }
-}
-
 /// If a block begins with a statement (possibly a `let` binding) and has an
 /// expression, return it.
 fn extract_expr_from_first_stmt<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<'tcx>> {