From e49a29933be3bd988ccb75b053f480d9c99a7ff5 Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 1 Sep 2020 16:26:59 -0400 Subject: [PATCH 1/7] Working map_err_ignore lint --- clippy_lints/src/lib.rs | 5 ++ clippy_lints/src/map_err_ignore.rs | 108 +++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 clippy_lints/src/map_err_ignore.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0eb1d3313660..8e80779377bc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -230,6 +230,7 @@ mod main_recursion; mod manual_async_fn; mod manual_non_exhaustive; mod map_clone; +mod map_err_ignore; mod map_identity; mod map_unit_fn; mod match_on_vec_items; @@ -624,6 +625,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &manual_async_fn::MANUAL_ASYNC_FN, &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, &map_clone::MAP_CLONE, + &map_err_ignore::MAP_ERR_IGNORE, &map_identity::MAP_IDENTITY, &map_unit_fn::OPTION_MAP_UNIT_FN, &map_unit_fn::RESULT_MAP_UNIT_FN, @@ -916,6 +918,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub); store.register_late_pass(|| box methods::Methods); store.register_late_pass(|| box map_clone::MapClone); + store.register_late_pass(|| box map_err_ignore::MapErrIgnore); store.register_late_pass(|| box shadow::Shadow); store.register_late_pass(|| box types::LetUnitValue); store.register_late_pass(|| box types::UnitCmp); @@ -1327,6 +1330,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), + LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&map_identity::MAP_IDENTITY), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), @@ -1534,6 +1538,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), + LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(&matches::MATCH_OVERLAPPING_ARM), diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs new file mode 100644 index 000000000000..c63c201a9f35 --- /dev/null +++ b/clippy_lints/src/map_err_ignore.rs @@ -0,0 +1,108 @@ +use crate::utils::span_lint_and_sugg; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, CaptureBy, PatKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for instances of `map_err(|_| Some::Enum)` + /// + /// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to bubble the original error + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// enum Errors { + /// Ignore + ///} + ///fn main() -> Result<(), Errors> { + /// + /// let x = u32::try_from(-123_i32); + /// + /// println!("{:?}", x.map_err(|_| Errors::Ignore)); + /// + /// Ok(()) + ///} + /// ``` + /// Use instead: + /// ```rust + /// enum Errors { + /// WithContext(TryFromIntError) + ///} + ///fn main() -> Result<(), Errors> { + /// + /// let x = u32::try_from(-123_i32); + /// + /// println!("{:?}", x.map_err(|e| Errors::WithContext(e))); + /// + /// Ok(()) + ///} + /// ``` + pub MAP_ERR_IGNORE, + style, + "`map_err` should not ignore the original error" +} + +declare_lint_pass!(MapErrIgnore => [MAP_ERR_IGNORE]); + +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<'_>) { + if e.span.from_expansion() { + return; + } + + // check if this is a method call (e.g. x.foo()) + if let ExprKind::MethodCall(ref method, _t_span, ref args, _) = e.kind { + // only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1] Enum::Variant[2])) + if method.ident.as_str() == "map_err" && args.len() == 2 { + // make sure the first argument is a closure, and grab the CaptureRef, body_id, and body_span fields + if let ExprKind::Closure(capture, _, body_id, body_span, _) = args[1].kind { + // check if this is by Reference (meaning there's no move statement) + if capture == CaptureBy::Ref { + // Get the closure body to check the parameters and values + let closure_body = cx.tcx.hir().body(body_id); + // make sure there's only one parameter (`|_|`) + 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 closure to see if we can build the enum we are throwing away the error for + // make sure this is a Path + if let ExprKind::Path(q_path) = &closure_body.value.kind { + // this should be a resolved path, only keep the path field + if let QPath::Resolved(_, path) = q_path { + // finally get the idents for each path segment collect them as a string and join them with the path separator ("::"") + let closure_fold: String = path.segments.iter().map(|x| x.ident.as_str().to_string()).collect::>().join("::"); + //Span the body of the closure (the |...| bit) and suggest the fix by taking the error and encapsulating it in the enum + span_lint_and_sugg( + cx, + MAP_ERR_IGNORE, + body_span, + "`map_err` has thrown away the original error", + "Allow the error enum to encapsulate the original error", + format!("|e| {}(e)", closure_fold), + Applicability::HasPlaceholders, + ); + } + } else { + //If we cannot build the enum in a human readable way just suggest not throwing way the error + span_lint_and_sugg( + cx, + MAP_ERR_IGNORE, + body_span, + "`map_err` has thrown away the original error", + "Allow the error enum to encapsulate the original error", + "|e|".to_string(), + Applicability::HasPlaceholders, + ); + } + } + } + } + } + } + } + } +} \ No newline at end of file From 202a80c927412a548180eca5990ee764d76ed643 Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 1 Sep 2020 16:59:37 -0400 Subject: [PATCH 2/7] Added tests for map_err, ignored map_err lint on drop_ref tests --- clippy_lints/src/map_err_ignore.rs | 49 ++++++++++++++++++------------ tests/ui/drop_ref.rs | 1 + tests/ui/drop_ref.stderr | 36 +++++++++++----------- tests/ui/map_err.rs | 24 +++++++++++++++ tests/ui/map_err.stderr | 10 ++++++ 5 files changed, 82 insertions(+), 38 deletions(-) create mode 100644 tests/ui/map_err.rs create mode 100644 tests/ui/map_err.stderr diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index c63c201a9f35..43bfcf0b8f14 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_sugg; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, CaptureBy, PatKind, QPath}; +use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -18,7 +18,7 @@ declare_clippy_lint! { /// Ignore ///} ///fn main() -> Result<(), Errors> { - /// + /// /// let x = u32::try_from(-123_i32); /// /// println!("{:?}", x.map_err(|_| Errors::Ignore)); @@ -32,7 +32,7 @@ declare_clippy_lint! { /// WithContext(TryFromIntError) ///} ///fn main() -> Result<(), Errors> { - /// + /// /// let x = u32::try_from(-123_i32); /// /// println!("{:?}", x.map_err(|e| Errors::WithContext(e))); @@ -48,7 +48,7 @@ declare_clippy_lint! { declare_lint_pass!(MapErrIgnore => [MAP_ERR_IGNORE]); impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { - // do not try to lint if this is from a macro or desugaring + // do not try to lint if this is from a macro or desugaring fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { if e.span.from_expansion() { return; @@ -56,26 +56,34 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { // check if this is a method call (e.g. x.foo()) if let ExprKind::MethodCall(ref method, _t_span, ref args, _) = e.kind { - // only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1] Enum::Variant[2])) + // only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1] + // Enum::Variant[2])) if method.ident.as_str() == "map_err" && args.len() == 2 { // make sure the first argument is a closure, and grab the CaptureRef, body_id, and body_span fields - if let ExprKind::Closure(capture, _, body_id, body_span, _) = args[1].kind { + if let ExprKind::Closure(capture, _, body_id, body_span, _) = args[1].kind { // check if this is by Reference (meaning there's no move statement) - if capture == CaptureBy::Ref { - // Get the closure body to check the parameters and values + if capture == CaptureBy::Ref { + // Get the closure body to check the parameters and values let closure_body = cx.tcx.hir().body(body_id); // make sure there's only one parameter (`|_|`) - if closure_body.params.len() == 1 { - // make sure that parameter is the wild token (`_`) + 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 closure to see if we can build the enum we are throwing away the error for - // make sure this is a Path + // Check the value of the closure to see if we can build the enum we are throwing away + // the error for make sure this is a Path if let ExprKind::Path(q_path) = &closure_body.value.kind { // this should be a resolved path, only keep the path field if let QPath::Resolved(_, path) = q_path { - // finally get the idents for each path segment collect them as a string and join them with the path separator ("::"") - let closure_fold: String = path.segments.iter().map(|x| x.ident.as_str().to_string()).collect::>().join("::"); - //Span the body of the closure (the |...| bit) and suggest the fix by taking the error and encapsulating it in the enum + // finally get the idents for each path segment collect them as a string and + // join them with the path separator ("::"") + let closure_fold: String = path + .segments + .iter() + .map(|x| x.ident.as_str().to_string()) + .collect::>() + .join("::"); + //Span the body of the closure (the |...| bit) and suggest the fix by taking + // the error and encapsulating it in the enum span_lint_and_sugg( cx, MAP_ERR_IGNORE, @@ -84,10 +92,11 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { "Allow the error enum to encapsulate the original error", format!("|e| {}(e)", closure_fold), Applicability::HasPlaceholders, - ); + ); } } else { - //If we cannot build the enum in a human readable way just suggest not throwing way the error + //If we cannot build the enum in a human readable way just suggest not throwing way + // the error span_lint_and_sugg( cx, MAP_ERR_IGNORE, @@ -96,13 +105,13 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { "Allow the error enum to encapsulate the original error", "|e|".to_string(), Applicability::HasPlaceholders, - ); + ); } } } - } + } } } } } -} \ No newline at end of file +} diff --git a/tests/ui/drop_ref.rs b/tests/ui/drop_ref.rs index 9181d789d4fb..6b5bcdaa78e2 100644 --- a/tests/ui/drop_ref.rs +++ b/tests/ui/drop_ref.rs @@ -1,5 +1,6 @@ #![warn(clippy::drop_ref)] #![allow(clippy::toplevel_ref_arg)] +#![allow(clippy::map_err_ignore)] use std::mem::drop; diff --git a/tests/ui/drop_ref.stderr b/tests/ui/drop_ref.stderr index 35ae88b78a4c..7974bf56d44c 100644 --- a/tests/ui/drop_ref.stderr +++ b/tests/ui/drop_ref.stderr @@ -1,108 +1,108 @@ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:9:5 + --> $DIR/drop_ref.rs:10:5 | LL | drop(&SomeStruct); | ^^^^^^^^^^^^^^^^^ | = note: `-D clippy::drop-ref` implied by `-D warnings` note: argument has type `&SomeStruct` - --> $DIR/drop_ref.rs:9:10 + --> $DIR/drop_ref.rs:10:10 | LL | drop(&SomeStruct); | ^^^^^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:12:5 + --> $DIR/drop_ref.rs:13:5 | LL | drop(&owned1); | ^^^^^^^^^^^^^ | note: argument has type `&SomeStruct` - --> $DIR/drop_ref.rs:12:10 + --> $DIR/drop_ref.rs:13:10 | LL | drop(&owned1); | ^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:13:5 + --> $DIR/drop_ref.rs:14:5 | LL | drop(&&owned1); | ^^^^^^^^^^^^^^ | note: argument has type `&&SomeStruct` - --> $DIR/drop_ref.rs:13:10 + --> $DIR/drop_ref.rs:14:10 | LL | drop(&&owned1); | ^^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:14:5 + --> $DIR/drop_ref.rs:15:5 | LL | drop(&mut owned1); | ^^^^^^^^^^^^^^^^^ | note: argument has type `&mut SomeStruct` - --> $DIR/drop_ref.rs:14:10 + --> $DIR/drop_ref.rs:15:10 | LL | drop(&mut owned1); | ^^^^^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:18:5 + --> $DIR/drop_ref.rs:19:5 | LL | drop(reference1); | ^^^^^^^^^^^^^^^^ | note: argument has type `&SomeStruct` - --> $DIR/drop_ref.rs:18:10 + --> $DIR/drop_ref.rs:19:10 | LL | drop(reference1); | ^^^^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:21:5 + --> $DIR/drop_ref.rs:22:5 | LL | drop(reference2); | ^^^^^^^^^^^^^^^^ | note: argument has type `&mut SomeStruct` - --> $DIR/drop_ref.rs:21:10 + --> $DIR/drop_ref.rs:22:10 | LL | drop(reference2); | ^^^^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:24:5 + --> $DIR/drop_ref.rs:25:5 | LL | drop(reference3); | ^^^^^^^^^^^^^^^^ | note: argument has type `&SomeStruct` - --> $DIR/drop_ref.rs:24:10 + --> $DIR/drop_ref.rs:25:10 | LL | drop(reference3); | ^^^^^^^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:29:5 + --> $DIR/drop_ref.rs:30:5 | LL | drop(&val); | ^^^^^^^^^^ | note: argument has type `&T` - --> $DIR/drop_ref.rs:29:10 + --> $DIR/drop_ref.rs:30:10 | LL | drop(&val); | ^^^^ error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing. - --> $DIR/drop_ref.rs:37:5 + --> $DIR/drop_ref.rs:38:5 | LL | std::mem::drop(&SomeStruct); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: argument has type `&SomeStruct` - --> $DIR/drop_ref.rs:37:20 + --> $DIR/drop_ref.rs:38:20 | LL | std::mem::drop(&SomeStruct); | ^^^^^^^^^^^ diff --git a/tests/ui/map_err.rs b/tests/ui/map_err.rs new file mode 100644 index 000000000000..f3a74ad95cd2 --- /dev/null +++ b/tests/ui/map_err.rs @@ -0,0 +1,24 @@ +use std::convert::TryFrom; +use std::error::Error; +use std::fmt; + +#[derive(Debug)] +enum Errors { + Ignored, +} + +impl Error for Errors {} + +impl fmt::Display for Errors { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Error") + } +} + +fn main() -> Result<(), Errors> { + let x = u32::try_from(-123_i32); + + println!("{:?}", x.map_err(|_| Errors::Ignored)); + + Ok(()) +} diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr new file mode 100644 index 000000000000..8cd37d8c0252 --- /dev/null +++ b/tests/ui/map_err.stderr @@ -0,0 +1,10 @@ +error: `map_err` has thrown away the original error + --> $DIR/map_err.rs:21:32 + | +LL | println!("{:?}", x.map_err(|_| Errors::Ignored)); + | ^^^ help: Allow the error enum to encapsulate the original error: `|e| Errors::Ignored(e)` + | + = note: `-D clippy::map-err-ignore` implied by `-D warnings` + +error: aborting due to previous error + From 337729137bdec31b55665653ef0cf7dc124b0eaa Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 1 Sep 2020 17:05:40 -0400 Subject: [PATCH 3/7] Ran cargo dev update_lints --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 2 +- src/lintlist/mod.rs | 7 +++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99a8b1a6293c..dfe45a46f092 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1675,6 +1675,7 @@ Released 2018-09-13 [`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone [`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry +[`map_err_ignore`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_err_ignore [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten [`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity [`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8e80779377bc..3794cae091a4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1538,7 +1538,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), - LintId::of(&map_err_ignore::MAP_ERR_IGNORE), + LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(&matches::MATCH_OVERLAPPING_ARM), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index dff19ef440f3..6725c97f7938 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1165,6 +1165,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "entry", }, + Lint { + name: "map_err_ignore", + group: "style", + desc: "`map_err` should not ignore the original error", + deprecation: None, + module: "map_err_ignore", + }, Lint { name: "map_flatten", group: "pedantic", From 2387f68e437bf2ff5f117f63936257ce64052cfa Mon Sep 17 00:00:00 2001 From: Ricky Date: Wed, 2 Sep 2020 19:21:34 -0400 Subject: [PATCH 4/7] Removed map_err suggestion in lint, and updated lint documentation example --- clippy_lints/src/map_err_ignore.rs | 117 ++++++++++++++--------------- tests/ui/map_err.stderr | 5 +- 2 files changed, 60 insertions(+), 62 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index 43bfcf0b8f14..9211113ed046 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_sugg; -use rustc_errors::Applicability; -use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind, QPath}; +use crate::utils::span_lint_and_help; + +use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -12,33 +12,58 @@ declare_clippy_lint! { /// **Known problems:** None. /// /// **Example:** - /// + /// Before: /// ```rust + /// use std::convert::TryFrom; + /// + /// #[derive(Debug)] /// enum Errors { - /// Ignore - ///} - ///fn main() -> Result<(), Errors> { + /// Ignored + /// } /// - /// let x = u32::try_from(-123_i32); + /// fn divisible_by_3(inp: i32) -> Result { + /// let i = u32::try_from(inp).map_err(|_| Errors::Ignored)?; /// - /// println!("{:?}", x.map_err(|_| Errors::Ignore)); + /// Ok(i) + /// } + /// ``` /// - /// Ok(()) - ///} - /// ``` - /// Use instead: - /// ```rust - /// enum Errors { - /// WithContext(TryFromIntError) - ///} - ///fn main() -> Result<(), Errors> { + /// After: + /// ```rust + /// use std::convert::TryFrom; + /// use std::num::TryFromIntError; + /// use std::fmt; + /// use std::error::Error; + /// + /// #[derive(Debug)] + /// enum ParseError { + /// Indivisible { + /// source: TryFromIntError, + /// input: String, + /// } + /// } + /// + /// impl fmt::Display for ParseError { + /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + /// match &self { + /// ParseError::Indivisible{source: _, input} => write!(f, "Error: {}", input) + /// } + /// } + /// } + /// + /// impl Error for ParseError {} /// - /// let x = u32::try_from(-123_i32); + /// impl ParseError { + /// fn new(source: TryFromIntError, input: String) -> ParseError { + /// ParseError::Indivisible{source, input} + /// } + /// } /// - /// println!("{:?}", x.map_err(|e| Errors::WithContext(e))); + /// fn divisible_by_3(inp: i32) -> Result { + /// let i = u32::try_from(inp).map_err(|e| ParseError::new(e, e.to_string()))?; /// - /// Ok(()) - ///} + /// Ok(i) + /// } /// ``` pub MAP_ERR_IGNORE, style, @@ -69,44 +94,16 @@ 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 closure to see if we can build the enum we are throwing away - // the error for make sure this is a Path - if let ExprKind::Path(q_path) = &closure_body.value.kind { - // this should be a resolved path, only keep the path field - if let QPath::Resolved(_, path) = q_path { - // finally get the idents for each path segment collect them as a string and - // join them with the path separator ("::"") - let closure_fold: String = path - .segments - .iter() - .map(|x| x.ident.as_str().to_string()) - .collect::>() - .join("::"); - //Span the body of the closure (the |...| bit) and suggest the fix by taking - // the error and encapsulating it in the enum - span_lint_and_sugg( - cx, - MAP_ERR_IGNORE, - body_span, - "`map_err` has thrown away the original error", - "Allow the error enum to encapsulate the original error", - format!("|e| {}(e)", closure_fold), - Applicability::HasPlaceholders, - ); - } - } else { - //If we cannot build the enum in a human readable way just suggest not throwing way - // the error - span_lint_and_sugg( - cx, - MAP_ERR_IGNORE, - body_span, - "`map_err` has thrown away the original error", - "Allow the error enum to encapsulate the original error", - "|e|".to_string(), - Applicability::HasPlaceholders, - ); - } + // 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_else(|_|...` ignores the original error", + None, + "Consider wrapping the error in an enum variant", + ); } } } diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr index 8cd37d8c0252..7a269ab95ab2 100644 --- a/tests/ui/map_err.stderr +++ b/tests/ui/map_err.stderr @@ -1,10 +1,11 @@ -error: `map_err` has thrown away the original error +error: `map_else(|_|...` ignores the original error --> $DIR/map_err.rs:21:32 | LL | println!("{:?}", x.map_err(|_| Errors::Ignored)); - | ^^^ help: Allow the error enum to encapsulate the original error: `|e| Errors::Ignored(e)` + | ^^^ | = note: `-D clippy::map-err-ignore` implied by `-D warnings` + = help: Consider wrapping the error in an enum variant error: aborting due to previous error From c31d4730b0d40c62934839405d0c25e2ffa3fad1 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 4 Sep 2020 15:55:13 -0700 Subject: [PATCH 5/7] update example to be more idiomatic --- clippy_lints/src/map_err_ignore.rs | 87 ++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index 9211113ed046..649de4133e06 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/clippy_lints/src/map_err_ignore.rs @@ -14,55 +14,88 @@ declare_clippy_lint! { /// **Example:** /// Before: /// ```rust - /// use std::convert::TryFrom; + /// use std::fmt; /// /// #[derive(Debug)] - /// enum Errors { - /// Ignored + /// enum Error { + /// Indivisible, + /// Remainder(u8), + /// } + /// + /// impl fmt::Display for Error { + /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + /// match self { + /// Error::Indivisible => write!(f, "could not divide input by three"), + /// Error::Remainder(remainder) => write!( + /// f, + /// "input is not divisible by three, remainder = {}", + /// remainder + /// ), + /// } + /// } /// } /// - /// fn divisible_by_3(inp: i32) -> Result { - /// let i = u32::try_from(inp).map_err(|_| Errors::Ignored)?; + /// impl std::error::Error for Error {} /// - /// Ok(i) + /// fn divisible_by_3(input: &str) -> Result<(), Error> { + /// input + /// .parse::() + /// .map_err(|_| Error::Indivisible) + /// .map(|v| v % 3) + /// .and_then(|remainder| { + /// if remainder == 0 { + /// Ok(()) + /// } else { + /// Err(Error::Remainder(remainder as u8)) + /// } + /// }) /// } /// ``` /// /// After: /// ```rust - /// use std::convert::TryFrom; - /// use std::num::TryFromIntError; - /// use std::fmt; - /// use std::error::Error; + /// use std::{fmt, num::ParseIntError}; /// /// #[derive(Debug)] - /// enum ParseError { - /// Indivisible { - /// source: TryFromIntError, - /// input: String, - /// } + /// enum Error { + /// Indivisible(ParseIntError), + /// Remainder(u8), /// } /// - /// impl fmt::Display for ParseError { + /// impl fmt::Display for Error { /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - /// match &self { - /// ParseError::Indivisible{source: _, input} => write!(f, "Error: {}", input) + /// match self { + /// Error::Indivisible(_) => write!(f, "could not divide input by three"), + /// Error::Remainder(remainder) => write!( + /// f, + /// "input is not divisible by three, remainder = {}", + /// remainder + /// ), /// } /// } /// } /// - /// impl Error for ParseError {} - /// - /// impl ParseError { - /// fn new(source: TryFromIntError, input: String) -> ParseError { - /// ParseError::Indivisible{source, input} + /// impl std::error::Error for Error { + /// fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + /// match self { + /// Error::Indivisible(source) => Some(source), + /// _ => None, + /// } /// } /// } /// - /// fn divisible_by_3(inp: i32) -> Result { - /// let i = u32::try_from(inp).map_err(|e| ParseError::new(e, e.to_string()))?; - /// - /// Ok(i) + /// fn divisible_by_3(input: &str) -> Result<(), Error> { + /// input + /// .parse::() + /// .map_err(Error::Indivisible) + /// .map(|v| v % 3) + /// .and_then(|remainder| { + /// if remainder == 0 { + /// Ok(()) + /// } else { + /// Err(Error::Remainder(remainder as u8)) + /// } + /// }) /// } /// ``` pub MAP_ERR_IGNORE, From 4f1c4a99d4d98f1acea3c9c7cc55355aa46119aa Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 8 Sep 2020 12:05:18 -0400 Subject: [PATCH 6/7] Fixed typo in lint and test --- clippy_lints/src/map_err_ignore.rs | 4 ++-- tests/ui/map_err.stderr | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index 649de4133e06..fa59b6ec3706 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/clippy_lints/src/map_err_ignore.rs @@ -7,7 +7,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { /// **What it does:** Checks for instances of `map_err(|_| Some::Enum)` /// - /// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to bubble the original error + /// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to contain and report the cause of the error /// /// **Known problems:** None. /// @@ -133,7 +133,7 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { cx, MAP_ERR_IGNORE, body_span, - "`map_else(|_|...` ignores the original error", + "`map_err(|_|...` ignores the original error", None, "Consider wrapping the error in an enum variant", ); diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr index 7a269ab95ab2..e4e6a289ba03 100644 --- a/tests/ui/map_err.stderr +++ b/tests/ui/map_err.stderr @@ -1,4 +1,4 @@ -error: `map_else(|_|...` ignores the original error +error: `map_err(|_|...` ignores the original error --> $DIR/map_err.rs:21:32 | LL | println!("{:?}", x.map_err(|_| Errors::Ignored)); From d719b485434eac557e65bf55cca79e63f7b83d5b Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 8 Sep 2020 19:37:14 -0400 Subject: [PATCH 7/7] Move map_err_ignore from style to pedantic --- clippy_lints/src/lib.rs | 3 +-- clippy_lints/src/map_err_ignore.rs | 2 +- src/lintlist/mod.rs | 2 +- tests/ui/map_err.rs | 1 + tests/ui/map_err.stderr | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3794cae091a4..797ab62cb544 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1179,6 +1179,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP), LintId::of(&loops::EXPLICIT_ITER_LOOP), LintId::of(¯o_use::MACRO_USE_IMPORTS), + LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS), LintId::of(&matches::MATCH_BOOL), LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS), @@ -1330,7 +1331,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), - LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&map_identity::MAP_IDENTITY), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), @@ -1538,7 +1538,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), - LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(&matches::MATCH_OVERLAPPING_ARM), diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index fa59b6ec3706..5298e16a04d9 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/clippy_lints/src/map_err_ignore.rs @@ -99,7 +99,7 @@ declare_clippy_lint! { /// } /// ``` pub MAP_ERR_IGNORE, - style, + pedantic, "`map_err` should not ignore the original error" } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6725c97f7938..5105e9531626 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1167,7 +1167,7 @@ pub static ref ALL_LINTS: Vec = vec![ }, Lint { name: "map_err_ignore", - group: "style", + group: "pedantic", desc: "`map_err` should not ignore the original error", deprecation: None, module: "map_err_ignore", diff --git a/tests/ui/map_err.rs b/tests/ui/map_err.rs index f3a74ad95cd2..617b64228726 100644 --- a/tests/ui/map_err.rs +++ b/tests/ui/map_err.rs @@ -1,3 +1,4 @@ +#![warn(clippy::map_err_ignore)] use std::convert::TryFrom; use std::error::Error; use std::fmt; diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr index e4e6a289ba03..7273f4603807 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:21:32 + --> $DIR/map_err.rs:22:32 | LL | println!("{:?}", x.map_err(|_| Errors::Ignored)); | ^^^