Make map_err_ignore lint more precise

clippy_lints/src/map_err_ignore.rs

@@ -1,6 +1,6 @@
 use crate::utils::span_lint_and_help;
 
-use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind};
+use rustc_hir::{def::CtorKind, def::CtorOf, def::DefKind, def::Res, CaptureBy, Expr, ExprKind, PatKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
@@ -105,6 +105,27 @@ declare_clippy_lint! {
 
 declare_lint_pass!(MapErrIgnore => [MAP_ERR_IGNORE]);
 
+fn is_unit_enum_variant(input: &ExprKind<'_>) -> bool {
+    match input {
+        ExprKind::Path(qpath) => {
+            match qpath {
+                QPath::Resolved(None, enum_path) => {
+                    // the definition should be a enum constructor with a
+                    // Const (unit) enum variant (and we do not want to match on the `DefId`)
+                    matches!(
+                        enum_path.res,
+                        Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), _)
+                    )
+                },
+                // If this is not a resolved qualified path it isn't a unit enum
+                _ => false,
+            }
+        },
+        // if this expression isn't a path it isn't an enum
+        _ => false,
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for MapErrIgnore {
     // do not try to lint if this is from a macro or desugaring
     fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
@@ -127,16 +148,19 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore {
                         if closure_body.params.len() == 1 {
                             // make sure that parameter is the wild token (`_`)
                             if let PatKind::Wild = closure_body.params[0].pat.kind {
-                                // span the area of the closure capture and warn that the
-                                // original error will be thrown away
-                                span_lint_and_help(
-                                    cx,
-                                    MAP_ERR_IGNORE,
-                                    body_span,
-                                    "`map_err(|_|...` ignores the original error",
-                                    None,
-                                    "Consider wrapping the error in an enum variant",
-                                );
+                                // check the value of the body is only a unit enum
+                                if is_unit_enum_variant(&closure_body.value.kind) {
+                                    // span the area of the closure capture and warn that the
+                                    // original error will be thrown away
+                                    span_lint_and_help(
+                                        cx,
+                                        MAP_ERR_IGNORE,
+                                        body_span,
+                                        "`map_err(|_|...` ignores the original error",
+                                        None,
+                                        "Consider wrapping the error in an enum variant, or giving the closure parameter a name to intentionally ignore the error (e.g. `.map_err(|_ignored| ...)`",
+                                    );
+                                }
                             }
                         }
                     }

tests/ui/map_err.rs

@@ -7,6 +7,8 @@ use std::fmt;
 #[derive(Debug)]
 enum Errors {
     Ignored,
+    OwnContext(u32),
+    WithContext(std::num::TryFromIntError),
 }
 
 impl Error for Errors {}
@@ -17,10 +19,45 @@ impl fmt::Display for Errors {
     }
 }
 
+#[derive(Debug)]
+struct SError {
+    x: u32,
+}
+
+fn test_fn_call() -> u32 {
+    0
+}
+
 fn main() -> Result<(), Errors> {
     let x = u32::try_from(-123_i32);
+    let y = 0;
+
+    // Should not warn you here, because you are giving context with a non-unit enum variant
+    println!("{:?}", x.map_err(|_| Errors::OwnContext(0)));
+
+    // Should not warn you here, because you are wrapping the context
+    println!("{:?}", x.map_err(Errors::WithContext));
 
+    // Should not warn you here, because you are giving context via a `String`
+    println!("{:?}", x.map_err(|_| "There was an error!"));
+
+    // Should not warn you here, because you are calling a constant for context
+    println!("{:?}", x.map_err(|_| u32::MAX));
+
+    // Should not warn you here, because you are calling a function for context
+    println!("{:?}", x.map_err(|_| test_fn_call()));
+
+    // Should not warn you here, because you are providing a variable for context
+    println!("{:?}", x.map_err(|_| y));
+
+    // Should not warn you here, because you are providing a struct for context
+    println!("{:?}", x.map_err(|_| SError { x: 0 }));
+
+    // Should warn you here because you are just ignoring the original error
     println!("{:?}", x.map_err(|_| Errors::Ignored));
 
+    // Should not warn you because you explicitly ignore the parameter
+    println!("{:?}", x.map_err(|_ignored_no_extra_context| Errors::Ignored));
+
     Ok(())
 }

tests/ui/map_err.stderr

@@ -1,11 +1,11 @@
 error: `map_err(|_|...` ignores the original error
-  --> $DIR/map_err.rs:23:32
+  --> $DIR/map_err.rs:57:32
    |
 LL |     println!("{:?}", x.map_err(|_| Errors::Ignored));
    |                                ^^^
    |
    = note: `-D clippy::map-err-ignore` implied by `-D warnings`
-   = help: Consider wrapping the error in an enum variant
+   = help: Consider wrapping the error in an enum variant, or giving the closure parameter a name to intentionally ignore the error (e.g. `.map_err(|_ignored| ...)`
 
 error: aborting due to previous error