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

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.

Fixes #15807

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

Author: flip1995

clippy_lints/src/matches/manual_unwrap_or.rs

@@ -1,5 +1,5 @@
 use clippy_utils::consts::ConstEvalCtxt;
-use clippy_utils::res::{MaybeDef, MaybeQPath};
+use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
 use clippy_utils::source::{SpanRangeExt as _, indent_of, reindent_multiline};
 use rustc_ast::{BindingMode, ByRef};
 use rustc_errors::Applicability;
@@ -11,7 +11,8 @@ 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, implements_trait};
+use clippy_utils::ty::{expr_type_is_certain, implements_trait, is_copy};
+use clippy_utils::usage::local_used_after_expr;
 use clippy_utils::{is_default_equivalent, is_lint_allowed, peel_blocks, span_contains_comment};
 
 use super::{MANUAL_UNWRAP_OR, MANUAL_UNWRAP_OR_DEFAULT};
@@ -87,7 +88,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;
     }
 
@@ -185,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) = scrutinee.res_local_id()
+        && let scrutinee_ty = cx.typeck_results().expr_ty(scrutinee)
+        && scrutinee_ty.is_diag_item(cx, 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

@@ -128,3 +128,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

@@ -169,3 +169,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:178: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:182: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