Auto merge of rust-lang#7330 - xFrednet:0000-refactor-map-identity, r=flip1995

Refactoring identity function lints

I've noticed that we have several lints that all check for identity functions and each used their own check implementation. I moved the `is_expr_identity_function` function to `clippy_utils` and adapted all lints to reuse that one function. This should make the addition of new lints like this also easier in the future.

I've also moved the `map_identity` lint into the `methods` module. It's probably the best to review this PR by checking each commit individually. And that's it, have a great day 🙃

changelog: none

Author: bors

clippy_lints/src/lib.rs

@@ -254,7 +254,6 @@ mod manual_strip;
 mod manual_unwrap_or;
 mod map_clone;
 mod map_err_ignore;
-mod map_identity;
 mod map_unit_fn;
 mod match_on_vec_items;
 mod matches;
@@ -705,7 +704,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         manual_unwrap_or::MANUAL_UNWRAP_OR,
         map_clone::MAP_CLONE,
         map_err_ignore::MAP_ERR_IGNORE,
-        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,
@@ -765,6 +763,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         methods::MANUAL_STR_REPEAT,
         methods::MAP_COLLECT_RESULT_UNIT,
         methods::MAP_FLATTEN,
+        methods::MAP_IDENTITY,
         methods::MAP_UNWRAP_OR,
         methods::NEW_RET_NO_SELF,
         methods::OK_EXPECT,
@@ -1260,7 +1259,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(manual_strip::MANUAL_STRIP),
         LintId::of(manual_unwrap_or::MANUAL_UNWRAP_OR),
         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),
@@ -1301,6 +1299,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(methods::MANUAL_SATURATING_ARITHMETIC),
         LintId::of(methods::MANUAL_STR_REPEAT),
         LintId::of(methods::MAP_COLLECT_RESULT_UNIT),
+        LintId::of(methods::MAP_IDENTITY),
         LintId::of(methods::NEW_RET_NO_SELF),
         LintId::of(methods::OK_EXPECT),
         LintId::of(methods::OPTION_AS_REF_DEREF),
@@ -1586,7 +1585,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(loops::WHILE_LET_LOOP),
         LintId::of(manual_strip::MANUAL_STRIP),
         LintId::of(manual_unwrap_or::MANUAL_UNWRAP_OR),
-        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),
@@ -1601,6 +1599,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(methods::ITER_COUNT),
         LintId::of(methods::MANUAL_FILTER_MAP),
         LintId::of(methods::MANUAL_FIND_MAP),
+        LintId::of(methods::MAP_IDENTITY),
         LintId::of(methods::OPTION_AS_REF_DEREF),
         LintId::of(methods::OPTION_FILTER_MAP),
         LintId::of(methods::SEARCH_IS_SOME),
@@ -2039,7 +2038,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         single_char_binding_names_threshold,
     });
     store.register_late_pass(|| box macro_use::MacroUseImports::default());
-    store.register_late_pass(|| box map_identity::MapIdentity);
     store.register_late_pass(|| box pattern_type_mismatch::PatternTypeMismatch);
     store.register_late_pass(|| box stable_sort_primitive::StableSortPrimitive);
     store.register_late_pass(|| box repeat_once::RepeatOnce);

clippy_lints/src/map_identity.rs

@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::{is_expr_path_def_path, is_trait_method, path_to_local_id, paths};
-use if_chain::if_chain;
+use clippy_utils::{is_expr_identity_function, is_trait_method};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -9,32 +8,15 @@ use rustc_span::{source_map::Span, sym};
 use super::FILTER_MAP_IDENTITY;
 
 pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: &hir::Expr<'_>, filter_map_span: Span) {
-    if is_trait_method(cx, expr, sym::Iterator) {
-        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, _, _) = filter_map_arg.kind;
-            let body = cx.tcx.hir().body(body_id);
-
-            if let hir::PatKind::Binding(_, binding_id, ..) = body.params[0].pat.kind;
-            if path_to_local_id(&body.value, binding_id);
-            then {
-                apply_lint("called `filter_map(|x| x)` on an `Iterator`");
-            }
-        }
-
-        if is_expr_path_def_path(cx, filter_map_arg, &paths::CONVERT_IDENTITY) {
-            apply_lint("called `filter_map(std::convert::identity)` on an `Iterator`");
-        }
+    if is_trait_method(cx, expr, sym::Iterator) && is_expr_identity_function(cx, filter_map_arg) {
+        span_lint_and_sugg(
+            cx,
+            FILTER_MAP_IDENTITY,
+            filter_map_span.with_hi(expr.span.hi()),
+            "use of `filter_map` with an identity function",
+            "try",
+            "flatten()".to_string(),
+            Applicability::MachineApplicable,
+        );
     }
 }

clippy_lints/src/methods/filter_map_identity.rs

@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::{is_expr_path_def_path, is_trait_method, paths};
-use if_chain::if_chain;
+use clippy_utils::{is_expr_identity_function, is_trait_method};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -15,36 +14,15 @@ pub(super) fn check<'tcx>(
     flat_map_arg: &'tcx hir::Expr<'_>,
     flat_map_span: Span,
 ) {
-    if is_trait_method(cx, expr, sym::Iterator) {
-        let apply_lint = |message: &str| {
-            span_lint_and_sugg(
-                cx,
-                FLAT_MAP_IDENTITY,
-                flat_map_span.with_hi(expr.span.hi()),
-                message,
-                "try",
-                "flatten()".to_string(),
-                Applicability::MachineApplicable,
-            );
-        };
-
-        if_chain! {
-            if let hir::ExprKind::Closure(_, _, body_id, _, _) = flat_map_arg.kind;
-            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(_, path)) = body.value.kind;
-
-            if path.segments.len() == 1;
-            if path.segments[0].ident.name == binding_ident.name;
-
-            then {
-                apply_lint("called `flat_map(|x| x)` on an `Iterator`");
-            }
-        }
-
-        if is_expr_path_def_path(cx, flat_map_arg, &paths::CONVERT_IDENTITY) {
-            apply_lint("called `flat_map(std::convert::identity)` on an `Iterator`");
-        }
+    if is_trait_method(cx, expr, sym::Iterator) && is_expr_identity_function(cx, flat_map_arg) {
+        span_lint_and_sugg(
+            cx,
+            FLAT_MAP_IDENTITY,
+            flat_map_span.with_hi(expr.span.hi()),
+            "use of `flat_map` with an identity function",
+            "try",
+            "flatten()".to_string(),
+            Applicability::MachineApplicable,
+        );
     }
 }

clippy_lints/src/methods/flat_map_identity.rs

@@ -0,0 +1,38 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{is_expr_identity_function, is_trait_method};
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_span::{source_map::Span, sym};
+
+use super::MAP_IDENTITY;
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    expr: &hir::Expr<'_>,
+    caller: &hir::Expr<'_>,
+    map_arg: &hir::Expr<'_>,
+    _map_span: Span,
+) {
+    let caller_ty = cx.typeck_results().expr_ty(caller);
+
+    if_chain! {
+        if is_trait_method(cx, expr, sym::Iterator)
+            || is_type_diagnostic_item(cx, caller_ty, sym::result_type)
+            || is_type_diagnostic_item(cx, caller_ty, sym::option_type);
+        if is_expr_identity_function(cx, map_arg);
+        if let Some(sugg_span) = expr.span.trim_start(caller.span);
+        then {
+            span_lint_and_sugg(
+                cx,
+                MAP_IDENTITY,
+                sugg_span,
+                "unnecessary map of the identity function",
+                "remove the call to `map`",
+                String::new(),
+                Applicability::MachineApplicable,
+            )
+        }
+    }
+}

clippy_lints/src/methods/map_identity.rs

@@ -35,6 +35,7 @@ mod manual_saturating_arithmetic;
 mod manual_str_repeat;
 mod map_collect_result_unit;
 mod map_flatten;
+mod map_identity;
 mod map_unwrap_or;
 mod ok_expect;
 mod option_as_ref_deref;
@@ -1561,6 +1562,29 @@ declare_clippy_lint! {
     "call to `filter_map` where `flatten` is sufficient"
 }
 
+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_clippy_lint! {
     /// **What it does:** Checks for the use of `.bytes().nth()`.
     ///
@@ -1728,6 +1752,7 @@ impl_lint_pass!(Methods => [
     FILTER_NEXT,
     SKIP_WHILE_NEXT,
     FILTER_MAP_IDENTITY,
+    MAP_IDENTITY,
     MANUAL_FILTER_MAP,
     MANUAL_FIND_MAP,
     OPTION_FILTER_MAP,
@@ -2058,6 +2083,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                         _ => {},
                     }
                 }
+                map_identity::check(cx, expr, recv, m_arg, span);
             },
             ("map_or", [def, map]) => option_map_or_none::check(cx, expr, recv, def, map),
             ("next", []) => {