diff --git a/CHANGELOG.md b/CHANGELOG.md index ed8f16e65bb9..1a081bb85fea 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 2d0855f68955..d68d0d8ccf58 100644 --- a/clippy_lints/src/attrs.rs +++ b/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 } diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index f911cb68ea57..fbd2eeacc6ef 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/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)) } } diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 26d96428771d..1b09328ceabb 100644 --- a/clippy_lints/src/len_zero.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1d5be893ffba..fe34e4390d65 100644 --- a/clippy_lints/src/lib.rs +++ b/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), diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 7ba43562d7d4..a36fdca5d5de 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/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('_'); diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index d821b5134841..b803d753b6d0 100644 --- a/clippy_lints/src/loops.rs +++ b/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 - } + }) } } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 160304865c56..f1c8894c0ee2 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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, } diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index fdcba1105428..75e123eb5939 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/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; diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs index 0a2d577396a5..c8aa98d34892 100644 --- a/clippy_lints/src/minmax.rs +++ b/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 + } + }, + ) } diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 6a256627bd18..3d4225f36a7d 100644 --- a/clippy_lints/src/misc.rs +++ b/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 diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs new file mode 100644 index 000000000000..8dbe58763bfb --- /dev/null +++ b/clippy_lints/src/option_if_let_else.rs @@ -0,0 +1,267 @@ +use crate::utils; +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::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::map::Map; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +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 = Some(0); + /// # fn do_complicated_function() -> u32 { 5 }; + /// 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 = Some(0); + /// # fn do_complicated_function() -> u32 { 5 }; + /// 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, + pedantic, + "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 let ExprKind::MethodCall(ref path, _, &[ref receiver], _) = &expr.kind { + path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables().expr_ty(&receiver), &paths::RESULT) + } 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, + wrap_braces: bool, +} + +struct ReturnBreakContinueMacroVisitor { + seen_return_break_continue: bool, +} +impl ReturnBreakContinueMacroVisitor { + fn new() -> ReturnBreakContinueMacroVisitor { + ReturnBreakContinueMacroVisitor { + seen_return_break_continue: false, + } + } +} +impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor { + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + 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. + _ => { + if utils::in_macro(ex.span) { + self.seen_return_break_continue = true; + } else { + rustc_hir::intravisit::walk_expr(self, ex); + } + }, + } + } +} + +fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool { + let mut recursive_visitor = ReturnBreakContinueMacroVisitor::new(); + recursive_visitor.visit_expr(expression); + recursive_visitor.seen_return_break_continue +} + +/// Extracts the body of a given arm. If the arm contains only an expression, +/// then it returns the expression. Otherwise, it returns the entire block +fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> { + if let ExprKind::Block( + Block { + stmts: statements, + expr: Some(expr), + .. + }, + _, + ) = &arm.body.kind + { + if let [] = statements { + Some(&expr) + } else { + Some(&arm.body) + } + } else { + None + } +} + +/// If this is the else body of an if/else expression, then we need to wrap +/// it in curcly braces. Otherwise, we don't. +fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { + if let Some(Expr { + kind: + ExprKind::Match( + _, + arms, + MatchSource::IfDesugar { + contains_else_clause: true, + } + | MatchSource::IfLetDesugar { + contains_else_clause: true, + }, + ), + .. + }) = parent.expr + { + expr.hir_id == arms[1].body.hir_id + } else { + false + } + }) +} + +fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: bool, as_mut: bool) -> String { + format!( + "{}{}", + Sugg::hir(cx, cond_expr, "..").maybe_par(), + if as_mut { + ".as_mut()" + } else if as_ref { + ".as_ref()" + } else { + "" + } + ) +} + +/// 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(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { + if_chain! { + if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly + if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind; + if arms.len() == 2; + if !is_result_ok(cx, cond_expr); // Don't lint on Result::ok because a different lint does it already + if let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind; + if utils::match_qpath(struct_qpath, &paths::OPTION_SOME); + if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind; + if !contains_return_break_continue_macro(arms[0].body); + if !contains_return_break_continue_macro(arms[1].body); + then { + let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" }; + let some_body = extract_body_from_arm(&arms[0])?; + let none_body = extract_body_from_arm(&arms[1])?; + let method_sugg = match &none_body.kind { + ExprKind::Block(..) => "map_or_else", + _ => "map_or", + }; + let capture_name = id.name.to_ident_string(); + let wrap_braces = should_wrap_in_braces(cx, expr); + let (as_ref, as_mut) = match &cond_expr.kind { + ExprKind::AddrOf(_, Mutability::Not, _) => (true, false), + ExprKind::AddrOf(_, Mutability::Mut, _) => (false, true), + _ => (bind_annotation == &BindingAnnotation::Ref, bind_annotation == &BindingAnnotation::RefMut), + }; + let cond_expr = match &cond_expr.kind { + // Pointer dereferencing happens automatically, so we can omit it in the suggestion + ExprKind::Unary(UnOp::UnDeref, expr) | ExprKind::AddrOf(_, _, expr) => expr, + _ => cond_expr, + }; + Some(OptionIfLetElseOccurence { + option: format_option_in_sugg(cx, cond_expr, as_ref, as_mut), + method_sugg: method_sugg.to_string(), + some_expr: format!("|{}{}| {}", capture_mut, capture_name, Sugg::hir(cx, some_body, "..")), + none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")), + wrap_braces, + }) + } else { + None + } + } +} + +impl<'a> LateLintPass<'a> for OptionIfLetElse { + fn check_expr(&mut self, cx: &LateContext<'a>, expr: &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!( + "{}{}.{}({}, {}){}", + if detection.wrap_braces { "{ " } else { "" }, + detection.option, + detection.method_sugg, + detection.none_expr, + detection.some_expr, + if detection.wrap_braces { " }" } else { "" }, + ), + Applicability::MaybeIncorrect, + ); + } + } +} diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 3c9397441735..faef7e724dd0 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -259,15 +259,15 @@ fn is_unit_expr(expr: &ast::Expr) -> bool { fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - if let Some(rpos) = fn_source.rfind("->") { - #[allow(clippy::cast_possible_truncation)] - ( - ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), - Applicability::MachineApplicable, - ) - } else { - (ty.span, Applicability::MaybeIncorrect) - } + fn_source + .rfind("->") + .map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { + ( + #[allow(clippy::cast_possible_truncation)] + ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), + Applicability::MachineApplicable, + ) + }) } else { (ty.span, Applicability::MaybeIncorrect) }; diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 7da47ee4ff94..4cdff63f1180 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -165,14 +165,10 @@ fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: & fn is_binding(cx: &LateContext<'_>, pat_id: HirId) -> bool { let var_ty = cx.tables().node_type_opt(pat_id); - if let Some(var_ty) = var_ty { - match var_ty.kind { - ty::Adt(..) => false, - _ => true, - } - } else { - false - } + var_ty.map_or(false, |var_ty| match var_ty.kind { + ty::Adt(..) => false, + _ => true, + }) } fn check_pat<'tcx>( diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index b1345f0de5e4..d6f31a99bb36 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1205,16 +1205,19 @@ fn span_lossless_lint(cx: &LateContext<'_>, expr: &Expr<'_>, op: &Expr<'_>, cast // has parens on the outside, they are no longer needed. let mut applicability = Applicability::MachineApplicable; let opt = snippet_opt(cx, op.span); - let sugg = if let Some(ref snip) = opt { - if should_strip_parens(op, snip) { - &snip[1..snip.len() - 1] - } else { - snip.as_str() - } - } else { - applicability = Applicability::HasPlaceholders; - ".." - }; + let sugg = opt.as_ref().map_or_else( + || { + applicability = Applicability::HasPlaceholders; + ".." + }, + |snip| { + if should_strip_parens(op, snip) { + &snip[1..snip.len() - 1] + } else { + snip.as_str() + } + }, + ); span_lint_and_sugg( cx, diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index f85db1e2006e..39a8c0208728 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -167,14 +167,13 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { if let TyKind::Path(QPath::Resolved(_, ref item_path)) = item_type.kind; then { let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args; - let should_check = if let Some(ref params) = *parameters { - !params.parenthesized && !params.args.iter().any(|arg| match arg { + let should_check = parameters.as_ref().map_or( + true, + |params| !params.parenthesized && !params.args.iter().any(|arg| match arg { GenericArg::Lifetime(_) => true, _ => false, }) - } else { - true - }; + ); if should_check { let visitor = &mut UseSelfVisitor { diff --git a/clippy_lints/src/utils/attrs.rs b/clippy_lints/src/utils/attrs.rs index 4dcf6c105ec6..4bb4b087c556 100644 --- a/clippy_lints/src/utils/attrs.rs +++ b/clippy_lints/src/utils/attrs.rs @@ -65,42 +65,45 @@ pub fn get_attr<'a>( }; let attr_segments = &attr.path.segments; if attr_segments.len() == 2 && attr_segments[0].ident.to_string() == "clippy" { - if let Some(deprecation_status) = - BUILTIN_ATTRIBUTES - .iter() - .find_map(|(builtin_name, deprecation_status)| { - if *builtin_name == attr_segments[1].ident.to_string() { - Some(deprecation_status) - } else { - None - } - }) - { - let mut diag = sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute"); - match *deprecation_status { - DeprecationStatus::Deprecated => { - diag.emit(); - false - }, - DeprecationStatus::Replaced(new_name) => { - diag.span_suggestion( - attr_segments[1].ident.span, - "consider using", - new_name.to_string(), - Applicability::MachineApplicable, - ); - diag.emit(); + BUILTIN_ATTRIBUTES + .iter() + .find_map(|(builtin_name, deprecation_status)| { + if *builtin_name == attr_segments[1].ident.to_string() { + Some(deprecation_status) + } else { + None + } + }) + .map_or_else( + || { + sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute"); false }, - DeprecationStatus::None => { - diag.cancel(); - attr_segments[1].ident.to_string() == name + |deprecation_status| { + let mut diag = + sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute"); + match *deprecation_status { + DeprecationStatus::Deprecated => { + diag.emit(); + false + }, + DeprecationStatus::Replaced(new_name) => { + diag.span_suggestion( + attr_segments[1].ident.span, + "consider using", + new_name.to_string(), + Applicability::MachineApplicable, + ); + diag.emit(); + false + }, + DeprecationStatus::None => { + diag.cancel(); + attr_segments[1].ident.to_string() == name + }, + } }, - } - } else { - sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute"); - false - } + ) } else { false } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 99ba7d64331c..3a3b79925ff9 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -153,11 +153,7 @@ pub fn is_type_diagnostic_item(cx: &LateContext<'_>, ty: Ty<'_>, diag_item: Symb pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str]) -> bool { let def_id = cx.tables().type_dependent_def_id(expr.hir_id).unwrap(); let trt_id = cx.tcx.trait_of_item(def_id); - if let Some(trt_id) = trt_id { - match_def_path(cx, trt_id, path) - } else { - false - } + trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path)) } /// Checks if an expression references a variable of the given name. @@ -600,21 +596,15 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>( /// // ^^^^^^^^^^ /// ``` pub fn first_line_of_span(cx: &T, span: Span) -> Span { - if let Some(first_char_pos) = first_char_in_first_line(cx, span) { - span.with_lo(first_char_pos) - } else { - span - } + first_char_in_first_line(cx, span).map_or(span, |first_char_pos| span.with_lo(first_char_pos)) } fn first_char_in_first_line(cx: &T, span: Span) -> Option { let line_span = line_span(cx, span); - if let Some(snip) = snippet_opt(cx, line_span) { + snippet_opt(cx, line_span).and_then(|snip| { snip.find(|c: char| !c.is_whitespace()) .map(|pos| line_span.lo() + BytePos::from_usize(pos)) - } else { - None - } + }) } /// Returns the indentation of the line of a span @@ -626,11 +616,7 @@ fn first_char_in_first_line(cx: &T, span: Span) -> Option(cx: &T, span: Span) -> Option { - if let Some(snip) = snippet_opt(cx, line_span(cx, span)) { - snip.find(|c: char| !c.is_whitespace()) - } else { - None - } + snippet_opt(cx, line_span(cx, span)).and_then(|snip| snip.find(|c: char| !c.is_whitespace())) } /// Extends the span to the beginning of the spans line, incl. whitespaces. @@ -738,25 +724,21 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio let enclosing_node = map .get_enclosing_scope(hir_id) .and_then(|enclosing_id| map.find(enclosing_id)); - if let Some(node) = enclosing_node { - match node { - Node::Block(block) => Some(block), - Node::Item(&Item { - kind: ItemKind::Fn(_, _, eid), - .. - }) - | Node::ImplItem(&ImplItem { - kind: ImplItemKind::Fn(_, eid), - .. - }) => match cx.tcx.hir().body(eid).value.kind { - ExprKind::Block(ref block, _) => Some(block), - _ => None, - }, + enclosing_node.and_then(|node| match node { + Node::Block(block) => Some(block), + Node::Item(&Item { + kind: ItemKind::Fn(_, _, eid), + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Fn(_, eid), + .. + }) => match cx.tcx.hir().body(eid).value.kind { + ExprKind::Block(ref block, _) => Some(block), _ => None, - } - } else { - None - } + }, + _ => None, + }) } /// Returns the base type for HIR references and pointers. @@ -1328,11 +1310,7 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { _ => None, }; - if let Some(did) = did { - must_use_attr(&cx.tcx.get_attrs(did)).is_some() - } else { - false - } + did.map_or(false, |did| must_use_attr(&cx.tcx.get_attrs(did)).is_some()) } pub fn is_no_std_crate(krate: &Crate<'_>) -> bool { diff --git a/clippy_lints/src/utils/numeric_literal.rs b/clippy_lints/src/utils/numeric_literal.rs index 99413153d49b..7a79741b30bd 100644 --- a/clippy_lints/src/utils/numeric_literal.rs +++ b/clippy_lints/src/utils/numeric_literal.rs @@ -200,12 +200,10 @@ impl<'a> NumericLiteral<'a> { fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a str>) { debug_assert!(lit_kind.is_numeric()); - if let Some(suffix_length) = lit_suffix_length(lit_kind) { + lit_suffix_length(lit_kind).map_or((src, None), |suffix_length| { let (unsuffixed, suffix) = src.split_at(src.len() - suffix_length); (unsuffixed, Some(suffix)) - } else { - (src, None) - } + }) } fn lit_suffix_length(lit_kind: &LitKind) -> Option { diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 8fc97f2fd64a..0ac7714fbeb7 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -492,20 +492,20 @@ fn astbinop2assignop(op: ast::BinOp) -> AssocOp { /// before it on its line. fn indentation(cx: &T, span: Span) -> Option { let lo = cx.sess().source_map().lookup_char_pos(span.lo()); - if let Some(line) = lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) { - if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { - // We can mix char and byte positions here because we only consider `[ \t]`. - if lo.col == CharPos(pos) { - Some(line[..pos].into()) + lo.file + .get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) + .and_then(|line| { + if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { + // We can mix char and byte positions here because we only consider `[ \t]`. + if lo.col == CharPos(pos) { + Some(line[..pos].into()) + } else { + None + } } else { None } - } else { - None - } - } else { - None - } + }) } /// Convenience extension trait for `DiagnosticBuilder`. diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 732f4b28e06e..063f94582b9d 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -297,12 +297,13 @@ impl EarlyLintPass for Write { if let (Some(fmt_str), expr) = self.check_tts(cx, &mac.args.inner_tokens(), true) { if fmt_str.symbol == Symbol::intern("") { let mut applicability = Applicability::MachineApplicable; - let suggestion = if let Some(e) = expr { - snippet_with_applicability(cx, e.span, "v", &mut applicability) - } else { - applicability = Applicability::HasPlaceholders; - Cow::Borrowed("v") - }; + let suggestion = expr.map_or_else( + || { + applicability = Applicability::HasPlaceholders; + Cow::Borrowed("v") + }, + |e| snippet_with_applicability(cx, e.span, "v", &mut Applicability::MachineApplicable), + ); span_lint_and_sugg( cx, diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6402efc25212..e681f47f949d 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1620,6 +1620,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "option_env_unwrap", }, + Lint { + name: "option_if_let_else", + group: "pedantic", + desc: "reimplementation of Option::map_or", + deprecation: None, + module: "option_if_let_else", + }, Lint { name: "option_map_or_none", group: "style", diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 99505fc6b29b..eb6d495acbe2 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -12,19 +12,11 @@ use std::path::{Path, PathBuf}; mod cargo; fn host_lib() -> PathBuf { - if let Some(path) = option_env!("HOST_LIBS") { - PathBuf::from(path) - } else { - cargo::CARGO_TARGET_DIR.join(env!("PROFILE")) - } + option_env!("HOST_LIBS").map_or(cargo::CARGO_TARGET_DIR.join(env!("PROFILE")), PathBuf::from) } fn clippy_driver_path() -> PathBuf { - if let Some(path) = option_env!("CLIPPY_DRIVER_PATH") { - PathBuf::from(path) - } else { - cargo::TARGET_LIB.join("clippy-driver") - } + option_env!("CLIPPY_DRIVER_PATH").map_or(cargo::TARGET_LIB.join("clippy-driver"), PathBuf::from) } // When we'll want to use `extern crate ..` for a dependency that is used diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed new file mode 100644 index 000000000000..695a460cc4ed --- /dev/null +++ b/tests/ui/option_if_let_else.fixed @@ -0,0 +1,74 @@ +// run-rustfix +#![warn(clippy::option_if_let_else)] + +fn bad1(string: Option<&str>) -> (bool, &str) { + string.map_or((false, "hello"), |x| (true, x)) +} + +fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> { + if string.is_none() { + None + } else { string.map_or(Some((false, "")), |x| Some((true, x))) } +} + +fn unop_bad(string: &Option<&str>, mut num: Option) { + let _ = string.map_or(0, |s| s.len()); + let _ = num.as_ref().map_or(&0, |s| s); + let _ = num.as_mut().map_or(&mut 0, |s| { + *s += 1; + s + }); + let _ = num.as_ref().map_or(&0, |s| s); + let _ = num.map_or(0, |mut s| { + s += 1; + s + }); + let _ = num.as_mut().map_or(&mut 0, |s| { + *s += 1; + s + }); +} + +fn longer_body(arg: Option) -> u32 { + arg.map_or(13, |x| { + let y = x * x; + y * y + }) +} + +fn test_map_or_else(arg: Option) { + 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 { + 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 _ = else_if_option(None); + unop_bad(&None, None); + let _ = longer_body(None); + test_map_or_else(None); + let _ = negative_tests(None); +} diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs new file mode 100644 index 000000000000..dee80d26bd97 --- /dev/null +++ b/tests/ui/option_if_let_else.rs @@ -0,0 +1,92 @@ +// 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 else_if_option(string: Option<&str>) -> Option<(bool, &str)> { + if string.is_none() { + None + } else if let Some(x) = string { + Some((true, x)) + } else { + Some((false, "")) + } +} + +fn unop_bad(string: &Option<&str>, mut num: Option) { + let _ = if let Some(s) = *string { s.len() } else { 0 }; + let _ = if let Some(s) = &num { s } else { &0 }; + let _ = if let Some(s) = &mut num { + *s += 1; + s + } else { + &mut 0 + }; + let _ = if let Some(ref s) = num { s } else { &0 }; + let _ = if let Some(mut s) = num { + s += 1; + s + } else { + 0 + }; + let _ = if let Some(ref mut s) = num { + *s += 1; + s + } else { + &mut 0 + }; +} + +fn longer_body(arg: Option) -> u32 { + if let Some(x) = arg { + let y = x * x; + y * y + } else { + 13 + } +} + +fn test_map_or_else(arg: Option) { + 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 { + 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 _ = else_if_option(None); + unop_bad(&None, None); + let _ = longer_body(None); + test_map_or_else(None); + let _ = negative_tests(None); +} diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr new file mode 100644 index 000000000000..7005850efaf8 --- /dev/null +++ b/tests/ui/option_if_let_else.stderr @@ -0,0 +1,151 @@ +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:15:12 + | +LL | } else if let Some(x) = string { + | ____________^ +LL | | Some((true, x)) +LL | | } else { +LL | | Some((false, "")) +LL | | } + | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:23:13 + | +LL | let _ = if let Some(s) = *string { s.len() } else { 0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:24:13 + | +LL | let _ = if let Some(s) = &num { s } else { &0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:25:13 + | +LL | let _ = if let Some(s) = &mut num { + | _____________^ +LL | | *s += 1; +LL | | s +LL | | } else { +LL | | &mut 0 +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = num.as_mut().map_or(&mut 0, |s| { +LL | *s += 1; +LL | s +LL | }); + | + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:31:13 + | +LL | let _ = if let Some(ref s) = num { s } else { &0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:32:13 + | +LL | let _ = if let Some(mut s) = num { + | _____________^ +LL | | s += 1; +LL | | s +LL | | } else { +LL | | 0 +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = num.map_or(0, |mut s| { +LL | s += 1; +LL | s +LL | }); + | + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:38:13 + | +LL | let _ = if let Some(ref mut s) = num { + | _____________^ +LL | | *s += 1; +LL | | s +LL | | } else { +LL | | &mut 0 +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = num.as_mut().map_or(&mut 0, |s| { +LL | *s += 1; +LL | s +LL | }); + | + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:47: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:56: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:85: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 11 previous errors +