move changed logic to into its own util function

Author: y21

clippy_lints/src/methods/filter_map_identity.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::{is_expr_identity_function, is_trait_method};
+use clippy_utils::{is_expr_untyped_identity_function, is_trait_method};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -9,7 +9,7 @@ use rustc_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) && is_expr_identity_function(cx, filter_map_arg) {
+    if is_trait_method(cx, expr, sym::Iterator) && is_expr_untyped_identity_function(cx, filter_map_arg) {
         span_lint_and_sugg(
             cx,
             FILTER_MAP_IDENTITY,

clippy_lints/src/methods/flat_map_identity.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::{is_expr_identity_function, is_trait_method};
+use clippy_utils::{is_expr_untyped_identity_function, is_trait_method};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -15,7 +15,7 @@ pub(super) fn check<'tcx>(
     flat_map_arg: &'tcx hir::Expr<'_>,
     flat_map_span: Span,
 ) {
-    if is_trait_method(cx, expr, sym::Iterator) && is_expr_identity_function(cx, flat_map_arg) {
+    if is_trait_method(cx, expr, sym::Iterator) && is_expr_untyped_identity_function(cx, flat_map_arg) {
         span_lint_and_sugg(
             cx,
             FLAT_MAP_IDENTITY,

clippy_lints/src/methods/map_identity.rs

@@ -1,6 +1,6 @@
 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 clippy_utils::{is_expr_untyped_identity_function, is_trait_method};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -23,7 +23,7 @@ pub(super) fn check(
         if is_trait_method(cx, expr, sym::Iterator)
             || is_type_diagnostic_item(cx, caller_ty, sym::Result)
             || is_type_diagnostic_item(cx, caller_ty, sym::Option);
-        if is_expr_identity_function(cx, map_arg);
+        if is_expr_untyped_identity_function(cx, map_arg);
         if let Some(sugg_span) = expr.span.trim_start(caller.span);
         then {
             span_lint_and_sugg(

clippy_utils/src/lib.rs

@@ -2027,36 +2027,31 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     did.map_or(false, |did| cx.tcx.has_attr(did, sym::must_use))
 }
 
-/// Checks if an expression represents the identity function
-/// Only examines closures and `std::convert::identity`
+/// Checks if a function's body represents the identity function. Looks for bodies of the form:
+/// * `|x| x`
+/// * `|x| return x`
+/// * `|x| { return x }`
+/// * `|x| { return x; }`
 ///
-/// Closure bindings with type annotations and `std::convert::identity` with generic args
-/// are not considered identity functions because they can guide type inference,
-/// and removing it may lead to compile errors.
-pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    /// Checks if a function's body represents the identity function. Looks for bodies of the form:
-    /// * `|x| x`
-    /// * `|x| return x`
-    /// * `|x| { return x }`
-    /// * `|x| { return x; }`
-    fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool {
-        let id = if_chain! {
-            if let [param] = func.params;
-            if let PatKind::Binding(_, id, _, _) = param.pat.kind;
-            then {
-                id
-            } else {
-                return false;
-            }
-        };
+/// Consider calling [`is_expr_untyped_identity_function`] or [`is_expr_identity_function`] instead.
+fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool {
+    let id = if_chain! {
+        if let [param] = func.params;
+        if let PatKind::Binding(_, id, _, _) = param.pat.kind;
+        then {
+            id
+        } else {
+            return false;
+        }
+    };
 
-        let mut expr = func.value;
-        loop {
-            match expr.kind {
-                #[rustfmt::skip]
+    let mut expr = func.value;
+    loop {
+        match expr.kind {
+            #[rustfmt::skip]
                 ExprKind::Block(&Block { stmts: [], expr: Some(e), .. }, _, )
                 | ExprKind::Ret(Some(e)) => expr = e,
-                #[rustfmt::skip]
+            #[rustfmt::skip]
                 ExprKind::Block(&Block { stmts: [stmt], expr: None, .. }, _) => {
                     if_chain! {
                         if let StmtKind::Semi(e) | StmtKind::Expr(e) = stmt.kind;
@@ -2068,11 +2063,16 @@ pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool
                         }
                     }
                 },
-                _ => return path_to_local_id(expr, id) && cx.typeck_results().expr_adjustments(expr).is_empty(),
-            }
+            _ => return path_to_local_id(expr, id) && cx.typeck_results().expr_adjustments(expr).is_empty(),
         }
     }
+}
 
+/// This is the same as [`is_expr_identity_function`], but does not consider closures
+/// with type annotations for its bindings (or similar) as identity functions:
+/// * `|x: u8| x`
+/// * `std::convert::identity::<u8>`
+pub fn is_expr_untyped_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     match expr.kind {
         ExprKind::Closure(&Closure { body, fn_decl, .. })
             if fn_decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer)) =>
@@ -2089,6 +2089,21 @@ pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool
     }
 }
 
+/// Checks if an expression represents the identity function
+/// Only examines closures and `std::convert::identity`
+///
+/// NOTE: If you want to use this function to find out if a closure is unnecessary, you likely want
+/// to call [`is_expr_untyped_identity_function`] instead, which makes sure that the closure doesn't
+/// have type annotations. This is important because removing a closure with bindings can
+/// remove type information that helped type inference before, which can then lead to compile
+/// errors.
+pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    match expr.kind {
+        ExprKind::Closure(&Closure { body, .. }) => is_body_identity_function(cx, cx.tcx.hir().body(body)),
+        _ => path_def_id(cx, expr).map_or(false, |id| match_def_path(cx, id, &paths::CONVERT_IDENTITY)),
+    }
+}
+
 /// Gets the node where an expression is either used, or it's type is unified with another branch.
 /// Returns both the node and the `HirId` of the closest child node.
 pub fn get_expr_use_or_unification_node<'tcx>(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<(Node<'tcx>, HirId)> {