From f7271936ddfd78f5f3f534b12d73e22dbfc78ed1 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 10 Jan 2021 09:46:03 -0500 Subject: [PATCH] Add: option_manual_map lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 5 + clippy_lints/src/methods/mod.rs | 5 +- clippy_lints/src/option_manual_map.rs | 175 ++++++++++++++++++++++++++ tests/ui/option_manual_map.fixed | 22 ++++ tests/ui/option_manual_map.rs | 38 ++++++ tests/ui/option_manual_map.stderr | 50 ++++++++ 7 files changed, 293 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/option_manual_map.rs create mode 100644 tests/ui/option_manual_map.fixed create mode 100644 tests/ui/option_manual_map.rs create mode 100644 tests/ui/option_manual_map.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 64864c2e2780..78fcb2164ebf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2122,6 +2122,7 @@ Released 2018-09-13 [`option_if_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn +[`option_match_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_match_map [`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option [`or_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call [`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 37a56bc20c8c..35e2b29dd9b9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -282,6 +282,7 @@ mod non_expressive_names; mod open_options; mod option_env_unwrap; mod option_if_let_else; +mod option_manual_map; mod overflow_check_conditional; mod panic_in_result_fn; mod panic_unimplemented; @@ -817,6 +818,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &open_options::NONSENSICAL_OPEN_OPTIONS, &option_env_unwrap::OPTION_ENV_UNWRAP, &option_if_let_else::OPTION_IF_LET_ELSE, + &option_manual_map::OPTION_MANUAL_MAP, &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, &panic_in_result_fn::PANIC_IN_RESULT_FN, &panic_unimplemented::PANIC, @@ -1224,6 +1226,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box zero_sized_map_values::ZeroSizedMapValues); store.register_late_pass(|| box vec_init_then_push::VecInitThenPush::default()); store.register_late_pass(move || box types::PtrAsPtr::new(msrv)); + store.register_late_pass(|| box option_manual_map::OptionManualMap); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1569,6 +1572,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP), + LintId::of(&option_manual_map::OPTION_MANUAL_MAP), LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL), LintId::of(&precedence::PRECEDENCE), @@ -1744,6 +1748,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST), LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), + LintId::of(&option_manual_map::OPTION_MANUAL_MAP), LintId::of(&ptr::CMP_NULL), LintId::of(&ptr::PTR_ARG), LintId::of(&ptr_eq::PTR_EQ), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index e99fe1b97ff6..dfe6e504a4f3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3143,10 +3143,9 @@ fn lint_search_is_some<'tcx>( then { if let hir::PatKind::Ref(..) = closure_arg.pat.kind { Some(search_snippet.replacen('&', "", 1)) - } else if let Some(name) = get_arg_name(&closure_arg.pat) { - Some(search_snippet.replace(&format!("*{}", name), &name.as_str())) } else { - None + get_arg_name(&closure_arg.pat).map(|name| + search_snippet.replace(&format!("*{}", name), &name.as_str())) } } else { None diff --git a/clippy_lints/src/option_manual_map.rs b/clippy_lints/src/option_manual_map.rs new file mode 100644 index 000000000000..2ca744c1927c --- /dev/null +++ b/clippy_lints/src/option_manual_map.rs @@ -0,0 +1,175 @@ +use crate::utils::{is_type_diagnostic_item, match_def_path, paths, snippet_with_applicability, span_lint_and_sugg}; +use core::fmt; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, PatKind, QPath}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::{sym, Ident}; + +declare_clippy_lint! { + /// **What it does:** Checks for usages of `match` which could be implemented using `Option::map` + /// + /// **Why is this bad?** Using the `map` method is clearer and more concise. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// match Some(0) { + /// Some(x) => Some(x + 1), + /// None => None, + /// }; + /// ``` + /// Use instead: + /// ```rust + /// Some(0).map(|x| x + 1); + /// ``` + pub OPTION_MANUAL_MAP, + style, + "reimplementation of `Option::map`" +} + +declare_lint_pass!(OptionManualMap => [OPTION_MANUAL_MAP]); + +impl LateLintPass<'_> for OptionManualMap { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + if !in_external_macro(cx.sess(), expr.span); + if let ExprKind::Match(scrutinee, [arm1, arm2], _) = expr.kind; + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type); + if let Some((some_binding, some_expr, none_expr)) = if is_none_arm(cx, arm1) { + get_some_binding(cx, arm2).map(|binding| (binding, arm2.body, arm1.body)) + } else if is_none_arm(cx, arm2) { + get_some_binding(cx, arm1).map(|binding| (binding, arm1.body, arm2.body)) + } else { + None + }; + + if is_none_expr(cx, none_expr); + if let Some(some_expr) = get_some_expr(cx, some_expr); + + then { + let mut app = Applicability::MachineApplicable; + + let body = if_chain! { + if let SomeBinding::Name(some_name) = some_binding; + if let ExprKind::Call(func, [arg]) = some_expr.kind; + if let ExprKind::Path(QPath::Resolved(_, arg_path)) = arg.kind; + if let [arg_name] = arg_path.segments; + if arg_name.ident.name == some_name.name; + then { + snippet_with_applicability(cx, func.span, "_", &mut app).into_owned() + } else { + format!("|{}| {}", some_binding, snippet_with_applicability(cx, some_expr.span, "..", &mut app)) + } + }; + + let scrutinee = snippet_with_applicability(cx, scrutinee.span, "_", &mut app); + span_lint_and_sugg( + cx, + OPTION_MANUAL_MAP, + expr.span, + "manual implementation of `Option::map`", + "use map instead", + format!("{}.map({})", scrutinee, body), + app + ); + } + } + } +} + +enum SomeBinding { + Wild, + Name(Ident), +} +impl fmt::Display for SomeBinding { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Wild => f.write_str("_"), + Self::Name(name) => name.fmt(f), + } + } +} + +fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { + match expr.kind { + ExprKind::Call( + Expr { + kind: ExprKind::Path(QPath::Resolved(None, path)), + .. + }, + [arg], + ) => { + if match_def_path(cx, path.res.opt_def_id()?, &paths::OPTION_SOME) { + Some(arg) + } else { + None + } + }, + ExprKind::Block( + Block { + stmts: [], + expr: Some(expr), + .. + }, + _, + ) => get_some_expr(cx, expr), + _ => None, + } +} + +fn is_none_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { + match expr.kind { + ExprKind::Path(QPath::Resolved(None, path)) => path + .res + .opt_def_id() + .map_or(false, |id| match_def_path(cx, id, &paths::OPTION_NONE)), + ExprKind::Block( + Block { + stmts: [], + expr: Some(expr), + .. + }, + _, + ) => is_none_expr(cx, expr), + _ => false, + } +} + +fn is_none_arm(cx: &LateContext<'tcx>, arm: &'tcx Arm<'_>) -> bool { + if arm.guard.is_some() { + return false; + } + match &arm.pat.kind { + PatKind::Wild => true, + PatKind::Path(QPath::Resolved(None, path)) => path + .res + .opt_def_id() + .map_or(false, |id| match_def_path(cx, id, &paths::OPTION_NONE)), + _ => false, + } +} + +fn get_some_binding(cx: &LateContext<'tcx>, arm: &'tcx Arm<'_>) -> Option { + if_chain! { + if arm.guard.is_none(); + if let PatKind::TupleStruct(QPath::Resolved(None, path), [some_pat], _) = arm.pat.kind; + if let Some(id) = path.res.opt_def_id(); + if match_def_path(cx, id, &paths::OPTION_SOME); + then { + match some_pat.kind { + PatKind::Binding(BindingAnnotation::Unannotated, _, bind_name, None) => { + Some(SomeBinding::Name(bind_name)) + } + PatKind::Wild => Some(SomeBinding::Wild), + _ => None, + } + } else { + None + } + } +} diff --git a/tests/ui/option_manual_map.fixed b/tests/ui/option_manual_map.fixed new file mode 100644 index 000000000000..1732cb782aa9 --- /dev/null +++ b/tests/ui/option_manual_map.fixed @@ -0,0 +1,22 @@ +// run-rustfix + +#![warn(clippy::option_manual_map)] +#![allow(clippy::map_identity)] + +fn main() { + Some(0).map(|_| 2); + + Some(0).map(|x| x + 1); + + Some("").map(|x| x.is_empty()); + + Some(0).map(|x| !x); + + #[rustfmt::skip] + Some(0).map(std::convert::identity); + + match Some(0) { + Some(x) if false => Some(x + 1), + _ => None, + }; +} diff --git a/tests/ui/option_manual_map.rs b/tests/ui/option_manual_map.rs new file mode 100644 index 000000000000..17ea27929da7 --- /dev/null +++ b/tests/ui/option_manual_map.rs @@ -0,0 +1,38 @@ +// run-rustfix + +#![warn(clippy::option_manual_map)] +#![allow(clippy::map_identity)] + +fn main() { + match Some(0) { + Some(_) => Some(2), + None:: => None, + }; + + match Some(0) { + Some(x) => Some(x + 1), + _ => None, + }; + + match Some("") { + Some(x) => Some(x.is_empty()), + None => None, + }; + + if let Some(x) = Some(0) { + Some(!x) + } else { + None + }; + + #[rustfmt::skip] + match Some(0) { + Some(x) => { Some(std::convert::identity(x)) } + None => { None } + }; + + match Some(0) { + Some(x) if false => Some(x + 1), + _ => None, + }; +} diff --git a/tests/ui/option_manual_map.stderr b/tests/ui/option_manual_map.stderr new file mode 100644 index 000000000000..94013c59b797 --- /dev/null +++ b/tests/ui/option_manual_map.stderr @@ -0,0 +1,50 @@ +error: manual implementation of `Option::map` + --> $DIR/option_manual_map.rs:7:5 + | +LL | / match Some(0) { +LL | | Some(_) => Some(2), +LL | | None:: => None, +LL | | }; + | |_____^ help: use map instead: `Some(0).map(|_| 2)` + | + = note: `-D clippy::option-manual-map` implied by `-D warnings` + +error: manual implementation of `Option::map` + --> $DIR/option_manual_map.rs:12:5 + | +LL | / match Some(0) { +LL | | Some(x) => Some(x + 1), +LL | | _ => None, +LL | | }; + | |_____^ help: use map instead: `Some(0).map(|x| x + 1)` + +error: manual implementation of `Option::map` + --> $DIR/option_manual_map.rs:17:5 + | +LL | / match Some("") { +LL | | Some(x) => Some(x.is_empty()), +LL | | None => None, +LL | | }; + | |_____^ help: use map instead: `Some("").map(|x| x.is_empty())` + +error: manual implementation of `Option::map` + --> $DIR/option_manual_map.rs:22:5 + | +LL | / if let Some(x) = Some(0) { +LL | | Some(!x) +LL | | } else { +LL | | None +LL | | }; + | |_____^ help: use map instead: `Some(0).map(|x| !x)` + +error: manual implementation of `Option::map` + --> $DIR/option_manual_map.rs:29:5 + | +LL | / match Some(0) { +LL | | Some(x) => { Some(std::convert::identity(x)) } +LL | | None => { None } +LL | | }; + | |_____^ help: use map instead: `Some(0).map(std::convert::identity)` + +error: aborting due to 5 previous errors +