Add: option_manual_map lint

Jarcho · Jarcho · commit f7271936ddfd · 2021-01-10T16:30:47.000-05:00
diff --git 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
@@ -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
@@ -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
@@ -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<SomeBinding> {
+    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
@@ -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
@@ -0,0 +1,38 @@
+// run-rustfix
+
+#![warn(clippy::option_manual_map)]
+#![allow(clippy::map_identity)]
+
+fn main() {
+    match Some(0) {
+        Some(_) => Some(2),
+        None::<u32> => 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
@@ -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::<u32> => 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
+
