diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 9494efe736cc..0a9fa6df1ab6 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -4,8 +4,9 @@ use crate::utils::{match_type, paths, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; +use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; -use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp}; +use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, Path, QPath, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -13,22 +14,16 @@ 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). + /// idiomatically done with `Option::map_or` (if the else bit is a pure + /// expression) or `Option::map_or_else` (if the else bit is an impure + /// expresion). /// /// **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 + /// 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. @@ -92,6 +87,7 @@ struct OptionIfLetElseOccurence { struct ReturnBreakContinueMacroVisitor { seen_return_break_continue: bool, } + impl ReturnBreakContinueMacroVisitor { fn new() -> ReturnBreakContinueMacroVisitor { ReturnBreakContinueMacroVisitor { @@ -99,6 +95,7 @@ impl ReturnBreakContinueMacroVisitor { } } } + impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor { type Map = Map<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { @@ -157,7 +154,7 @@ fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> { } /// If this is the else body of an if/else expression, then we need to wrap -/// it in curcly braces. Otherwise, we don't. +/// it in curly 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 { @@ -196,6 +193,35 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo ) } +/// Is the expr pure (is it free from side-effects)? +/// If yes, we can use `map_or`, else we must use `map_or_else` to preserve the behavior. +/// This implementation can be improved and identify more pure patterns. +fn is_pure(expr: &Expr<'_>) -> bool { + match expr.kind { + ExprKind::Lit(..) | ExprKind::Path(..) => true, + ExprKind::AddrOf(_, _, addr_of_expr) => is_pure(addr_of_expr), + ExprKind::Tup(tup_exprs) => tup_exprs.iter().all(|expr| is_pure(expr)), + ExprKind::Struct(_, fields, expr) => { + fields.iter().all(|f| is_pure(f.expr)) && expr.map_or(true, |e| is_pure(e)) + }, + ExprKind::Call( + &Expr { + kind: + ExprKind::Path(QPath::Resolved( + _, + Path { + res: Res::Def(DefKind::Ctor(..) | DefKind::Variant, ..), + .. + }, + )), + .. + }, + args, + ) => args.iter().all(|expr| is_pure(expr)), + _ => false, + } +} + /// 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. @@ -214,10 +240,7 @@ fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option "map_or_else", - _ => "map_or", - }; + let method_sugg = if is_pure(none_body) { "map_or" } else { "map_or_else" }; 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 { diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 695a460cc4ed..a7fb00a27057 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -1,5 +1,6 @@ // run-rustfix #![warn(clippy::option_if_let_else)] +#![allow(clippy::redundant_closure)] fn bad1(string: Option<&str>) -> (bool, &str) { string.map_or((false, "hello"), |x| (true, x)) @@ -36,6 +37,14 @@ fn longer_body(arg: Option) -> u32 { }) } +fn impure_else(arg: Option) { + let side_effect = || { + println!("return 1"); + 1 + }; + let _ = arg.map_or_else(|| side_effect(), |x| x); +} + fn test_map_or_else(arg: Option) { let _ = arg.map_or_else(|| { let mut y = 1; @@ -71,4 +80,5 @@ fn main() { let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); + let _ = impure_else(None); } diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index dee80d26bd97..895fd86321fa 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -1,5 +1,6 @@ // run-rustfix #![warn(clippy::option_if_let_else)] +#![allow(clippy::redundant_closure)] fn bad1(string: Option<&str>) -> (bool, &str) { if let Some(x) = string { @@ -52,6 +53,19 @@ fn longer_body(arg: Option) -> u32 { } } +fn impure_else(arg: Option) { + let side_effect = || { + println!("return 1"); + 1 + }; + let _ = if let Some(x) = arg { + x + } else { + // map_or_else must be suggested + side_effect() + }; +} + fn test_map_or_else(arg: Option) { let _ = if let Some(x) = arg { x * x * x * x @@ -89,4 +103,5 @@ fn main() { let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); + let _ = impure_else(None); } diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index 7005850efaf8..b69fe7676827 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -1,5 +1,5 @@ error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:5:5 + --> $DIR/option_if_let_else.rs:6:5 | LL | / if let Some(x) = string { LL | | (true, x) @@ -11,7 +11,7 @@ LL | | } = 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 + --> $DIR/option_if_let_else.rs:16:12 | LL | } else if let Some(x) = string { | ____________^ @@ -22,19 +22,19 @@ 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 + --> $DIR/option_if_let_else.rs:24: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 + --> $DIR/option_if_let_else.rs:25: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 + --> $DIR/option_if_let_else.rs:26:13 | LL | let _ = if let Some(s) = &mut num { | _____________^ @@ -54,13 +54,13 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:31:13 + --> $DIR/option_if_let_else.rs:32: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 + --> $DIR/option_if_let_else.rs:33:13 | LL | let _ = if let Some(mut s) = num { | _____________^ @@ -80,7 +80,7 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:38:13 + --> $DIR/option_if_let_else.rs:39:13 | LL | let _ = if let Some(ref mut s) = num { | _____________^ @@ -100,7 +100,7 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:47:5 + --> $DIR/option_if_let_else.rs:48:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -119,7 +119,19 @@ LL | }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:56:13 + --> $DIR/option_if_let_else.rs:61:13 + | +LL | let _ = if let Some(x) = arg { + | _____________^ +LL | | x +LL | | } else { +LL | | // map_or_else must be suggested +LL | | side_effect() +LL | | }; + | |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)` + +error: use Option::map_or_else instead of an if let/else + --> $DIR/option_if_let_else.rs:70:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -142,10 +154,10 @@ 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 + --> $DIR/option_if_let_else.rs:99: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 +error: aborting due to 12 previous errors