diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index 5298e16a04d9..2e6bf763a87e 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/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| ...)`", + ); + } } } } diff --git a/tests/ui/map_err.rs b/tests/ui/map_err.rs index 05b9949f1021..13686e5dba17 100644 --- a/tests/ui/map_err.rs +++ b/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(()) } diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr index 390d7ce2e4e7..79b531e115da 100644 --- a/tests/ui/map_err.stderr +++ b/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