diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index a7899dc8b..46f06362c 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(l), .. })) = e.kind { + match some_captures.get(l) + .or_else(|| (method_sugg == "map_or_else").then(|| ()).and_then(|_| none_captures.get(l))) + { + 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 bd229402f..bb7de1653 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -709,6 +709,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 769ccc14b..56e032cdb 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -86,4 +86,46 @@ 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 + }; } diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index e2f8dec3b..595ed8172 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -105,4 +105,52 @@ 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 + }; } diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index 099e49ef8..803d941c3 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -148,5 +148,53 @@ error: use Option::map_or instead of an if let/else 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:109: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:137: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:141: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