Auto merge of #5694 - wangtheo:issue-5626, r=matthiaskrgr

#5626: lint iterator.map(|x| x)

changelog: adds a new lint for iterator.map(|x| x) (see https://github.com/rust-lang/rust-clippy/issues/5626)

The code also lints for result.map(|x| x) and option.map(|x| x). Also, I'm not sure if I'm checking for type adjustments correctly and I can't think of an example where .map(|x| x) would apply type adjustments.

10 files changed, 228 insertions, 2 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index adc945a6944..d6186c319d0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1508,6 +1508,7 @@ Released 2018-09-13
 [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
 [`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
 [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
+[`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity
 [`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or
 [`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref
 [`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 501220f28e5..756b6b9a8a4 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -229,6 +229,7 @@ mod main_recursion;
 mod manual_async_fn;
 mod manual_non_exhaustive;
 mod map_clone;
+mod map_identity;
 mod map_unit_fn;
 mod match_on_vec_items;
 mod matches;
@@ -608,6 +609,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &manual_async_fn::MANUAL_ASYNC_FN,
         &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
         &map_clone::MAP_CLONE,
+        &map_identity::MAP_IDENTITY,
         &map_unit_fn::OPTION_MAP_UNIT_FN,
         &map_unit_fn::RESULT_MAP_UNIT_FN,
         &match_on_vec_items::MATCH_ON_VEC_ITEMS,
@@ -1057,6 +1059,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     });
     store.register_early_pass(|| box unnested_or_patterns::UnnestedOrPatterns);
     store.register_late_pass(|| box macro_use::MacroUseImports::default());
+    store.register_late_pass(|| box map_identity::MapIdentity);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1273,6 +1276,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
         LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
         LintId::of(&map_clone::MAP_CLONE),
+        LintId::of(&map_identity::MAP_IDENTITY),
         LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
         LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),
         LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),
@@ -1550,6 +1554,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::EXPLICIT_COUNTER_LOOP),
         LintId::of(&loops::MUT_RANGE_BOUND),
         LintId::of(&loops::WHILE_LET_LOOP),
+        LintId::of(&map_identity::MAP_IDENTITY),
         LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
         LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),
         LintId::of(&matches::MATCH_AS_REF),
diff --git a/clippy_lints/src/map_identity.rs b/clippy_lints/src/map_identity.rs
new file mode 100644
index 00000000000..6607a26b130
--- /dev/null
+++ b/clippy_lints/src/map_identity.rs
@@ -0,0 +1,126 @@
+use crate::utils::{
+    is_adjusted, is_type_diagnostic_item, match_path, match_trait_method, match_var, paths, remove_blocks,
+    span_lint_and_sugg,
+};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Body, Expr, ExprKind, Pat, PatKind, QPath, StmtKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for instances of `map(f)` where `f` is the identity function.
+    ///
+    /// **Why is this bad?** It can be written more concisely without the call to `map`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let x = [1, 2, 3];
+    /// let y: Vec<_> = x.iter().map(|x| x).map(|x| 2*x).collect();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let x = [1, 2, 3];
+    /// let y: Vec<_> = x.iter().map(|x| 2*x).collect();
+    /// ```
+    pub MAP_IDENTITY,
+    complexity,
+    "using iterator.map(|x| x)"
+}
+
+declare_lint_pass!(MapIdentity => [MAP_IDENTITY]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MapIdentity {
+    fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
+        if expr.span.from_expansion() {
+            return;
+        }
+
+        if_chain! {
+            if let Some([caller, func]) = get_map_argument(cx, expr);
+            if is_expr_identity_function(cx, func);
+            then {
+                span_lint_and_sugg(
+                    cx,
+                    MAP_IDENTITY,
+                    expr.span.trim_start(caller.span).unwrap(),
+                    "unnecessary map of the identity function",
+                    "remove the call to `map`",
+                    String::new(),
+                    Applicability::MachineApplicable
+                )
+            }
+        }
+    }
+}
+
+/// Returns the arguments passed into map() if the expression is a method call to
+/// map(). Otherwise, returns None.
+fn get_map_argument<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'a>) -> Option<&'a [Expr<'a>]> {
+    if_chain! {
+        if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind;
+        if args.len() == 2 && method.ident.as_str() == "map";
+        let caller_ty = cx.tables.expr_ty(&args[0]);
+        if match_trait_method(cx, expr, &paths::ITERATOR)
+            || is_type_diagnostic_item(cx, caller_ty, sym!(result_type))
+            || is_type_diagnostic_item(cx, caller_ty, sym!(option_type));
+        then {
+            Some(args)
+        } else {
+            None
+        }
+    }
+}
+
+/// Checks if an expression represents the identity function
+/// Only examines closures and `std::convert::identity`
+fn is_expr_identity_function(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+    match expr.kind {
+        ExprKind::Closure(_, _, body_id, _, _) => is_body_identity_function(cx, cx.tcx.hir().body(body_id)),
+        ExprKind::Path(QPath::Resolved(_, ref path)) => match_path(path, &paths::STD_CONVERT_IDENTITY),
+        _ => false,
+    }
+}
+
+/// Checks if a function's body represents the identity function
+/// Looks for bodies of the form `|x| x`, `|x| return x`, `|x| { return x }` or `|x| {
+/// return x; }`
+fn is_body_identity_function(cx: &LateContext<'_, '_>, func: &Body<'_>) -> bool {
+    let params = func.params;
+    let body = remove_blocks(&func.value);
+
+    // if there's less/more than one parameter, then it is not the identity function
+    if params.len() != 1 {
+        return false;
+    }
+
+    match body.kind {
+        ExprKind::Path(QPath::Resolved(None, _)) => match_expr_param(cx, body, params[0].pat),
+        ExprKind::Ret(Some(ref ret_val)) => match_expr_param(cx, ret_val, params[0].pat),
+        ExprKind::Block(ref block, _) => {
+            if_chain! {
+                if block.stmts.len() == 1;
+                if let StmtKind::Semi(ref expr) | StmtKind::Expr(ref expr) = block.stmts[0].kind;
+                if let ExprKind::Ret(Some(ref ret_val)) = expr.kind;
+                then {
+                    match_expr_param(cx, ret_val, params[0].pat)
+                } else {
+                    false
+                }
+            }
+        },
+        _ => false,
+    }
+}
+
+/// Returns true iff an expression returns the same thing as a parameter's pattern
+fn match_expr_param(cx: &LateContext<'_, '_>, expr: &Expr<'_>, pat: &Pat<'_>) -> bool {
+    if let PatKind::Binding(_, _, ident, _) = pat.kind {
+        match_var(expr, ident.name) && !(cx.tables.hir_owner == Some(expr.hir_id.owner) && is_adjusted(cx, expr))
+    } else {
+        false
+    }
+}
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index edceb755180..5a43a1a07d2 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1145,6 +1145,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         module: "methods",
     },
     Lint {
+        name: "map_identity",
+        group: "complexity",
+        desc: "using iterator.map(|x| x)",
+        deprecation: None,
+        module: "map_identity",
+    },
+    Lint {
         name: "map_unwrap_or",
         group: "pedantic",
         desc: "using `.map(f).unwrap_or(a)` or `.map(f).unwrap_or_else(func)`, which are more succinctly expressed as `map_or(a, f)` or `map_or_else(a, f)`",
diff --git a/tests/ui/map_flatten.fixed b/tests/ui/map_flatten.fixed
index 7ac368878ab..4171d80f48a 100644
--- a/tests/ui/map_flatten.fixed
+++ b/tests/ui/map_flatten.fixed
@@ -2,6 +2,7 @@
 
 #![warn(clippy::all, clippy::pedantic)]
 #![allow(clippy::missing_docs_in_private_items)]
+#![allow(clippy::map_identity)]
 
 fn main() {
     let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect();
diff --git a/tests/ui/map_flatten.rs b/tests/ui/map_flatten.rs
index a608601039c..16a0fd090ad 100644
--- a/tests/ui/map_flatten.rs
+++ b/tests/ui/map_flatten.rs
@@ -2,6 +2,7 @@
 
 #![warn(clippy::all, clippy::pedantic)]
 #![allow(clippy::missing_docs_in_private_items)]
+#![allow(clippy::map_identity)]
 
 fn main() {
     let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
diff --git a/tests/ui/map_flatten.stderr b/tests/ui/map_flatten.stderr
index 3cf2abd5b6d..00bc41c15e9 100644
--- a/tests/ui/map_flatten.stderr
+++ b/tests/ui/map_flatten.stderr
@@ -1,5 +1,5 @@
 error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)`
-  --> $DIR/map_flatten.rs:7:21
+  --> $DIR/map_flatten.rs:8:21
    |
 LL |     let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map(|x| 0..x)`
@@ -7,7 +7,7 @@ LL |     let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().colle
    = note: `-D clippy::map-flatten` implied by `-D warnings`
 
 error: called `map(..).flatten()` on an `Option`. This is more succinctly expressed by calling `.and_then(..)`
-  --> $DIR/map_flatten.rs:8:24
+  --> $DIR/map_flatten.rs:9:24
    |
 LL |     let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `(Some(Some(1))).and_then(|x| x)`
diff --git a/tests/ui/map_identity.fixed b/tests/ui/map_identity.fixed
new file mode 100644
index 00000000000..4a1452b25f3
--- /dev/null
+++ b/tests/ui/map_identity.fixed
@@ -0,0 +1,23 @@
+// run-rustfix
+#![warn(clippy::map_identity)]
+#![allow(clippy::needless_return)]
+
+fn main() {
+    let x: [u16; 3] = [1, 2, 3];
+    // should lint
+    let _: Vec<_> = x.iter().map(not_identity).collect();
+    let _: Vec<_> = x.iter().collect();
+    let _: Option<u8> = Some(3);
+    let _: Result<i8, f32> = Ok(-3);
+    // should not lint
+    let _: Vec<_> = x.iter().map(|x| 2 * x).collect();
+    let _: Vec<_> = x.iter().map(not_identity).map(|x| return x - 4).collect();
+    let _: Option<u8> = None.map(|x: u8| x - 1);
+    let _: Result<i8, f32> = Err(2.3).map(|x: i8| {
+        return x + 3;
+    });
+}
+
+fn not_identity(x: &u16) -> u16 {
+    *x
+}
diff --git a/tests/ui/map_identity.rs b/tests/ui/map_identity.rs
new file mode 100644
index 00000000000..65c7e6e1ea5
--- /dev/null
+++ b/tests/ui/map_identity.rs
@@ -0,0 +1,25 @@
+// run-rustfix
+#![warn(clippy::map_identity)]
+#![allow(clippy::needless_return)]
+
+fn main() {
+    let x: [u16; 3] = [1, 2, 3];
+    // should lint
+    let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
+    let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
+    let _: Option<u8> = Some(3).map(|x| x);
+    let _: Result<i8, f32> = Ok(-3).map(|x| {
+        return x;
+    });
+    // should not lint
+    let _: Vec<_> = x.iter().map(|x| 2 * x).collect();
+    let _: Vec<_> = x.iter().map(not_identity).map(|x| return x - 4).collect();
+    let _: Option<u8> = None.map(|x: u8| x - 1);
+    let _: Result<i8, f32> = Err(2.3).map(|x: i8| {
+        return x + 3;
+    });
+}
+
+fn not_identity(x: &u16) -> u16 {
+    *x
+}
diff --git a/tests/ui/map_identity.stderr b/tests/ui/map_identity.stderr
new file mode 100644
index 00000000000..e4a0320cbda
--- /dev/null
+++ b/tests/ui/map_identity.stderr
@@ -0,0 +1,37 @@
+error: unnecessary map of the identity function
+  --> $DIR/map_identity.rs:8:47
+   |
+LL |     let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
+   |                                               ^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
+   |
+   = note: `-D clippy::map-identity` implied by `-D warnings`
+
+error: unnecessary map of the identity function
+  --> $DIR/map_identity.rs:9:57
+   |
+LL |     let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
+   |                                                         ^^^^^^^^^^^ help: remove the call to `map`
+
+error: unnecessary map of the identity function
+  --> $DIR/map_identity.rs:9:29
+   |
+LL |     let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
+
+error: unnecessary map of the identity function
+  --> $DIR/map_identity.rs:10:32
+   |
+LL |     let _: Option<u8> = Some(3).map(|x| x);
+   |                                ^^^^^^^^^^^ help: remove the call to `map`
+
+error: unnecessary map of the identity function
+  --> $DIR/map_identity.rs:11:36
+   |
+LL |       let _: Result<i8, f32> = Ok(-3).map(|x| {
+   |  ____________________________________^
+LL | |         return x;
+LL | |     });
+   | |______^ help: remove the call to `map`
+
+error: aborting due to 5 previous errors
+