From 58cf2d7f3773500afc625f67f82263b27a42be16 Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 24 Nov 2020 11:44:10 -0500 Subject: [PATCH 1/4] Added a check to make sure the body of the closure is a unit enum. Updated map_err.rs ui test to make sure we only fail on '.map_err(|_| Unit::Enum)' --- clippy_lints/src/map_err_ignore.rs | 46 +++++++++++++++++++++++------- tests/ui/map_err.rs | 34 ++++++++++++++++++++++ tests/ui/map_err.stderr | 2 +- 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index 5298e16a04d9..d774683e5092 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::{CaptureBy, Expr, ExprKind, PatKind, QPath, def::Res, def::DefKind, def::CtorKind, def::CtorOf}; 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) => { + match enum_path.res { + // the definition should be a enum constructor with a + // Const (unit) enum variant (and we do not want to match on the `DefId`) + Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), _) => true, + _ => false, + } + }, + // 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", + ); + } } } } diff --git a/tests/ui/map_err.rs b/tests/ui/map_err.rs index 05b9949f1021..9861f47bf45d 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,9 +19,41 @@ 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)); Ok(()) diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr index 390d7ce2e4e7..4d77bdaeaa41 100644 --- a/tests/ui/map_err.stderr +++ b/tests/ui/map_err.stderr @@ -1,5 +1,5 @@ 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)); | ^^^ From 658722a4063319776f74ef9c09117d6618e6b719 Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 24 Nov 2020 12:16:18 -0500 Subject: [PATCH 2/4] cargo dev fmt --- clippy_lints/src/map_err_ignore.rs | 14 +++++++------- tests/ui/map_err.rs | 12 ++++++------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index d774683e5092..01fae88150ec 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, QPath, def::Res, def::DefKind, def::CtorKind, def::CtorOf}; +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}; @@ -111,17 +111,17 @@ fn is_unit_enum_variant(input: &ExprKind<'_>) -> bool { match qpath { QPath::Resolved(None, enum_path) => { match enum_path.res { - // the definition should be a enum constructor with a + // the definition should be a enum constructor with a // Const (unit) enum variant (and we do not want to match on the `DefId`) Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), _) => true, _ => false, } - }, + }, // If this is not a resolved qualified path it isn't a unit enum - _ => false, + _ => false, } - } - // if this expression isn't a path it isn't an enum + }, + // if this expression isn't a path it isn't an enum _ => false, } } @@ -148,7 +148,7 @@ 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 { - // check the value of the body is only a unit enum + // 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 diff --git a/tests/ui/map_err.rs b/tests/ui/map_err.rs index 9861f47bf45d..96e6956a848d 100644 --- a/tests/ui/map_err.rs +++ b/tests/ui/map_err.rs @@ -8,7 +8,7 @@ use std::fmt; enum Errors { Ignored, OwnContext(u32), - WithContext(std::num::TryFromIntError) + WithContext(std::num::TryFromIntError), } impl Error for Errors {} @@ -21,7 +21,7 @@ impl fmt::Display for Errors { #[derive(Debug)] struct SError { - x: u32 + x: u32, } fn test_fn_call() -> u32 { @@ -30,7 +30,7 @@ fn test_fn_call() -> u32 { fn main() -> Result<(), Errors> { let x = u32::try_from(-123_i32); - let y = 0; + 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))); @@ -47,11 +47,11 @@ fn main() -> Result<(), Errors> { // 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 + // 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 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)); From d679c21a161b089322774451bbda75e8510b6003 Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 24 Nov 2020 12:41:43 -0500 Subject: [PATCH 3/4] Fixed dogfood XD --- clippy_lints/src/map_err_ignore.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index 01fae88150ec..a4cd8b12d1d2 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/clippy_lints/src/map_err_ignore.rs @@ -110,12 +110,12 @@ fn is_unit_enum_variant(input: &ExprKind<'_>) -> bool { ExprKind::Path(qpath) => { match qpath { QPath::Resolved(None, enum_path) => { - match enum_path.res { - // the definition should be a enum constructor with a - // Const (unit) enum variant (and we do not want to match on the `DefId`) - Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), _) => true, - _ => false, - } + // 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, From 2c502ffb541b1e9cca47a50209abea40fe2d079b Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 24 Nov 2020 14:21:41 -0500 Subject: [PATCH 4/4] Give the user a hit in the output of the lint that they can name the closure parameter (e.g. _ignored) to convey they are intentionally ignoring the original error and avoid this lint without needing an allow attribute --- clippy_lints/src/map_err_ignore.rs | 2 +- tests/ui/map_err.rs | 3 +++ tests/ui/map_err.stderr | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index a4cd8b12d1d2..2e6bf763a87e 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/clippy_lints/src/map_err_ignore.rs @@ -158,7 +158,7 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { body_span, "`map_err(|_|...` ignores the original error", None, - "Consider wrapping the error in an enum variant", + "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 96e6956a848d..13686e5dba17 100644 --- a/tests/ui/map_err.rs +++ b/tests/ui/map_err.rs @@ -56,5 +56,8 @@ fn main() -> Result<(), Errors> { // 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 4d77bdaeaa41..79b531e115da 100644 --- a/tests/ui/map_err.stderr +++ b/tests/ui/map_err.stderr @@ -5,7 +5,7 @@ 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