Auto merge of #4386 - lzutao:lint-option-and_then-some, r=flip1995

Add option_and_then_some lint

changelog: Add complexity lint to warn about `option.and_then(|o| Some(x))` and suggest replacing with `option.map(|o| x)`.

Closes #4299

Author: bors

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 310 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 311 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_dev/src/lib.rs

@@ -123,13 +123,13 @@ pub fn gen_deprecated(lints: &[Lint]) -> Vec<String> {
     lints
         .iter()
         .filter_map(|l| {
-            l.clone().deprecation.and_then(|depr_text| {
-                Some(vec![
+            l.clone().deprecation.map(|depr_text| {
+                vec![
                     "    store.register_removed(".to_string(),
                     format!("        \"clippy::{}\",", l.name),
                     format!("        \"{}\",", depr_text),
                     "    );".to_string(),
-                ])
+                ]
             })
         })
         .flatten()

clippy_lints/src/lib.rs

@@ -795,6 +795,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,
@@ -1033,6 +1034,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::SUSPICIOUS_MAP,
         methods::UNNECESSARY_FILTER_MAP,

clippy_lints/src/methods/mod.rs

@@ -21,11 +21,11 @@ use syntax::symbol::LocalInternedString;
 use crate::utils::sugg;
 use crate::utils::usage::mutated_variables;
 use crate::utils::{
-    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy,
-    is_ctor_function, is_expn_of, iter_input_pats, last_path_segment, match_def_path, match_qpath, match_trait_method,
-    match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path,
-    snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_sugg,
-    span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
+    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, in_macro_or_desugar,
+    is_copy, is_ctor_function, is_expn_of, iter_input_pats, last_path_segment, match_def_path, match_qpath,
+    match_trait_method, match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys,
+    single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
+    span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
 };
 use crate::utils::{paths, span_help_and_lint};
 
@@ -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()`.
     ///
@@ -918,6 +944,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,
@@ -967,6 +994,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]),
@@ -2072,6 +2100,97 @@ 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]) {
+    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 a no-op";
+
+    // Searches an return expressions in `y` in `_.and_then(|x| Some(y))`, which we don't lint
+    struct RetCallFinder {
+        found: bool,
+    }
+
+    impl<'tcx> intravisit::Visitor<'tcx> for RetCallFinder {
+        fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
+            if self.found {
+                return;
+            }
+            if let hir::ExprKind::Ret(..) = &expr.node {
+                self.found = true;
+            } else {
+                intravisit::walk_expr(self, expr);
+            }
+        }
+
+        fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
+            intravisit::NestedVisitorMap::None
+        }
+    }
+
+    let ty = cx.tables.expr_ty(&args[0]);
+    if !match_type(cx, ty, &paths::OPTION) {
+        return;
+    }
+
+    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);
+            if_chain! {
+                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;
+                then {
+                    let inner_expr = &some_args[0];
+
+                    let mut finder = RetCallFinder { found: false };
+                    finder.visit_expr(inner_expr);
+                    if finder.found {
+                        return;
+                    }
+
+                    let some_inner_snip = if in_macro_or_desugar(inner_expr.span) {
+                        snippet_with_macro_callsite(cx, inner_expr.span, "_")
+                    } else {
+                        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; 310] = [
+pub const ALL_LINTS: [Lint; 311] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1316,6 +1316,13 @@ pub const ALL_LINTS: [Lint; 310] = [
         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,25 @@
+// 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);
+}
+
+pub fn foo() -> Option<String> {
+    let x = Some(String::from("hello"));
+    Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?)))
+}
+
+pub fn example2(x: bool) -> Option<&'static str> {
+    Some("a").and_then(|s| Some(if x { s } else { return None }))
+}

tests/ui/option_and_then_some.rs

@@ -0,0 +1,25 @@
+// 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);
+}
+
+pub fn foo() -> Option<String> {
+    let x = Some(String::from("hello"));
+    Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?)))
+}
+
+pub fn example2(x: bool) -> Option<&'static str> {
+    Some("a").and_then(|s| Some(if x { s } else { return None }))
+}

tests/ui/option_and_then_some.stderr

@@ -0,0 +1,20 @@
+error: using `Option.and_then(Some)`, which is a 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
+

tests/ui/option_map_or_none.fixed

@@ -1,5 +1,7 @@
 // run-rustfix
 
+#![allow(clippy::option_and_then_some)]
+
 fn main() {
     let opt = Some(1);
 

tests/ui/option_map_or_none.rs

@@ -1,5 +1,7 @@
 // run-rustfix
 
+#![allow(clippy::option_and_then_some)]
+
 fn main() {
     let opt = Some(1);
 

tests/ui/option_map_or_none.stderr

@@ -1,13 +1,13 @@
 error: called `map_or(None, f)` on an Option value. This can be done more directly by calling `and_then(f)` instead
-  --> $DIR/option_map_or_none.rs:8:13
+  --> $DIR/option_map_or_none.rs:10:13
    |
 LL |     let _ = opt.map_or(None, |x| Some(x + 1));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using and_then instead: `opt.and_then(|x| Some(x + 1))`
    |
    = note: `-D clippy::option-map-or-none` implied by `-D warnings`
 
 error: called `map_or(None, f)` on an Option value. This can be done more directly by calling `and_then(f)` instead
-  --> $DIR/option_map_or_none.rs:11:13
+  --> $DIR/option_map_or_none.rs:13:13
    |
 LL |       let _ = opt.map_or(None, |x| {
    |  _____________^