manual_unwrap_or(_default): don't lint if not safe to move scrutinee

When matched against `Result` with copyable `Ok` variant, uncopyable scrutinee won't
actually be moved. But `manual_unwrap_or`/`manual_unwrap_or_default`
will force it to move, which can cause problem if scrutinee is used later.

changelog: [`manual_unwrap_or`]: don't lint if not safe to move scrutinee
changelog: [`manual_unwrap_or_default`]: don't lint if not safe to move scrutinee

Signed-off-by: Zihan <[email protected]>

Author: zihan0822

clippy_lints/src/matches/manual_unwrap_or.rs

@@ -10,8 +10,13 @@ use rustc_span::sym;
 
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::sugg::Sugg;
-use clippy_utils::ty::{expr_type_is_certain, get_type_diagnostic_name, implements_trait};
-use clippy_utils::{is_default_equivalent, is_lint_allowed, path_res, peel_blocks, span_contains_comment};
+use clippy_utils::ty::{
+    expr_type_is_certain, get_type_diagnostic_name, implements_trait, is_copy, is_type_diagnostic_item,
+};
+use clippy_utils::usage::local_used_after_expr;
+use clippy_utils::{
+    is_default_equivalent, is_lint_allowed, path_res, path_to_local, peel_blocks, span_contains_comment,
+};
 
 use super::{MANUAL_UNWRAP_OR, MANUAL_UNWRAP_OR_DEFAULT};
 
@@ -84,7 +89,9 @@ fn handle(
     binding_id: HirId,
 ) {
     // Only deal with situations where both alternatives return the same non-adjusted type.
-    if cx.typeck_results().expr_ty(body_some) != cx.typeck_results().expr_ty(body_none) {
+    if cx.typeck_results().expr_ty(body_some) != cx.typeck_results().expr_ty(body_none)
+        || !safe_to_move_scrutinee(cx, expr, condition)
+    {
         return;
     }
 
@@ -181,6 +188,29 @@ fn find_type_name<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'static
     }
 }
 
+/// Checks whether it is safe to move scrutinee.
+/// It is not safe to move if:
+///     1. `scrutinee` is a `Result` that doesn't implemenet `Copy`, mainly because the `Err`
+///        variant is not copyable.
+///     2. `expr` is a local variable that is used after the if-let-else expression.
+/// ```rust,ignore
+/// let foo: Result<usize, String> = Ok(0);
+/// let v = if let Ok(v) = foo { v } else { 1 };
+/// let bar = foo;
+/// ```
+fn safe_to_move_scrutinee(cx: &LateContext<'_>, expr: &Expr<'_>, scrutinee: &Expr<'_>) -> bool {
+    if let Some(hir_id) = path_to_local(scrutinee)
+        && let scrutinee_ty = cx.typeck_results().expr_ty(scrutinee)
+        && is_type_diagnostic_item(cx, scrutinee_ty, sym::Result)
+        && !is_copy(cx, scrutinee_ty)
+        && local_used_after_expr(cx, hir_id, expr)
+    {
+        false
+    } else {
+        true
+    }
+}
+
 pub fn check_match<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,

tests/ui/manual_unwrap_or.fixed

@@ -250,4 +250,18 @@ fn allowed_manual_unwrap_or_zero() -> u32 {
     Some(42).unwrap_or(0)
 }
 
+fn issue_15807() {
+    let uncopyable_res: Result<usize, String> = Ok(1);
+    let _ = if let Ok(v) = uncopyable_res { v } else { 2 };
+
+    let x = uncopyable_res;
+    let _ = x.unwrap_or(2);
+    //~^ manual_unwrap_or
+
+    let copyable_res: Result<usize, ()> = Ok(1);
+    let _ = copyable_res.unwrap_or(2);
+    //~^ manual_unwrap_or
+    let _ = copyable_res;
+}
+
 fn main() {}

tests/ui/manual_unwrap_or.rs

@@ -329,4 +329,18 @@ fn allowed_manual_unwrap_or_zero() -> u32 {
     }
 }
 
+fn issue_15807() {
+    let uncopyable_res: Result<usize, String> = Ok(1);
+    let _ = if let Ok(v) = uncopyable_res { v } else { 2 };
+
+    let x = uncopyable_res;
+    let _ = if let Ok(v) = x { v } else { 2 };
+    //~^ manual_unwrap_or
+
+    let copyable_res: Result<usize, ()> = Ok(1);
+    let _ = if let Ok(v) = copyable_res { v } else { 2 };
+    //~^ manual_unwrap_or
+    let _ = copyable_res;
+}
+
 fn main() {}

tests/ui/manual_unwrap_or.stderr

@@ -201,5 +201,17 @@ LL | |         0
 LL | |     }
    | |_____^ help: replace with: `Some(42).unwrap_or(0)`
 
-error: aborting due to 18 previous errors
+error: this pattern reimplements `Result::unwrap_or`
+  --> tests/ui/manual_unwrap_or.rs:337:13
+   |
+LL |     let _ = if let Ok(v) = x { v } else { 2 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `x.unwrap_or(2)`
+
+error: this pattern reimplements `Result::unwrap_or`
+  --> tests/ui/manual_unwrap_or.rs:341:13
+   |
+LL |     let _ = if let Ok(v) = copyable_res { v } else { 2 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `copyable_res.unwrap_or(2)`
+
+error: aborting due to 20 previous errors
 

tests/ui/manual_unwrap_or_default.fixed

@@ -119,3 +119,17 @@ mod issue14716 {
         };
     }
 }
+
+fn issue_15807() {
+    let uncopyable_res: Result<usize, String> = Ok(1);
+    let _ = if let Ok(v) = uncopyable_res { v } else { 0 };
+
+    let x = uncopyable_res;
+    let _ = x.unwrap_or_default();
+    //~^ manual_unwrap_or_default
+
+    let copyable_res: Result<usize, ()> = Ok(1);
+    let _ = copyable_res.unwrap_or_default();
+    //~^ manual_unwrap_or_default
+    let _ = copyable_res;
+}

tests/ui/manual_unwrap_or_default.rs

@@ -160,3 +160,17 @@ mod issue14716 {
         };
     }
 }
+
+fn issue_15807() {
+    let uncopyable_res: Result<usize, String> = Ok(1);
+    let _ = if let Ok(v) = uncopyable_res { v } else { 0 };
+
+    let x = uncopyable_res;
+    let _ = if let Ok(v) = x { v } else { 0 };
+    //~^ manual_unwrap_or_default
+
+    let copyable_res: Result<usize, ()> = Ok(1);
+    let _ = if let Ok(v) = copyable_res { v } else { 0 };
+    //~^ manual_unwrap_or_default
+    let _ = copyable_res;
+}

tests/ui/manual_unwrap_or_default.stderr

@@ -97,5 +97,17 @@ LL | |         0
 LL | |     }
    | |_____^ help: replace it with: `Some(42).unwrap_or_default()`
 
-error: aborting due to 9 previous errors
+error: if let can be simplified with `.unwrap_or_default()`
+  --> tests/ui/manual_unwrap_or_default.rs:169:13
+   |
+LL |     let _ = if let Ok(v) = x { v } else { 0 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x.unwrap_or_default()`
+
+error: if let can be simplified with `.unwrap_or_default()`
+  --> tests/ui/manual_unwrap_or_default.rs:173:13
+   |
+LL |     let _ = if let Ok(v) = copyable_res { v } else { 0 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `copyable_res.unwrap_or_default()`
+
+error: aborting due to 11 previous errors