Auto merge of rust-lang#5301 - JarredAllen:option_if_let_else, r=flip…

…1995

Suggest `Option::map_or`(_else) for `if let Some { y } else { x }`

Fixes rust-lang#5203

There are two issues with this code that I have noticed:

- Use of `Option::map_or` causes it to always evaluate the code in the else block. If that block is computationally expensive or if it updates some state (such as getting the next value from an iterator), then this change would cause the code to behave differently. In either of those circumstances, it should suggest Option::map_or_else, which takes both cases as a closure and runs one. However, I don't know how to check if the expression would change some state, so I left the lint's applicability as MaybeIncorrect.

- There are lints which can trigger on specific sub-cases of this lint (`if_let_some_result`, `question_mark`, and `while_let_loop`) and suggest different changes (usually better ones because they're more specific). Is this acceptable for clippy to give multiple suggestions, or should I have the code check if those other lints trigger and then not trigger this lint if they do?

changelog: Add lint [`option_if_let_else`]

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/attrs.rs

@@ -481,15 +481,14 @@ fn is_relevant_trait(cx: &LateContext<'_>, item: &TraitItem<'_>) -> bool {
 }
 
 fn is_relevant_block(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool {
-    if let Some(stmt) = block.stmts.first() {
-        match &stmt.kind {
+    block.stmts.first().map_or(
+        block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)),
+        |stmt| match &stmt.kind {
             StmtKind::Local(_) => true,
             StmtKind::Expr(expr) | StmtKind::Semi(expr) => is_relevant_expr(cx, tables, expr),
             _ => false,
-        }
-    } else {
-        block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e))
-    }
+        },
+    )
 }
 
 fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: &Expr<'_>) -> bool {
@@ -499,11 +498,10 @@ fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: &
         ExprKind::Ret(None) | ExprKind::Break(_, None) => false,
         ExprKind::Call(path_expr, _) => {
             if let ExprKind::Path(qpath) = &path_expr.kind {
-                if let Some(fun_id) = tables.qpath_res(qpath, path_expr.hir_id).opt_def_id() {
-                    !match_def_path(cx, fun_id, &paths::BEGIN_PANIC)
-                } else {
-                    true
-                }
+                tables
+                    .qpath_res(qpath, path_expr.hir_id)
+                    .opt_def_id()
+                    .map_or(true, |fun_id| !match_def_path(cx, fun_id, &paths::BEGIN_PANIC))
             } else {
                 true
             }

clippy_lints/src/if_let_mutex.rs

@@ -135,13 +135,10 @@ impl<'tcx> Visitor<'tcx> for ArmVisitor<'_, 'tcx> {
     }
 }
 
-impl<'tcx> ArmVisitor<'_, 'tcx> {
+impl<'tcx, 'l> ArmVisitor<'tcx, 'l> {
     fn same_mutex(&self, cx: &LateContext<'_>, op_mutex: &Expr<'_>) -> bool {
-        if let Some(arm_mutex) = self.found_mutex {
-            SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)
-        } else {
-            false
-        }
+        self.found_mutex
+            .map_or(false, |arm_mutex| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex))
     }
 }
 

clippy_lints/src/len_zero.rs

@@ -302,16 +302,12 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
 
     let ty = &walk_ptrs_ty(cx.tables().expr_ty(expr));
     match ty.kind {
-        ty::Dynamic(ref tt, ..) => {
-            if let Some(principal) = tt.principal() {
-                cx.tcx
-                    .associated_items(principal.def_id())
-                    .in_definition_order()
-                    .any(|item| is_is_empty(cx, &item))
-            } else {
-                false
-            }
-        },
+        ty::Dynamic(ref tt, ..) => tt.principal().map_or(false, |principal| {
+            cx.tcx
+                .associated_items(principal.def_id())
+                .in_definition_order()
+                .any(|item| is_is_empty(cx, &item))
+        }),
         ty::Projection(ref proj) => has_is_empty_impl(cx, proj.item_def_id),
         ty::Adt(id, _) => has_is_empty_impl(cx, id.did),
         ty::Array(..) | ty::Slice(..) | ty::Str => true,

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);
@@ -1158,6 +1161,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&needless_continue::NEEDLESS_CONTINUE),
         LintId::of(&needless_pass_by_value::NEEDLESS_PASS_BY_VALUE),
         LintId::of(&non_expressive_names::SIMILAR_NAMES),
+        LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE),
         LintId::of(&ranges::RANGE_PLUS_ONE),
         LintId::of(&shadow::SHADOW_UNRELATED),
         LintId::of(&strings::STRING_ADD_ASSIGN),

clippy_lints/src/literal_representation.rs

@@ -264,10 +264,13 @@ impl LiteralDigitGrouping {
 
         let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut num_lit.exponent {
             (exponent, &["32", "64"][..], 'f')
-        } else if let Some(fraction) = &mut num_lit.fraction {
-            (fraction, &["32", "64"][..], 'f')
         } else {
-            (&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i')
+            num_lit
+                .fraction
+                .as_mut()
+                .map_or((&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i'), |fraction| {
+                    (fraction, &["32", "64"][..], 'f')
+                })
         };
 
         let mut split = part.rsplit('_');

clippy_lints/src/loops.rs

@@ -686,13 +686,9 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
                 NeverLoopResult::AlwaysBreak
             }
         },
-        ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => {
-            if let Some(ref e) = *e {
-                combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
-            } else {
-                NeverLoopResult::AlwaysBreak
-            }
-        },
+        ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
+            combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
+        }),
         ExprKind::InlineAsm(ref asm) => asm
             .operands
             .iter()
@@ -1881,13 +1877,9 @@ fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
 fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
     // IntoIterator is currently only implemented for array sizes <= 32 in rustc
     match ty.kind {
-        ty::Array(_, n) => {
-            if let Some(val) = n.try_eval_usize(cx.tcx, cx.param_env) {
-                (0..=32).contains(&val)
-            } else {
-                false
-            }
-        },
+        ty::Array(_, n) => n
+            .try_eval_usize(cx.tcx, cx.param_env)
+            .map_or(false, |val| (0..=32).contains(&val)),
         _ => false,
     }
 }
@@ -1899,11 +1891,7 @@ fn extract_expr_from_first_stmt<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<
         return None;
     }
     if let StmtKind::Local(ref local) = block.stmts[0].kind {
-        if let Some(expr) = local.init {
-            Some(expr)
-        } else {
-            None
-        }
+        local.init //.map(|expr| expr)
     } else {
         None
     }
@@ -2023,15 +2011,13 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
                 if let PatKind::Binding(.., ident, _) = local.pat.kind {
                     self.name = Some(ident.name);
 
-                    self.state = if let Some(ref init) = local.init {
+                    self.state = local.init.as_ref().map_or(VarState::Declared, |init| {
                         if is_integer_const(&self.cx, init, 0) {
                             VarState::Warn
                         } else {
                             VarState::Declared
                         }
-                    } else {
-                        VarState::Declared
-                    }
+                    })
                 }
             }
         }

clippy_lints/src/methods/mod.rs

@@ -2460,13 +2460,9 @@ fn derefs_to_slice<'tcx>(
             ty::Slice(_) => true,
             ty::Adt(def, _) if def.is_box() => may_slice(cx, ty.boxed_ty()),
             ty::Adt(..) => is_type_diagnostic_item(cx, ty, sym!(vec_type)),
-            ty::Array(_, size) => {
-                if let Some(size) = size.try_eval_usize(cx.tcx, cx.param_env) {
-                    size < 32
-                } else {
-                    false
-                }
-            },
+            ty::Array(_, size) => size
+                .try_eval_usize(cx.tcx, cx.param_env)
+                .map_or(false, |size| size < 32),
             ty::Ref(_, inner, _) => may_slice(cx, inner),
             _ => false,
         }

clippy_lints/src/methods/unnecessary_filter_map.rs

@@ -77,13 +77,10 @@ fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tc
             }
             (true, true)
         },
-        hir::ExprKind::Block(ref block, _) => {
-            if let Some(expr) = &block.expr {
-                check_expression(cx, arg_id, &expr)
-            } else {
-                (false, false)
-            }
-        },
+        hir::ExprKind::Block(ref block, _) => block
+            .expr
+            .as_ref()
+            .map_or((false, false), |expr| check_expression(cx, arg_id, &expr)),
         hir::ExprKind::Match(_, arms, _) => {
             let mut found_mapping = false;
             let mut found_filtering = false;

clippy_lints/src/minmax.rs

@@ -53,7 +53,7 @@ impl<'tcx> LateLintPass<'tcx> for MinMaxPass {
     }
 }
 
-#[derive(PartialEq, Eq, Debug)]
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
 enum MinMax {
     Min,
     Max,
@@ -86,16 +86,15 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt
     if args.len() != 2 {
         return None;
     }
-    if let Some(c) = constant_simple(cx, cx.tables(), &args[0]) {
-        if constant_simple(cx, cx.tables(), &args[1]).is_none() {
-            // otherwise ignore
-            Some((m, c, &args[1]))
-        } else {
-            None
-        }
-    } else if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) {
-        Some((m, c, &args[0]))
-    } else {
-        None
-    }
+    constant_simple(cx, cx.tables(), &args[0]).map_or_else(
+        || constant_simple(cx, cx.tables(), &args[1]).map(|c| (m, c, &args[0])),
+        |c| {
+            if constant_simple(cx, cx.tables(), &args[1]).is_none() {
+                // otherwise ignore
+                Some((m, c, &args[1]))
+            } else {
+                None
+            }
+        },
+    )
 }

clippy_lints/src/misc.rs

@@ -682,16 +682,10 @@ fn check_to_owned(cx: &LateContext<'_>, expr: &Expr<'_>, other: &Expr<'_>, left:
 /// `unused_variables`'s idea
 /// of what it means for an expression to be "used".
 fn is_used(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    if let Some(parent) = get_parent_expr(cx, expr) {
-        match parent.kind {
-            ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => {
-                SpanlessEq::new(cx).eq_expr(rhs, expr)
-            },
-            _ => is_used(cx, parent),
-        }
-    } else {
-        true
-    }
+    get_parent_expr(cx, expr).map_or(true, |parent| match parent.kind {
+        ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => SpanlessEq::new(cx).eq_expr(rhs, expr),
+        _ => is_used(cx, parent),
+    })
 }
 
 /// Tests whether an expression is in a macro expansion (e.g., something