From aff6178b1fb88fe38f13dcd38376ec4d79759da4 Mon Sep 17 00:00:00 2001 From: Zihan Date: Sat, 4 Oct 2025 12:56:27 -0400 Subject: [PATCH] `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 --- clippy_lints/src/matches/manual_unwrap_or.rs | 36 ++++++++++++++++++-- tests/ui/manual_unwrap_or.fixed | 14 ++++++++ tests/ui/manual_unwrap_or.rs | 14 ++++++++ tests/ui/manual_unwrap_or.stderr | 14 +++++++- tests/ui/manual_unwrap_or_default.fixed | 14 ++++++++ tests/ui/manual_unwrap_or_default.rs | 14 ++++++++ tests/ui/manual_unwrap_or_default.stderr | 14 +++++++- 7 files changed, 115 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/matches/manual_unwrap_or.rs b/clippy_lints/src/matches/manual_unwrap_or.rs index 8c3f52542d91..11534d359eff 100644 --- a/clippy_lints/src/matches/manual_unwrap_or.rs +++ b/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 = 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>, diff --git a/tests/ui/manual_unwrap_or.fixed b/tests/ui/manual_unwrap_or.fixed index e12287a70939..8dd9e7a8a6fa 100644 --- a/tests/ui/manual_unwrap_or.fixed +++ b/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 = 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 = Ok(1); + let _ = copyable_res.unwrap_or(2); + //~^ manual_unwrap_or + let _ = copyable_res; +} + fn main() {} diff --git a/tests/ui/manual_unwrap_or.rs b/tests/ui/manual_unwrap_or.rs index 53cffcab5b56..f8e2c5f43745 100644 --- a/tests/ui/manual_unwrap_or.rs +++ b/tests/ui/manual_unwrap_or.rs @@ -329,4 +329,18 @@ fn allowed_manual_unwrap_or_zero() -> u32 { } } +fn issue_15807() { + let uncopyable_res: Result = 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 = Ok(1); + let _ = if let Ok(v) = copyable_res { v } else { 2 }; + //~^ manual_unwrap_or + let _ = copyable_res; +} + fn main() {} diff --git a/tests/ui/manual_unwrap_or.stderr b/tests/ui/manual_unwrap_or.stderr index 320e895fb823..18764d1501d8 100644 --- a/tests/ui/manual_unwrap_or.stderr +++ b/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 diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed index 189fe876aa5d..5ac3bd67968b 100644 --- a/tests/ui/manual_unwrap_or_default.fixed +++ b/tests/ui/manual_unwrap_or_default.fixed @@ -119,3 +119,17 @@ mod issue14716 { }; } } + +fn issue_15807() { + let uncopyable_res: Result = 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 = Ok(1); + let _ = copyable_res.unwrap_or_default(); + //~^ manual_unwrap_or_default + let _ = copyable_res; +} diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs index ca87926763c9..9ba36f06ab98 100644 --- a/tests/ui/manual_unwrap_or_default.rs +++ b/tests/ui/manual_unwrap_or_default.rs @@ -160,3 +160,17 @@ mod issue14716 { }; } } + +fn issue_15807() { + let uncopyable_res: Result = 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 = Ok(1); + let _ = if let Ok(v) = copyable_res { v } else { 0 }; + //~^ manual_unwrap_or_default + let _ = copyable_res; +} diff --git a/tests/ui/manual_unwrap_or_default.stderr b/tests/ui/manual_unwrap_or_default.stderr index e8f38a2e3899..3627b64a5d1b 100644 --- a/tests/ui/manual_unwrap_or_default.stderr +++ b/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