Auto merge of #6646 - nahuakang:for_loops_over_options_or_results, r=flip1995

New Lint: Manual Flatten

This is a draft PR for [Issue 6564](rust-lang/rust-clippy#6564).

r? `@camsteffen`

- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`

---

*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: Add new lint [`manual_flatten`] to check for loops over a single `if let` expression with `Result` or `Option`.

Author: bors

CHANGELOG.md

@@ -2039,6 +2039,7 @@ Released 2018-09-13
 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
 [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
 [`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_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or

clippy_lints/src/lib.rs

@@ -687,6 +687,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &loops::FOR_KV_MAP,
         &loops::FOR_LOOPS_OVER_FALLIBLES,
         &loops::ITER_NEXT_LOOP,
+        &loops::MANUAL_FLATTEN,
         &loops::MANUAL_MEMCPY,
         &loops::MUT_RANGE_BOUND,
         &loops::NEEDLESS_COLLECT,
@@ -1491,6 +1492,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::FOR_KV_MAP),
         LintId::of(&loops::FOR_LOOPS_OVER_FALLIBLES),
         LintId::of(&loops::ITER_NEXT_LOOP),
+        LintId::of(&loops::MANUAL_FLATTEN),
         LintId::of(&loops::MANUAL_MEMCPY),
         LintId::of(&loops::MUT_RANGE_BOUND),
         LintId::of(&loops::NEEDLESS_COLLECT),
@@ -1822,6 +1824,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&lifetimes::EXTRA_UNUSED_LIFETIMES),
         LintId::of(&lifetimes::NEEDLESS_LIFETIMES),
         LintId::of(&loops::EXPLICIT_COUNTER_LOOP),
+        LintId::of(&loops::MANUAL_FLATTEN),
         LintId::of(&loops::MUT_RANGE_BOUND),
         LintId::of(&loops::SINGLE_ELEMENT_LOOP),
         LintId::of(&loops::WHILE_LET_LOOP),

clippy_lints/src/loops.rs

@@ -5,10 +5,10 @@ use crate::utils::usage::{is_unused, mutated_variables};
 use crate::utils::visitors::LocalUsedVisitor;
 use crate::utils::{
     contains_name, get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
-    indent_of, is_in_panic_handler, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item,
-    last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, single_segment_path, snippet,
-    snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg,
-    span_lint_and_then, sugg, SpanlessEq,
+    indent_of, is_in_panic_handler, is_integer_const, is_no_std_crate, is_ok_ctor, is_refutable, is_some_ctor,
+    is_type_diagnostic_item, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg,
+    single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
+    span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq,
 };
 use if_chain::if_chain;
 use rustc_ast::ast;
@@ -494,8 +494,40 @@ declare_clippy_lint! {
     "there is no reason to have a single element loop"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Check for unnecessary `if let` usage in a for loop
+    /// where only the `Some` or `Ok` variant of the iterator element is used.
+    ///
+    /// **Why is this bad?** It is verbose and can be simplified
+    /// by first calling the `flatten` method on the `Iterator`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let x = vec![Some(1), Some(2), Some(3)];
+    /// for n in x {
+    ///     if let Some(n) = n {
+    ///         println!("{}", n);
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let x = vec![Some(1), Some(2), Some(3)];
+    /// for n in x.into_iter().flatten() {
+    ///     println!("{}", n);
+    /// }
+    /// ```
+    pub MANUAL_FLATTEN,
+    complexity,
+    "for loops over `Option`s or `Result`s with a single expression can be simplified"
+}
+
 declare_lint_pass!(Loops => [
     MANUAL_MEMCPY,
+    MANUAL_FLATTEN,
     NEEDLESS_RANGE_LOOP,
     EXPLICIT_ITER_LOOP,
     EXPLICIT_INTO_ITER_LOOP,
@@ -517,14 +549,14 @@ declare_lint_pass!(Loops => [
 impl<'tcx> LateLintPass<'tcx> for Loops {
     #[allow(clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if let Some((pat, arg, body)) = higher::for_loop(expr) {
+        if let Some((pat, arg, body, span)) = higher::for_loop(expr) {
             // we don't want to check expanded macros
             // this check is not at the top of the function
             // since higher::for_loop expressions are marked as expansions
             if body.span.from_expansion() {
                 return;
             }
-            check_for_loop(cx, pat, arg, body, expr);
+            check_for_loop(cx, pat, arg, body, expr, span);
         }
 
         // we don't want to check expanded macros
@@ -819,6 +851,7 @@ fn check_for_loop<'tcx>(
     arg: &'tcx Expr<'_>,
     body: &'tcx Expr<'_>,
     expr: &'tcx Expr<'_>,
+    span: Span,
 ) {
     let is_manual_memcpy_triggered = detect_manual_memcpy(cx, pat, arg, body, expr);
     if !is_manual_memcpy_triggered {
@@ -830,6 +863,7 @@ fn check_for_loop<'tcx>(
     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);
+    check_manual_flatten(cx, pat, arg, body, span);
 }
 
 // this function assumes the given expression is a `for` loop.
@@ -1953,6 +1987,79 @@ fn check_for_single_element_loop<'tcx>(
     }
 }
 
+/// Check for unnecessary `if let` usage in a for loop where only the `Some` or `Ok` variant of the
+/// iterator element is used.
+fn check_manual_flatten<'tcx>(
+    cx: &LateContext<'tcx>,
+    pat: &'tcx Pat<'_>,
+    arg: &'tcx Expr<'_>,
+    body: &'tcx Expr<'_>,
+    span: Span,
+) {
+    if let ExprKind::Block(ref block, _) = body.kind {
+        // Ensure the `if let` statement is the only expression or statement in the for-loop
+        let inner_expr = if block.stmts.len() == 1 && block.expr.is_none() {
+            let match_stmt = &block.stmts[0];
+            if let StmtKind::Semi(inner_expr) = match_stmt.kind {
+                Some(inner_expr)
+            } else {
+                None
+            }
+        } else if block.stmts.is_empty() {
+            block.expr
+        } else {
+            None
+        };
+
+        if_chain! {
+            if let Some(inner_expr) = inner_expr;
+            if let ExprKind::Match(
+                ref match_expr, ref match_arms, MatchSource::IfLetDesugar{ contains_else_clause: false }
+            ) = inner_expr.kind;
+            // Ensure match_expr in `if let` statement is the same as the pat from the for-loop
+            if let PatKind::Binding(_, pat_hir_id, _, _) = pat.kind;
+            if let ExprKind::Path(QPath::Resolved(None, match_expr_path)) = match_expr.kind;
+            if let Res::Local(match_expr_path_id) = match_expr_path.res;
+            if pat_hir_id == match_expr_path_id;
+            // Ensure the `if let` statement is for the `Some` variant of `Option` or the `Ok` variant of `Result`
+            if let PatKind::TupleStruct(QPath::Resolved(None, path), _, _) = match_arms[0].pat.kind;
+            let some_ctor = is_some_ctor(cx, path.res);
+            let ok_ctor = is_ok_ctor(cx, path.res);
+            if some_ctor || ok_ctor;
+            let if_let_type = if some_ctor { "Some" } else { "Ok" };
+
+            then {
+                // Prepare the error message
+                let msg = format!("unnecessary `if let` since only the `{}` variant of the iterator element is used", if_let_type);
+
+                // Prepare the help message
+                let mut applicability = Applicability::MaybeIncorrect;
+                let arg_snippet = make_iterator_snippet(cx, arg, &mut applicability);
+
+                span_lint_and_then(
+                    cx,
+                    MANUAL_FLATTEN,
+                    span,
+                    &msg,
+                    |diag| {
+                        let sugg = format!("{}.flatten()", arg_snippet);
+                        diag.span_suggestion(
+                            arg.span,
+                            "try",
+                            sugg,
+                            Applicability::MaybeIncorrect,
+                        );
+                        diag.span_help(
+                            inner_expr.span,
+                            "...and remove the `if let` statement in the for loop",
+                        );
+                    }
+                );
+            }
+        }
+    }
+}
+
 struct MutatePairDelegate<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     hir_id_low: Option<HirId>,

clippy_lints/src/mut_mut.rs

@@ -52,7 +52,7 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for MutVisitor<'a, 'tcx> {
             return;
         }
 
-        if let Some((_, arg, body)) = higher::for_loop(expr) {
+        if let Some((_, arg, body, _)) = higher::for_loop(expr) {
             // A `for` loop lowers to:
             // ```rust
             // match ::std::iter::Iterator::next(&mut iter) {

clippy_lints/src/needless_question_mark.rs

@@ -1,8 +1,6 @@
 use rustc_errors::Applicability;
-use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::{Body, Expr, ExprKind, LangItem, MatchSource, QPath};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
-use rustc_middle::ty::DefIdTree;
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::sym;
@@ -160,7 +158,7 @@ fn is_some_or_ok_call<'a>(
         // Check outer expression matches CALL_IDENT(ARGUMENT) format
         if let ExprKind::Call(path, args) = &expr.kind;
         if let ExprKind::Path(QPath::Resolved(None, path)) = &path.kind;
-        if is_some_ctor(cx, path.res) || is_ok_ctor(cx, path.res);
+        if utils::is_some_ctor(cx, path.res) || utils::is_ok_ctor(cx, path.res);
 
         // Extract inner expression from ARGUMENT
         if let ExprKind::Match(inner_expr_with_q, _, MatchSource::TryDesugar) = &args[0].kind;
@@ -208,25 +206,3 @@ fn is_some_or_ok_call<'a>(
 fn has_implicit_error_from(cx: &LateContext<'_>, entire_expr: &Expr<'_>, inner_result_expr: &Expr<'_>) -> bool {
     return cx.typeck_results().expr_ty(entire_expr) != cx.typeck_results().expr_ty(inner_result_expr);
 }
-
-fn is_ok_ctor(cx: &LateContext<'_>, res: Res) -> bool {
-    if let Some(ok_id) = cx.tcx.lang_items().result_ok_variant() {
-        if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res {
-            if let Some(variant_id) = cx.tcx.parent(id) {
-                return variant_id == ok_id;
-            }
-        }
-    }
-    false
-}
-
-fn is_some_ctor(cx: &LateContext<'_>, res: Res) -> bool {
-    if let Some(some_id) = cx.tcx.lang_items().option_some_variant() {
-        if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res {
-            if let Some(variant_id) = cx.tcx.parent(id) {
-                return variant_id == some_id;
-            }
-        }
-    }
-    false
-}

clippy_lints/src/ranges.rs

@@ -442,7 +442,7 @@ fn check_reversed_empty_range(cx: &LateContext<'_>, expr: &Expr<'_>) {
         let mut cur_expr = expr;
         while let Some(parent_expr) = get_parent_expr(cx, cur_expr) {
             match higher::for_loop(parent_expr) {
-                Some((_, args, _)) if args.hir_id == expr.hir_id => return true,
+                Some((_, args, _, _)) if args.hir_id == expr.hir_id => return true,
                 _ => cur_expr = parent_expr,
             }
         }

clippy_lints/src/utils/higher.rs

@@ -9,6 +9,7 @@ use rustc_ast::ast;
 use rustc_hir as hir;
 use rustc_hir::{BorrowKind, Expr, ExprKind, StmtKind, UnOp};
 use rustc_lint::LateContext;
+use rustc_span::source_map::Span;
 
 /// Converts a hir binary operator to the corresponding `ast` type.
 #[must_use]
@@ -133,11 +134,11 @@ pub fn is_from_for_desugar(local: &hir::Local<'_>) -> bool {
     false
 }
 
-/// Recover the essential nodes of a desugared for loop:
-/// `for pat in arg { body }` becomes `(pat, arg, body)`.
+/// Recover the essential nodes of a desugared for loop as well as the entire span:
+/// `for pat in arg { body }` becomes `(pat, arg, body)`. Return `(pat, arg, body, span)`.
 pub fn for_loop<'tcx>(
     expr: &'tcx hir::Expr<'tcx>,
-) -> Option<(&hir::Pat<'_>, &'tcx hir::Expr<'tcx>, &'tcx hir::Expr<'tcx>)> {
+) -> Option<(&hir::Pat<'_>, &'tcx hir::Expr<'tcx>, &'tcx hir::Expr<'tcx>, Span)> {
     if_chain! {
         if let hir::ExprKind::Match(ref iterexpr, ref arms, hir::MatchSource::ForLoopDesugar) = expr.kind;
         if let hir::ExprKind::Call(_, ref iterargs) = iterexpr.kind;
@@ -148,7 +149,7 @@ pub fn for_loop<'tcx>(
         if let hir::StmtKind::Local(ref local) = let_stmt.kind;
         if let hir::StmtKind::Expr(ref expr) = body.kind;
         then {
-            return Some((&*local.pat, &iterargs[0], expr));
+            return Some((&*local.pat, &iterargs[0], expr, arms[0].span));
         }
     }
     None

clippy_lints/src/utils/internal_lints.rs

@@ -841,15 +841,13 @@ pub fn check_path(cx: &LateContext<'_>, path: &[&str]) -> bool {
     // implementations of native types. Check lang items.
     let path_syms: Vec<_> = path.iter().map(|p| Symbol::intern(p)).collect();
     let lang_items = cx.tcx.lang_items();
-    for lang_item in lang_items.items() {
-        if let Some(def_id) = lang_item {
-            let lang_item_path = cx.get_def_path(*def_id);
-            if path_syms.starts_with(&lang_item_path) {
-                if let [item] = &path_syms[lang_item_path.len()..] {
-                    for child in cx.tcx.item_children(*def_id) {
-                        if child.ident.name == *item {
-                            return true;
-                        }
+    for item_def_id in lang_items.items().iter().flatten() {
+        let lang_item_path = cx.get_def_path(*item_def_id);
+        if path_syms.starts_with(&lang_item_path) {
+            if let [item] = &path_syms[lang_item_path.len()..] {
+                for child in cx.tcx.item_children(*item_def_id) {
+                    if child.ident.name == *item {
+                        return true;
                     }
                 }
             }

clippy_lints/src/utils/mod.rs

@@ -37,7 +37,7 @@ use rustc_ast::ast::{self, Attribute, LitKind};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::def::{DefKind, Res};
+use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_hir::Node;
@@ -50,7 +50,7 @@ use rustc_lint::{LateContext, Level, Lint, LintContext};
 use rustc_middle::hir::exports::Export;
 use rustc_middle::hir::map::Map;
 use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
-use rustc_middle::ty::{self, layout::IntegerExt, Ty, TyCtxt, TypeFoldable};
+use rustc_middle::ty::{self, layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeFoldable};
 use rustc_semver::RustcVersion;
 use rustc_session::Session;
 use rustc_span::hygiene::{ExpnKind, MacroKind};
@@ -1703,6 +1703,30 @@ pub fn is_hir_ty_cfg_dependant(cx: &LateContext<'_>, ty: &hir::Ty<'_>) -> bool {
     }
 }
 
+/// Check if the resolution of a given path is an `Ok` variant of `Result`.
+pub fn is_ok_ctor(cx: &LateContext<'_>, res: Res) -> bool {
+    if let Some(ok_id) = cx.tcx.lang_items().result_ok_variant() {
+        if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res {
+            if let Some(variant_id) = cx.tcx.parent(id) {
+                return variant_id == ok_id;
+            }
+        }
+    }
+    false
+}
+
+/// Check if the resolution of a given path is a `Some` variant of `Option`.
+pub fn is_some_ctor(cx: &LateContext<'_>, res: Res) -> bool {
+    if let Some(some_id) = cx.tcx.lang_items().option_some_variant() {
+        if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res {
+            if let Some(variant_id) = cx.tcx.parent(id) {
+                return variant_id == some_id;
+            }
+        }
+    }
+    false
+}
+
 #[cfg(test)]
 mod test {
     use super::{reindent_multiline, without_block_comments};

clippy_lints/src/vec.rs

@@ -55,7 +55,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec {
 
         // search for `for _ in vec![…]`
         if_chain! {
-            if let Some((_, arg, _)) = higher::for_loop(expr);
+            if let Some((_, arg, _, _)) = higher::for_loop(expr);
             if let Some(vec_args) = higher::vec_macro(cx, arg);
             if is_copy(cx, vec_type(cx.typeck_results().expr_ty_adjusted(arg)));
             then {