Fix option_if_let_else

Jarcho · Jarcho · commit 8b3ca9a315ae · 2021-08-16T16:12:00.000-04:00
* `break` and `continue` statments local to the would-be closure are allowed
* don't lint in const contexts
* don't lint when yield expressions are used
* don't lint when the captures made by the would-be closure conflict with the other branch
* don't lint when a field of a local is used when the type could be pontentially moved from
* in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure
diff --git 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<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(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
@@ -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
@@ -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>) -> 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
@@ -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>) -> 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
@@ -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
 
