Auto merge of rust-lang#7573 - Jarcho:option_if_let_else, r=giraffate

Fix `option_if_let_else`

fixes: rust-lang#5822
fixes: rust-lang#6737
fixes: rust-lang#7567

The inference from rust-lang#6137 still exists so I'm not sure if this should be moved from the nursery. Before doing that though I'd almost want to see this split into two lints. One suggesting `map_or` and the other suggesting `map_or_else`.

`map_or_else` tends to have longer expressions for both branches so it doesn't end up much shorter than a match expression in practice. It also seems most people find it harder to read. `map_or` at least has the terseness benefit of being on one line most of the time, especially when the `None` branch is just a literal or path expression.

changelog: `break` and `continue` statments local to the would-be closure are allowed in `option_if_let_else`
changelog: don't lint in const contexts  in `option_if_let_else`
changelog: don't lint when yield expressions are used  in `option_if_let_else`
changelog: don't lint when the captures made by the would-be closure conflict with the other branch  in `option_if_let_else`
changelog: don't lint when a field of a local is used when the type could be pontentially moved from  in `option_if_let_else`
changelog: in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure  in `option_if_let_else`

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<OptionIfLetElseOccurence> {
     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 `<local>[.<field>]*` 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(),

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 {

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>) -> 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 };
+    }
 }

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>) -> 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 };
+    }
 }

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