Add option_and_then_some lint

Author: tesuji

CHANGELOG.md

@@ -1088,6 +1088,7 @@ Released 2018-09-13
 [`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
 [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
+[`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some
 [`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_map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 309 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 310 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -794,6 +794,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         methods::ITER_SKIP_NEXT,
         methods::NEW_RET_NO_SELF,
         methods::OK_EXPECT,
+        methods::OPTION_AND_THEN_SOME,
         methods::OPTION_MAP_OR_NONE,
         methods::OR_FUN_CALL,
         methods::SEARCH_IS_SOME,
@@ -1032,6 +1033,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         methods::CLONE_ON_COPY,
         methods::FILTER_NEXT,
         methods::FLAT_MAP_IDENTITY,
+        methods::OPTION_AND_THEN_SOME,
         methods::SEARCH_IS_SOME,
         methods::UNNECESSARY_FILTER_MAP,
         methods::USELESS_ASREF,

clippy_lints/src/methods/mod.rs

@@ -264,6 +264,32 @@ declare_clippy_lint! {
     "using `Option.map_or(None, f)`, which is more succinctly expressed as `and_then(f)`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely as
+    /// `_.map(|x| y)`.
+    ///
+    /// **Known problems:** None
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let x = Some("foo");
+    /// let _ = x.and_then(|s| Some(s.len()));
+    /// ```
+    ///
+    /// The correct use would be:
+    ///
+    /// ```rust
+    /// let x = Some("foo");
+    /// let _ = x.map(|s| s.len());
+    /// ```
+    pub OPTION_AND_THEN_SOME,
+    complexity,
+    "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for usage of `_.filter(_).next()`.
     ///
@@ -900,6 +926,7 @@ declare_lint_pass!(Methods => [
     OPTION_MAP_UNWRAP_OR_ELSE,
     RESULT_MAP_UNWRAP_OR_ELSE,
     OPTION_MAP_OR_NONE,
+    OPTION_AND_THEN_SOME,
     OR_FUN_CALL,
     EXPECT_FUN_CALL,
     CHARS_NEXT_CMP,
@@ -948,6 +975,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             ["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]),
             ["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]),
             ["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
+            ["and_then", ..] => lint_option_and_then_some(cx, expr, arg_lists[0]),
             ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
             ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]),
@@ -2052,6 +2080,68 @@ fn lint_map_or_none<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr,
     }
 }
 
+/// Lint use of `_.and_then(|x| Some(y))` for `Option`s
+fn lint_option_and_then_some(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) {
+    let ty = cx.tables.expr_ty(&args[0]);
+    if !match_type(cx, ty, &paths::OPTION) {
+        return;
+    }
+
+    const LINT_MSG: &str = "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`";
+    const NO_OP_MSG: &str = "using `Option.and_then(Some)`, which is no-op";
+
+    match args[1].node {
+        hir::ExprKind::Closure(_, _, body_id, closure_args_span, _) => {
+            let closure_body = cx.tcx.hir().body(body_id);
+            let closure_expr = remove_blocks(&closure_body.value);
+
+            // let note = format!("{:?}", args[1]);
+            // span_note_and_lint(cx, OPTION_AND_THEN_SOME, closure_args_span, "Outer debugging
+            // information", closure_args_span, &note);
+
+            if let hir::ExprKind::Call(ref some_expr, ref some_args) = closure_expr.node {
+                if let hir::ExprKind::Path(ref qpath) = some_expr.node {
+                    if match_qpath(qpath, &paths::OPTION_SOME) {
+                        if some_args.len() == 1 {
+                            let inner_expr = &some_args[0];
+                            let some_inner_snip = snippet(cx, inner_expr.span, "..");
+                            let closure_args_snip = snippet(cx, closure_args_span, "..");
+                            let option_snip = snippet(cx, args[0].span, "..");
+                            let note = format!("{}.map({} {})", option_snip, closure_args_snip, some_inner_snip);
+                            span_lint_and_sugg(
+                                cx,
+                                OPTION_AND_THEN_SOME,
+                                expr.span,
+                                LINT_MSG,
+                                "try this",
+                                note,
+                                Applicability::MachineApplicable,
+                            );
+                        }
+                    }
+                }
+            }
+        },
+        // `_.and_then(Some)` case, which is no-op.
+        hir::ExprKind::Path(ref qpath) => {
+            if match_qpath(qpath, &paths::OPTION_SOME) {
+                let option_snip = snippet(cx, args[0].span, "..");
+                let note = format!("{}", option_snip);
+                span_lint_and_sugg(
+                    cx,
+                    OPTION_AND_THEN_SOME,
+                    expr.span,
+                    NO_OP_MSG,
+                    "use the expression directly",
+                    note,
+                    Applicability::MachineApplicable,
+                );
+            }
+        },
+        _ => {},
+    }
+}
+
 /// lint use of `filter().next()` for `Iterators`
 fn lint_filter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, filter_args: &'tcx [hir::Expr]) {
     // lint if caller of `.filter().next()` is an Iterator

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 309] = [
+pub const ALL_LINTS: [Lint; 310] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1316,6 +1316,13 @@ pub const ALL_LINTS: [Lint; 309] = [
         deprecation: None,
         module: "eq_op",
     },
+    Lint {
+        name: "option_and_then_some",
+        group: "complexity",
+        desc: "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`",
+        deprecation: None,
+        module: "methods",
+    },
     Lint {
         name: "option_map_or_none",
         group: "style",

tests/ui/option_and_then_some.fixed

@@ -0,0 +1,16 @@
+// run-rustfix
+#![deny(clippy::option_and_then_some)]
+
+// need a main anyway, use it get rid of unused warnings too
+pub fn main() {
+    let x = Some(5);
+    // the easiest cases
+    let _ = x;
+    let _ = x.map(|o| o + 1);
+    // and an easy counter-example
+    let _ = x.and_then(|o| if o < 32 { Some(o) } else { None });
+
+    // Different type
+    let x: Result<u32, &str> = Ok(1);
+    let _ = x.and_then(Ok);
+}

tests/ui/option_and_then_some.rs

@@ -0,0 +1,16 @@
+// run-rustfix
+#![deny(clippy::option_and_then_some)]
+
+// need a main anyway, use it get rid of unused warnings too
+pub fn main() {
+    let x = Some(5);
+    // the easiest cases
+    let _ = x.and_then(Some);
+    let _ = x.and_then(|o| Some(o + 1));
+    // and an easy counter-example
+    let _ = x.and_then(|o| if o < 32 { Some(o) } else { None });
+
+    // Different type
+    let x: Result<u32, &str> = Ok(1);
+    let _ = x.and_then(Ok);
+}

tests/ui/option_and_then_some.stderr

@@ -0,0 +1,20 @@
+error: using `Option.and_then(Some)`, which is no-op
+  --> $DIR/option_and_then_some.rs:8:13
+   |
+LL |     let _ = x.and_then(Some);
+   |             ^^^^^^^^^^^^^^^^ help: use the expression directly: `x`
+   |
+note: lint level defined here
+  --> $DIR/option_and_then_some.rs:2:9
+   |
+LL | #![deny(clippy::option_and_then_some)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
+  --> $DIR/option_and_then_some.rs:9:13
+   |
+LL |     let _ = x.and_then(|o| Some(o + 1));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.map(|o| o + 1)`
+
+error: aborting due to 2 previous errors
+