diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index a7899dc8bc12..0614c14462b7 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -1,12 +1,16 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::usage::contains_return_break_continue_macro; -use clippy_utils::{eager_or_lazy, in_macro, is_else_clause, is_lang_ctor}; +use clippy_utils::{ + can_move_expr_to_closure, eager_or_lazy, in_constant, in_macro, is_else_clause, is_lang_ctor, peel_hir_expr_while, + CaptureKind, +}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionSome; -use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp}; +use rustc_hir::{ + def::Res, Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, Path, QPath, UnOp, +}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::sym; @@ -127,21 +131,30 @@ fn detect_option_if_let_else<'tcx>( ) -> Option { if_chain! { if !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 !in_constant(cx, expr.hir_id); + if let ExprKind::Match(cond_expr, [some_arm, none_arm], MatchSource::IfLetDesugar{contains_else_clause: true}) + = &expr.kind; if !is_else_clause(cx.tcx, expr); - 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 let PatKind::TupleStruct(struct_qpath, [inner_pat], _) = &some_arm.pat.kind; if is_lang_ctor(cx, struct_qpath, OptionSome); 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); + if let Some(some_captures) = can_move_expr_to_closure(cx, some_arm.body); + if let Some(none_captures) = can_move_expr_to_closure(cx, none_arm.body); + if some_captures + .iter() + .filter_map(|(id, &c)| none_captures.get(id).map(|&c2| (c, c2))) + .all(|(x, y)| x.is_imm_ref() && y.is_imm_ref()); 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 = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { "map_or" } else { "map_or_else" }; + let some_body = extract_body_from_arm(some_arm)?; + let none_body = extract_body_from_arm(none_arm)?; + let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { + "map_or" + } else { + "map_or_else" + }; let capture_name = id.name.to_ident_string(); let (as_ref, as_mut) = match &cond_expr.kind { ExprKind::AddrOf(_, Mutability::Not, _) => (true, false), @@ -153,6 +166,24 @@ fn detect_option_if_let_else<'tcx>( ExprKind::Unary(UnOp::Deref, expr) | ExprKind::AddrOf(_, _, expr) => expr, _ => cond_expr, }; + // Check if captures the closure will need conflict with borrows made in the scrutinee. + // TODO: check all the references made in the scrutinee expression. This will require interacting + // with the borrow checker. Currently only `[.]*` is checked for. + if as_ref || as_mut { + let e = peel_hir_expr_while(cond_expr, |e| match e.kind { + ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }); + if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(local_id), .. })) = e.kind { + match some_captures.get(local_id) + .or_else(|| (method_sugg == "map_or_else").then(|| ()).and_then(|_| none_captures.get(local_id))) + { + Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return None, + Some(CaptureKind::Ref(Mutability::Not)) if as_mut => return None, + Some(CaptureKind::Ref(Mutability::Not)) | None => (), + } + } + } Some(OptionIfLetElseOccurence { option: format_option_in_sugg(cx, cond_expr, as_ref, as_mut), method_sugg: method_sugg.to_string(), diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 603e831459d3..4d8d4709e04a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -710,6 +710,11 @@ pub enum CaptureKind { Value, Ref(Mutability), } +impl CaptureKind { + pub fn is_imm_ref(self) -> bool { + self == Self::Ref(Mutability::Not) + } +} impl std::ops::BitOr for CaptureKind { type Output = Self; fn bitor(self, rhs: Self) -> Self::Output { diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 769ccc14bc1e..d1815d0aec33 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -1,3 +1,4 @@ +// edition:2018 // run-rustfix #![warn(clippy::option_if_let_else)] #![allow(clippy::redundant_closure)] @@ -86,4 +87,65 @@ fn main() { test_map_or_else(None); let _ = negative_tests(None); let _ = impure_else(None); + + let _ = Some(0).map_or(0, |x| loop { + if x == 0 { + break x; + } + }); + + // #7576 + const fn _f(x: Option) -> u32 { + // Don't lint, `map_or` isn't const + if let Some(x) = x { x } else { 10 } + } + + // #5822 + let s = String::new(); + // Don't lint, `Some` branch consumes `s`, but else branch uses `s` + let _ = if let Some(x) = Some(0) { + let s = s; + s.len() + x + } else { + s.len() + }; + + let s = String::new(); + // Lint, both branches immutably borrow `s`. + let _ = Some(0).map_or_else(|| s.len(), |x| s.len() + x); + + let s = String::new(); + // Lint, `Some` branch consumes `s`, but else branch doesn't use `s`. + let _ = Some(0).map_or(1, |x| { + let s = s; + s.len() + x + }); + + let s = Some(String::new()); + // Don't lint, `Some` branch borrows `s`, but else branch consumes `s` + let _ = if let Some(x) = &s { + x.len() + } else { + let _s = s; + 10 + }; + + let mut s = Some(String::new()); + // Don't lint, `Some` branch mutably borrows `s`, but else branch also borrows `s` + let _ = if let Some(x) = &mut s { + x.push_str("test"); + x.len() + } else { + let _s = &s; + 10 + }; + + async fn _f1(x: u32) -> u32 { + x + } + + async fn _f2() { + // Don't lint. `await` can't be moved into a closure. + let _ = if let Some(x) = Some(0) { _f1(x).await } else { 0 }; + } } diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index e2f8dec3b930..a15627338cb4 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -1,3 +1,4 @@ +// edition:2018 // run-rustfix #![warn(clippy::option_if_let_else)] #![allow(clippy::redundant_closure)] @@ -105,4 +106,71 @@ fn main() { test_map_or_else(None); let _ = negative_tests(None); let _ = impure_else(None); + + let _ = if let Some(x) = Some(0) { + loop { + if x == 0 { + break x; + } + } + } else { + 0 + }; + + // #7576 + const fn _f(x: Option) -> u32 { + // Don't lint, `map_or` isn't const + if let Some(x) = x { x } else { 10 } + } + + // #5822 + let s = String::new(); + // Don't lint, `Some` branch consumes `s`, but else branch uses `s` + let _ = if let Some(x) = Some(0) { + let s = s; + s.len() + x + } else { + s.len() + }; + + let s = String::new(); + // Lint, both branches immutably borrow `s`. + let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() }; + + let s = String::new(); + // Lint, `Some` branch consumes `s`, but else branch doesn't use `s`. + let _ = if let Some(x) = Some(0) { + let s = s; + s.len() + x + } else { + 1 + }; + + let s = Some(String::new()); + // Don't lint, `Some` branch borrows `s`, but else branch consumes `s` + let _ = if let Some(x) = &s { + x.len() + } else { + let _s = s; + 10 + }; + + let mut s = Some(String::new()); + // Don't lint, `Some` branch mutably borrows `s`, but else branch also borrows `s` + let _ = if let Some(x) = &mut s { + x.push_str("test"); + x.len() + } else { + let _s = &s; + 10 + }; + + async fn _f1(x: u32) -> u32 { + x + } + + async fn _f2() { + // Don't lint. `await` can't be moved into a closure. + let _ = if let Some(x) = Some(0) { _f1(x).await } else { 0 }; + } } diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index 099e49ef8e3d..ed748ee8b39e 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:7:5 + --> $DIR/option_if_let_else.rs:8:5 | LL | / if let Some(x) = string { LL | | (true, x) @@ -11,19 +11,19 @@ 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:25:13 + --> $DIR/option_if_let_else.rs:26: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:26:13 + --> $DIR/option_if_let_else.rs:27: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:27:13 + --> $DIR/option_if_let_else.rs:28:13 | LL | let _ = if let Some(s) = &mut num { | _____________^ @@ -43,13 +43,13 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:33:13 + --> $DIR/option_if_let_else.rs:34: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:34:13 + --> $DIR/option_if_let_else.rs:35:13 | LL | let _ = if let Some(mut s) = num { | _____________^ @@ -69,7 +69,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:40:13 + --> $DIR/option_if_let_else.rs:41:13 | LL | let _ = if let Some(ref mut s) = num { | _____________^ @@ -89,7 +89,7 @@ LL ~ }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:49:5 + --> $DIR/option_if_let_else.rs:50:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -108,7 +108,7 @@ LL + }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:62:13 + --> $DIR/option_if_let_else.rs:63:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -120,7 +120,7 @@ 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:71:13 + --> $DIR/option_if_let_else.rs:72:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -143,10 +143,58 @@ LL ~ }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:100:13 + --> $DIR/option_if_let_else.rs:101: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: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:110:13 + | +LL | let _ = if let Some(x) = Some(0) { + | _____________^ +LL | | loop { +LL | | if x == 0 { +LL | | break x; +... | +LL | | 0 +LL | | }; + | |_____^ + | +help: try + | +LL ~ let _ = Some(0).map_or(0, |x| loop { +LL + if x == 0 { +LL + break x; +LL + } +LL ~ }); + | + +error: use Option::map_or_else instead of an if let/else + --> $DIR/option_if_let_else.rs:138:13 + | +LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or_else(|| s.len(), |x| s.len() + x)` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:142:13 + | +LL | let _ = if let Some(x) = Some(0) { + | _____________^ +LL | | let s = s; +LL | | s.len() + x +LL | | } else { +LL | | 1 +LL | | }; + | |_____^ + | +help: try + | +LL ~ let _ = Some(0).map_or(1, |x| { +LL + let s = s; +LL + s.len() + x +LL ~ }); + | + +error: aborting due to 14 previous errors