From 2f0ed0a0b1fa7928464b95e0cfd0883699cb9c33 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 5 Jun 2022 15:59:24 -0400 Subject: [PATCH] Move `MapErrIgnore` into `Methods` lint pass --- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.register_restriction.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/map_err_ignore.rs | 154 ------------------- clippy_lints/src/methods/map_err_ignore.rs | 34 ++++ clippy_lints/src/methods/mod.rs | 104 +++++++++++++ 6 files changed, 140 insertions(+), 158 deletions(-) delete mode 100644 clippy_lints/src/map_err_ignore.rs create mode 100644 clippy_lints/src/methods/map_err_ignore.rs diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c7ea7f703..67fb0d509 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -246,7 +246,6 @@ store.register_lints(&[ manual_rem_euclid::MANUAL_REM_EUCLID, manual_retain::MANUAL_RETAIN, manual_strip::MANUAL_STRIP, - map_err_ignore::MAP_ERR_IGNORE, map_unit_fn::OPTION_MAP_UNIT_FN, map_unit_fn::RESULT_MAP_UNIT_FN, match_result_ok::MATCH_RESULT_OK, @@ -326,6 +325,7 @@ store.register_lints(&[ methods::MANUAL_STR_REPEAT, methods::MAP_CLONE, methods::MAP_COLLECT_RESULT_UNIT, + methods::MAP_ERR_IGNORE, methods::MAP_FLATTEN, methods::MAP_IDENTITY, methods::MAP_UNWRAP_OR, diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index 890ae2792..7fc5eef5f 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -30,7 +30,6 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(large_include_file::LARGE_INCLUDE_FILE), LintId::of(let_underscore::LET_UNDERSCORE_MUST_USE), LintId::of(literal_representation::DECIMAL_LITERAL_REPRESENTATION), - LintId::of(map_err_ignore::MAP_ERR_IGNORE), LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS), LintId::of(matches::TRY_ERR), LintId::of(matches::WILDCARD_ENUM_MATCH_ARM), @@ -39,6 +38,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(methods::EXPECT_USED), LintId::of(methods::FILETYPE_IS_FILE), LintId::of(methods::GET_UNWRAP), + LintId::of(methods::MAP_ERR_IGNORE), LintId::of(methods::UNWRAP_USED), LintId::of(misc_early::SEPARATED_LITERAL_SUFFIX), LintId::of(misc_early::UNNEEDED_FIELD_PATTERN), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8f31e4b93..179d63008 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -273,7 +273,6 @@ mod manual_non_exhaustive; mod manual_rem_euclid; mod manual_retain; mod manual_strip; -mod map_err_ignore; mod map_unit_fn; mod match_result_ok; mod matches; @@ -636,7 +635,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: msrv, )) }); - store.register_late_pass(|| Box::new(map_err_ignore::MapErrIgnore)); store.register_late_pass(|| Box::new(shadow::Shadow::default())); store.register_late_pass(|| Box::new(unit_types::UnitTypes)); store.register_late_pass(|| Box::new(loops::Loops)); diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs deleted file mode 100644 index 1e542447c..000000000 --- a/clippy_lints/src/map_err_ignore.rs +++ /dev/null @@ -1,154 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_help; -use rustc_hir::{CaptureBy, Closure, Expr, ExprKind, PatKind}; -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 contain and report the cause of the error - /// - /// ### Example - /// Before: - /// ```rust - /// use std::fmt; - /// - /// #[derive(Debug)] - /// 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 - /// ), - /// } - /// } - /// } - /// - /// impl std::error::Error for Error {} - /// - /// 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::{fmt, num::ParseIntError}; - /// - /// #[derive(Debug)] - /// enum Error { - /// Indivisible(ParseIntError), - /// 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 - /// ), - /// } - /// } - /// } - /// - /// 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(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)) - /// } - /// }) - /// } - /// ``` - #[clippy::version = "1.48.0"] - pub MAP_ERR_IGNORE, - restriction, - "`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(method, [_, arg], _) = 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.name == sym!(map_err) { - // make sure the first argument is a closure, and grab the CaptureRef, BodyId, and fn_decl_span - // fields - if let ExprKind::Closure(&Closure { - capture_clause, - body, - fn_decl_span, - .. - }) = arg.kind - { - // check if this is by Reference (meaning there's no move statement) - if capture_clause == CaptureBy::Ref { - // Get the closure body to check the parameters and values - let closure_body = cx.tcx.hir().body(body); - // 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 { - // 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, - fn_decl_span, - "`map_err(|_|...` wildcard pattern discards the original error", - None, - "consider storing the original error as a source in the new error, or silence this warning using an ignored identifier (`.map_err(|_foo| ...`)", - ); - } - } - } - } - } - } - } -} diff --git a/clippy_lints/src/methods/map_err_ignore.rs b/clippy_lints/src/methods/map_err_ignore.rs new file mode 100644 index 000000000..1fb661714 --- /dev/null +++ b/clippy_lints/src/methods/map_err_ignore.rs @@ -0,0 +1,34 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_hir::{CaptureBy, Closure, Expr, ExprKind, PatKind}; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::MAP_ERR_IGNORE; + +pub(super) fn check<'tcx>(cx: &LateContext<'_>, e: &Expr<'_>, arg: &'tcx Expr<'_>) { + if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) + && let Some(impl_id) = cx.tcx.impl_of_method(method_id) + && is_type_diagnostic_item(cx, cx.tcx.type_of(impl_id), sym::Result) + && let ExprKind::Closure(&Closure { + capture_clause: CaptureBy::Ref, + body, + fn_decl_span, + .. + }) = arg.kind + && let closure_body = cx.tcx.hir().body(body) + && let [param] = closure_body.params + && let PatKind::Wild = param.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, + fn_decl_span, + "`map_err(|_|...` wildcard pattern discards the original error", + None, + "consider storing the original error as a source in the new error, or silence this warning using an ignored identifier (`.map_err(|_foo| ...`)", + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 02592da1a..e327bdcf5 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -47,6 +47,7 @@ mod manual_saturating_arithmetic; mod manual_str_repeat; mod map_clone; mod map_collect_result_unit; +mod map_err_ignore; mod map_flatten; mod map_identity; mod map_unwrap_or; @@ -2541,6 +2542,106 @@ declare_clippy_lint! { "using `iterator.map(|x| x.clone())`, or dereferencing closures for `Copy` types" } +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 contain and report the cause of the error + /// + /// ### Example + /// Before: + /// ```rust + /// use std::fmt; + /// + /// #[derive(Debug)] + /// 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 + /// ), + /// } + /// } + /// } + /// + /// impl std::error::Error for Error {} + /// + /// 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::{fmt, num::ParseIntError}; + /// + /// #[derive(Debug)] + /// enum Error { + /// Indivisible(ParseIntError), + /// 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 + /// ), + /// } + /// } + /// } + /// + /// 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(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)) + /// } + /// }) + /// } + /// ``` + #[clippy::version = "1.48.0"] + pub MAP_ERR_IGNORE, + restriction, + "`map_err` should not ignore the original error" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -2651,6 +2752,7 @@ impl_lint_pass!(Methods => [ GET_FIRST, MANUAL_OK_OR, MAP_CLONE, + MAP_ERR_IGNORE, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -2982,6 +3084,8 @@ impl Methods { (name @ ("map" | "map_err"), [m_arg]) => { if name == "map" { map_clone::check(cx, expr, recv, m_arg, self.msrv); + } else { + map_err_ignore::check(cx, expr, m_arg); } if let Some((name, [recv2, args @ ..], span2)) = method_call(recv) { match (name, args) {