Auto merge of rust-lang#6685 - magurotuna:filter_map_identity, r=phansch

Add new lint `filter_map_identity`

<!--
Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
`changelog: none`. Otherwise, please write a short comment
explaining your change.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- \[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`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.
-->

This commit adds a new lint named filter_map_identity.
This lint is the same as `flat_map_identity` except that it checks for the usage of `filter_map`.

---

Closes rust-lang#6643

changelog: Added a new lint: `filter_map_identity`

CHANGELOG.md

@@ -1955,6 +1955,7 @@ Released 2018-09-13
 [`field_reassign_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#field_reassign_with_default
 [`filetype_is_file`]: https://rust-lang.github.io/rust-clippy/master/index.html#filetype_is_file
 [`filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map
+[`filter_map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_identity
 [`filter_map_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_next
 [`filter_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_next
 [`find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#find_map

clippy_lints/src/lib.rs

@@ -743,6 +743,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::EXPECT_USED,
         &methods::FILETYPE_IS_FILE,
         &methods::FILTER_MAP,
+        &methods::FILTER_MAP_IDENTITY,
         &methods::FILTER_MAP_NEXT,
         &methods::FILTER_NEXT,
         &methods::FLAT_MAP_IDENTITY,
@@ -1535,6 +1536,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::CLONE_DOUBLE_REF),
         LintId::of(&methods::CLONE_ON_COPY),
         LintId::of(&methods::EXPECT_FUN_CALL),
+        LintId::of(&methods::FILTER_MAP_IDENTITY),
         LintId::of(&methods::FILTER_NEXT),
         LintId::of(&methods::FLAT_MAP_IDENTITY),
         LintId::of(&methods::FROM_ITER_INSTEAD_OF_COLLECT),
@@ -1842,6 +1844,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&matches::WILDCARD_IN_OR_PATTERNS),
         LintId::of(&methods::BIND_INSTEAD_OF_MAP),
         LintId::of(&methods::CLONE_ON_COPY),
+        LintId::of(&methods::FILTER_MAP_IDENTITY),
         LintId::of(&methods::FILTER_NEXT),
         LintId::of(&methods::FLAT_MAP_IDENTITY),
         LintId::of(&methods::INSPECT_FOR_EACH),

clippy_lints/src/loops.rs

@@ -738,7 +738,7 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult
 fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult {
     let stmts = block.stmts.iter().map(stmt_to_expr);
     let expr = once(block.expr.as_deref());
-    let mut iter = stmts.chain(expr).filter_map(|e| e);
+    let mut iter = stmts.chain(expr).flatten();
     never_loop_expr_seq(&mut iter, main_loop_id)
 }
 

clippy_lints/src/methods/filter_map_identity.rs

@@ -0,0 +1,56 @@
+use crate::utils::{match_qpath, match_trait_method, paths, span_lint_and_sugg};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_span::source_map::Span;
+
+use super::FILTER_MAP_IDENTITY;
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    expr: &hir::Expr<'_>,
+    filter_map_args: &[hir::Expr<'_>],
+    filter_map_span: Span,
+) {
+    if match_trait_method(cx, expr, &paths::ITERATOR) {
+        let arg_node = &filter_map_args[1].kind;
+
+        let apply_lint = |message: &str| {
+            span_lint_and_sugg(
+                cx,
+                FILTER_MAP_IDENTITY,
+                filter_map_span.with_hi(expr.span.hi()),
+                message,
+                "try",
+                "flatten()".to_string(),
+                Applicability::MachineApplicable,
+            );
+        };
+
+        if_chain! {
+            if let hir::ExprKind::Closure(_, _, body_id, _, _) = arg_node;
+            let body = cx.tcx.hir().body(*body_id);
+
+            if let hir::PatKind::Binding(_, _, binding_ident, _) = body.params[0].pat.kind;
+            if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = body.value.kind;
+
+            if path.segments.len() == 1;
+            if path.segments[0].ident.name == binding_ident.name;
+
+            then {
+                apply_lint("called `filter_map(|x| x)` on an `Iterator`");
+            }
+        }
+
+        if_chain! {
+            if let hir::ExprKind::Path(ref qpath) = arg_node;
+
+            if match_qpath(qpath, &paths::STD_CONVERT_IDENTITY);
+
+            then {
+                apply_lint("called `filter_map(std::convert::identity)` on an `Iterator`");
+            }
+        }
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -1,4 +1,5 @@
 mod bind_instead_of_map;
+mod filter_map_identity;
 mod inefficient_to_string;
 mod inspect_for_each;
 mod manual_saturating_arithmetic;
@@ -1466,6 +1467,29 @@ declare_clippy_lint! {
     "using `.inspect().for_each()`, which can be replaced with `.for_each()`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `filter_map(|x| x)`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely by using `flatten`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// # let iter = vec![Some(1)].into_iter();
+    /// iter.filter_map(|x| x);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # let iter = vec![Some(1)].into_iter();
+    /// iter.flatten();
+    /// ```
+    pub FILTER_MAP_IDENTITY,
+    complexity,
+    "call to `filter_map` where `flatten` is sufficient"
+}
+
 pub struct Methods {
     msrv: Option<RustcVersion>,
 }
@@ -1503,6 +1527,7 @@ impl_lint_pass!(Methods => [
     FILTER_NEXT,
     SKIP_WHILE_NEXT,
     FILTER_MAP,
+    FILTER_MAP_IDENTITY,
     MANUAL_FILTER_MAP,
     MANUAL_FIND_MAP,
     FILTER_MAP_NEXT,
@@ -1596,7 +1621,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             ["as_ref"] => lint_asref(cx, expr, "as_ref", arg_lists[0]),
             ["as_mut"] => lint_asref(cx, expr, "as_mut", arg_lists[0]),
             ["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0], method_spans[0]),
-            ["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]),
+            ["filter_map", ..] => {
+                unnecessary_filter_map::lint(cx, expr, arg_lists[0]);
+                filter_map_identity::check(cx, expr, arg_lists[0], method_spans[0]);
+            },
             ["count", "map"] => lint_suspicious_map(cx, expr),
             ["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr),
             ["unwrap_or", arith @ ("checked_add" | "checked_sub" | "checked_mul")] => {

tests/ui/filter_map_identity.fixed

@@ -0,0 +1,16 @@
+// run-rustfix
+
+#![allow(unused_imports)]
+#![warn(clippy::filter_map_identity)]
+
+fn main() {
+    let iterator = vec![Some(1), None, Some(2)].into_iter();
+    let _ = iterator.flatten();
+
+    let iterator = vec![Some(1), None, Some(2)].into_iter();
+    let _ = iterator.flatten();
+
+    use std::convert::identity;
+    let iterator = vec![Some(1), None, Some(2)].into_iter();
+    let _ = iterator.flatten();
+}

tests/ui/filter_map_identity.rs

@@ -0,0 +1,16 @@
+// run-rustfix
+
+#![allow(unused_imports)]
+#![warn(clippy::filter_map_identity)]
+
+fn main() {
+    let iterator = vec![Some(1), None, Some(2)].into_iter();
+    let _ = iterator.filter_map(|x| x);
+
+    let iterator = vec![Some(1), None, Some(2)].into_iter();
+    let _ = iterator.filter_map(std::convert::identity);
+
+    use std::convert::identity;
+    let iterator = vec![Some(1), None, Some(2)].into_iter();
+    let _ = iterator.filter_map(identity);
+}

tests/ui/filter_map_identity.stderr

@@ -0,0 +1,22 @@
+error: called `filter_map(|x| x)` on an `Iterator`
+  --> $DIR/filter_map_identity.rs:8:22
+   |
+LL |     let _ = iterator.filter_map(|x| x);
+   |                      ^^^^^^^^^^^^^^^^^ help: try: `flatten()`
+   |
+   = note: `-D clippy::filter-map-identity` implied by `-D warnings`
+
+error: called `filter_map(std::convert::identity)` on an `Iterator`
+  --> $DIR/filter_map_identity.rs:11:22
+   |
+LL |     let _ = iterator.filter_map(std::convert::identity);
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`
+
+error: called `filter_map(std::convert::identity)` on an `Iterator`
+  --> $DIR/filter_map_identity.rs:15:22
+   |
+LL |     let _ = iterator.filter_map(identity);
+   |                      ^^^^^^^^^^^^^^^^^^^^ help: try: `flatten()`
+
+error: aborting due to 3 previous errors
+