Auto merge of #11521 - y21:issue9122, r=llogiq

[`map_identity`]: allow closure with type annotations

Fixes #9122

`.map(|a: u32| a)` can help type inference, so we should probably allow this and not warn about "unnecessary map of the identity function"

changelog: [`map_identity`]: allow closure with type annotations

Author: bors

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,32 +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`
-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;
-            }
-        };
+/// 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; }`
+///
+/// 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;
@@ -2064,11 +2063,41 @@ 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)) =>
+        {
+            is_body_identity_function(cx, cx.tcx.hir().body(body))
+        },
+        ExprKind::Path(QPath::Resolved(_, path))
+            if path.segments.iter().all(|seg| seg.infer_args)
+                && let Some(did) = path.res.opt_def_id() =>
+        {
+            match_def_path(cx, did, &paths::CONVERT_IDENTITY)
+        },
+        _ => false,
+    }
+}
+
+/// 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)),

tests/ui/map_identity.fixed

@@ -17,6 +17,9 @@ fn main() {
     });
     let _: Result<u32, u32> = Ok(1);
     let _: Result<u32, u32> = Ok(1).map_err(|a: u32| a * 42);
+    // : u32 guides type inference
+    let _ = Ok(1).map_err(|a: u32| a);
+    let _ = Ok(1).map_err(std::convert::identity::<u32>);
 }
 
 fn not_identity(x: &u16) -> u16 {

tests/ui/map_identity.rs

@@ -19,6 +19,9 @@ fn main() {
     });
     let _: Result<u32, u32> = Ok(1).map_err(|a| a);
     let _: Result<u32, u32> = Ok(1).map_err(|a: u32| a * 42);
+    // : u32 guides type inference
+    let _ = Ok(1).map_err(|a: u32| a);
+    let _ = Ok(1).map_err(std::convert::identity::<u32>);
 }
 
 fn not_identity(x: &u16) -> u16 {