Lint for if let Some(x) = ... instead of Option::map_or

CHANGELOG.md

@@ -1577,6 +1577,7 @@ Released 2018-09-13
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
 [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
 [`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap
+[`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_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option

clippy_lints/src/lib.rs

@@ -264,6 +264,7 @@ mod non_copy_const;
 mod non_expressive_names;
 mod open_options;
 mod option_env_unwrap;
+mod option_if_let_else;
 mod overflow_check_conditional;
 mod panic_unimplemented;
 mod partialeq_ne_impl;
@@ -734,6 +735,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &non_expressive_names::SIMILAR_NAMES,
         &open_options::NONSENSICAL_OPEN_OPTIONS,
         &option_env_unwrap::OPTION_ENV_UNWRAP,
+        &option_if_let_else::OPTION_IF_LET_ELSE,
         &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
         &panic_unimplemented::PANIC,
         &panic_unimplemented::PANIC_PARAMS,
@@ -1052,6 +1054,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
     store.register_late_pass(|| box unnamed_address::UnnamedAddress);
     store.register_late_pass(|| box dereference::Dereferencing);
+    store.register_late_pass(|| box option_if_let_else::OptionIfLetElse);
     store.register_late_pass(|| box future_not_send::FutureNotSend);
     store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls);
     store.register_late_pass(|| box if_let_mutex::IfLetMutex);
@@ -1369,6 +1372,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_if_let_else::OPTION_IF_LET_ELSE),
         LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
         LintId::of(&panic_unimplemented::PANIC_PARAMS),
         LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL),
@@ -1517,6 +1521,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&new_without_default::NEW_WITHOUT_DEFAULT),
         LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
         LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES),
+        LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE),
         LintId::of(&panic_unimplemented::PANIC_PARAMS),
         LintId::of(&ptr::CMP_NULL),
         LintId::of(&ptr::PTR_ARG),

clippy_lints/src/option_if_let_else.rs

@@ -0,0 +1,202 @@
+use crate::utils::sugg::Sugg;
+use crate::utils::{match_type, paths, span_lint_and_sugg};
+use if_chain::if_chain;
+
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
+use rustc_hir::*;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::Map;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+use std::marker::PhantomData;
+
+declare_clippy_lint! {
+    /// **What it does:**
+    /// Lints usage of  `if let Some(v) = ... { y } else { x }` which is more
+    /// idiomatically done with `Option::map_or` (if the else bit is a simple
+    /// expression) or `Option::map_or_else` (if the else bit is a longer
+    /// block).
+    ///
+    /// **Why is this bad?**
+    /// Using the dedicated functions of the Option type is clearer and
+    /// more concise than an if let expression.
+    ///
+    /// **Known problems:**
+    /// This lint uses whether the block is just an expression or if it has
+    /// more statements to decide whether to use `Option::map_or` or
+    /// `Option::map_or_else`. If you have a single expression which calls
+    /// an expensive function, then it would be more efficient to use
+    /// `Option::map_or_else`, but this lint would suggest `Option::map_or`.
+    ///
+    /// Also, this lint uses a deliberately conservative metric for checking
+    /// if the inside of either body contains breaks or continues which will
+    /// cause it to not suggest a fix if either block contains a loop with
+    /// continues or breaks contained within the loop.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// # let optional: Option<u32> = Some(0);
+    /// let _ = if let Some(foo) = optional {
+    ///     foo
+    /// } else {
+    ///     5
+    /// };
+    /// let _ = if let Some(foo) = optional {
+    ///     foo
+    /// } else {
+    ///     let y = do_complicated_function();
+    ///     y*y
+    /// };
+    /// ```
+    ///
+    /// should be
+    ///
+    /// ```rust
+    /// # let optional: Option<u32> = Some(0);
+    /// let _ = optional.map_or(5, |foo| foo);
+    /// let _ = optional.map_or_else(||{
+    ///     let y = do_complicated_function;
+    ///     y*y
+    /// }, |foo| foo);
+    /// ```
+    pub OPTION_IF_LET_ELSE,
+    style,
+    "reimplementation of Option::map_or"
+}
+
+declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]);
+
+/// Returns true iff the given expression is the result of calling Result::ok
+fn is_result_ok(cx: &LateContext<'_, '_>, expr: &'_ Expr<'_>) -> bool {
+    if_chain! {
+        if let ExprKind::MethodCall(ref path, _, &[ref receiver]) = &expr.kind;
+        if path.ident.name.to_ident_string() == "ok";
+        if match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT);
+        then {
+            true
+        } else {
+            false
+        }
+    }
+}
+
+/// A struct containing information about occurences of the
+/// `if let Some(..) = .. else` construct that this lint detects.
+struct OptionIfLetElseOccurence {
+    option: String,
+    method_sugg: String,
+    some_expr: String,
+    none_expr: String,
+}
+
+struct ReturnBreakContinueVisitor<'tcx> {
+    seen_return_break_continue: bool,
+    phantom_data: PhantomData<&'tcx bool>,
+}
+impl<'tcx> ReturnBreakContinueVisitor<'tcx> {
+    fn new() -> ReturnBreakContinueVisitor<'tcx> {
+        ReturnBreakContinueVisitor {
+            seen_return_break_continue: false,
+            phantom_data: PhantomData,
+        }
+    }
+}
+impl<'tcx> Visitor<'tcx> for ReturnBreakContinueVisitor<'tcx> {
+    type Map = Map<'tcx>;
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+
+    fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
+        if self.seen_return_break_continue {
+            // No need to look farther if we've already seen one of them
+            return;
+        }
+        match &ex.kind {
+            ExprKind::Ret(..) | ExprKind::Break(..) | ExprKind::Continue(..) => {
+                self.seen_return_break_continue = true;
+            },
+            // Something special could be done here to handle while or for loop
+            // desugaring, as this will detect a break if there's a while loop
+            // or a for loop inside the expression.
+            _ => {
+                rustc_hir::intravisit::walk_expr(self, ex);
+            },
+        }
+    }
+}
+
+fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool {
+    let mut recursive_visitor: ReturnBreakContinueVisitor<'tcx> = ReturnBreakContinueVisitor::new();
+    recursive_visitor.visit_expr(expression);
+    recursive_visitor.seen_return_break_continue
+}
+
+/// If this expression is the option if let/else construct we're detecting, then
+/// this function returns an OptionIfLetElseOccurence struct with details if
+/// this construct is found, or None if this construct is not found.
+fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option<OptionIfLetElseOccurence> {
+    //(String, String, String, String)> {
+    if_chain! {
+        if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
+        if arms.len() == 2;
+        if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION);
+        if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already
+        if let PatKind::TupleStruct(_, &[inner_pat], _) = &arms[0].pat.kind;
+        if let PatKind::Binding(_, _, id, _) = &inner_pat.kind;
+        if !contains_return_break_continue(arms[0].body);
+        if !contains_return_break_continue(arms[1].body);
+        then {
+            let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
+                = &arms[0].body.kind {
+                if let &[] = &statements {
+                    expr
+                } else {
+                    &arms[0].body
+                }
+            } else {
+                return None;
+            };
+            let (none_body, method_sugg) = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
+                = &arms[1].body.kind {
+                if let &[] = &statements {
+                    (expr, "map_or")
+                } else {
+                    (&arms[1].body, "map_or_else")
+                }
+            } else {
+                return None;
+            };
+            let capture_name = id.name.to_ident_string();
+            Some(OptionIfLetElseOccurence {
+                option: format!("{}", Sugg::hir(cx, let_body, "..")),
+                method_sugg: format!("{}", method_sugg),
+                some_expr: format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
+                none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, ".."))
+            })
+        } else {
+            None
+        }
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
+        if let Some(detection) = detect_option_if_let_else(cx, expr) {
+            span_lint_and_sugg(
+                cx,
+                OPTION_IF_LET_ELSE,
+                expr.span,
+                format!("use Option::{} instead of an if let/else", detection.method_sugg).as_str(),
+                "try",
+                format!(
+                    "{}.{}({}, {})",
+                    detection.option, detection.method_sugg, detection.none_expr, detection.some_expr
+                ),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}

src/lintlist/mod.rs

@@ -1620,6 +1620,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "option_env_unwrap",
     },
+    Lint {
+        name: "option_if_let_else",
+        group: "style",
+        desc: "reimplementation of Option::map_or",
+        deprecation: None,
+        module: "option_if_let_else",
+    },
     Lint {
         name: "option_map_or_none",
         group: "style",

tests/ui/option_if_let_else.fixed

@@ -0,0 +1,48 @@
+// run-rustfix
+#![warn(clippy::option_if_let_else)]
+
+fn bad1(string: Option<&str>) -> (bool, &str) {
+    string.map_or((false, "hello"), |x| (true, x))
+}
+
+fn longer_body(arg: Option<u32>) -> u32 {
+    arg.map_or(13, |x| {
+        let y = x * x;
+        y * y
+    })
+}
+
+fn test_map_or_else(arg: Option<u32>) {
+    let _ = arg.map_or_else(|| {
+        let mut y = 1;
+        y = (y + 2 / y) / 2;
+        y = (y + 2 / y) / 2;
+        y
+    }, |x| x * x * x * x);
+}
+
+fn negative_tests(arg: Option<u32>) -> u32 {
+    let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
+    for _ in 0..10 {
+        let _ = if let Some(x) = arg {
+            x
+        } else {
+            continue;
+        };
+    }
+    let _ = if let Some(x) = arg {
+        return x;
+    } else {
+        5
+    };
+    7
+}
+
+fn main() {
+    let optional = Some(5);
+    let _ = optional.map_or(5, |x| x + 2);
+    let _ = bad1(None);
+    let _ = longer_body(None);
+    test_map_or_else(None);
+    let _ = negative_tests(None);
+}

tests/ui/option_if_let_else.rs

@@ -0,0 +1,56 @@
+// run-rustfix
+#![warn(clippy::option_if_let_else)]
+
+fn bad1(string: Option<&str>) -> (bool, &str) {
+    if let Some(x) = string {
+        (true, x)
+    } else {
+        (false, "hello")
+    }
+}
+
+fn longer_body(arg: Option<u32>) -> u32 {
+    if let Some(x) = arg {
+        let y = x * x;
+        y * y
+    } else {
+        13
+    }
+}
+
+fn test_map_or_else(arg: Option<u32>) {
+    let _ = if let Some(x) = arg {
+        x * x * x * x
+    } else {
+        let mut y = 1;
+        y = (y + 2 / y) / 2;
+        y = (y + 2 / y) / 2;
+        y
+    };
+}
+
+fn negative_tests(arg: Option<u32>) -> u32 {
+    let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
+    for _ in 0..10 {
+        let _ = if let Some(x) = arg {
+            x
+        } else {
+            continue;
+        };
+    }
+    let _ = if let Some(x) = arg {
+        return x;
+    } else {
+        5
+    };
+    7
+}
+
+fn main() {
+    let optional = Some(5);
+    let _ = if let Some(x) = optional { x + 2 } else { 5 };
+    let _ = bad1(None);
+    let _ = longer_body(None);
+    test_map_or_else(None);
+    let _ = negative_tests(None);
+}

tests/ui/option_if_let_else.stderr

@@ -0,0 +1,62 @@
+error: use Option::map_or instead of an if let/else
+  --> $DIR/option_if_let_else.rs:5:5
+   |
+LL | /     if let Some(x) = string {
+LL | |         (true, x)
+LL | |     } else {
+LL | |         (false, "hello")
+LL | |     }
+   | |_____^ help: try: `string.map_or((false, "hello"), |x| (true, x))`
+   |
+   = note: `-D clippy::option-if-let-else` implied by `-D warnings`
+
+error: use Option::map_or instead of an if let/else
+  --> $DIR/option_if_let_else.rs:13:5
+   |
+LL | /     if let Some(x) = arg {
+LL | |         let y = x * x;
+LL | |         y * y
+LL | |     } else {
+LL | |         13
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL |     arg.map_or(13, |x| {
+LL |         let y = x * x;
+LL |         y * y
+LL |     })
+   |
+
+error: use Option::map_or_else instead of an if let/else
+  --> $DIR/option_if_let_else.rs:22:13
+   |
+LL |       let _ = if let Some(x) = arg {
+   |  _____________^
+LL | |         x * x * x * x
+LL | |     } else {
+LL | |         let mut y = 1;
+...  |
+LL | |         y
+LL | |     };
+   | |_____^
+   |
+help: try
+   |
+LL |     let _ = arg.map_or_else(|| {
+LL |         let mut y = 1;
+LL |         y = (y + 2 / y) / 2;
+LL |         y = (y + 2 / y) / 2;
+LL |         y
+LL |     }, |x| x * x * x * x);
+   |
+
+error: use Option::map_or instead of an if let/else
+  --> $DIR/option_if_let_else.rs:51:13
+   |
+LL |     let _ = if let Some(x) = optional { x + 2 } else { 5 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
+
+error: aborting due to 4 previous errors
+