fix [manual_unwrap_or_default] suggestion ignoring side-effects

rust-lang · Mar 28, 2024 · 6338925 · 6338925
1 parent 124e68b
commit 6338925
Show file tree

Hide file tree

Showing 4 changed files with 137 additions and 24 deletions.
diff --git a/clippy_lints/src/manual_unwrap_or_default.rs b/clippy_lints/src/manual_unwrap_or_default.rs
@@ -2,14 +2,14 @@ use clippy_utils::sugg::Sugg;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatKind, QPath};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::declare_lint_pass;
 use rustc_span::sym;
 
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{in_constant, is_default_equivalent};
+use clippy_utils::{in_constant, is_default_equivalent, peel_blocks_with_stmt, span_contains_comment};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -112,18 +112,19 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
     // We don't want conditions on the arms to simplify things.
     if arm1.guard.is_none()
         && arm2.guard.is_none()
+        && !span_contains_comment(cx.sess().source_map(), expr.span)
         // We check that the returned type implements the `Default` trait.
         && let match_ty = cx.typeck_results().expr_ty(expr)
         && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
         && implements_trait(cx, match_ty, default_trait_id, &[])
         // We now get the bodies for both the `Some` and `None` arms.
         && let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2)
         // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`.
-        && let ExprKind::Path(QPath::Resolved(_, path)) = body_some.peel_blocks().kind
+        && let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks_with_stmt(body_some).kind
         && let Res::Local(local_id) = path.res
         && local_id == binding_id
         // We now check the `None` arm is calling a method equivalent to `Default::default`.
-        && let body_none = body_none.peel_blocks()
+        && let body_none = peel_blocks_with_stmt(body_none)
         && is_default_equivalent(cx, body_none)
         && let Some(receiver) = Sugg::hir_opt(cx, match_expr).map(Sugg::maybe_par)
     {
@@ -144,17 +145,18 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
     if let ExprKind::If(cond, if_block, Some(else_expr)) = expr.kind
         && let ExprKind::Let(let_) = cond.kind
         && let ExprKind::Block(_, _) = else_expr.kind
+        && !span_contains_comment(cx.sess().source_map(), expr.span)
         // We check that the returned type implements the `Default` trait.
         && let match_ty = cx.typeck_results().expr_ty(expr)
         && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
         && implements_trait(cx, match_ty, default_trait_id, &[])
         && let Some(binding_id) = get_some(cx, let_.pat)
         // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`.
-        && let ExprKind::Path(QPath::Resolved(_, path)) = if_block.peel_blocks().kind
+        && let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks_with_stmt(if_block).kind
         && let Res::Local(local_id) = path.res
         && local_id == binding_id
         // We now check the `None` arm is calling a method equivalent to `Default::default`.
-        && let body_else = else_expr.peel_blocks()
+        && let body_else = peel_blocks_with_stmt(else_expr)
         && is_default_equivalent(cx, body_else)
         && let Some(if_let_expr_snippet) = snippet_opt(cx, let_.init.span)
     {

diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed
@@ -3,25 +3,30 @@
 
 fn main() {
     let x: Option<Vec<String>> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     x.unwrap_or_default();
 
     let x: Option<Vec<String>> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     x.unwrap_or_default();
 
     let x: Option<String> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     x.unwrap_or_default();
 
     let x: Option<Vec<String>> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     x.unwrap_or_default();
 
     let x: Option<Vec<String>> = None;
+    //~v ERROR: if let can be simplified with `.unwrap_or_default()`
     x.unwrap_or_default();
 }
 
 // Issue #12531
 unsafe fn no_deref_ptr(a: Option<i32>, b: *const Option<i32>) -> i32 {
+    // `*b` being correct depends on `a == Some(_)`
     match a {
-        // `*b` being correct depends on `a == Some(_)`
         Some(_) => (*b).unwrap_or_default(),
         _ => 0,
     }
@@ -33,3 +38,42 @@ const fn issue_12568(opt: Option<bool>) -> bool {
         None => false,
     }
 }
+
+fn issue_12569() {
+    let match_none_se = match 1u32.checked_div(0) {
+        Some(v) => v,
+        None => {
+            println!("important");
+            0
+        }
+    };
+    let match_some_se = match 1u32.checked_div(0) {
+        Some(v) => {
+            println!("important");
+            v
+        },
+        None => 0
+    };
+    #[allow(clippy::manual_unwrap_or)]
+    let match_none_cm = match 1u32.checked_div(0) {
+        Some(v) => v,
+        None => {
+            // Important
+            0
+        }
+    };
+    let match_middle_cm = 1u32.checked_div(0).unwrap_or(0);
+    let match_middle_doc_cm = 1u32.checked_div(0).unwrap_or(0);
+    let iflet_else_se = if let Some(v) = 1u32.checked_div(0) {
+        v
+    } else {
+        println!("important");
+        0
+    };
+    let iflet_then_cm = if let Some(v) = 1u32.checked_div(0) {
+        // Important
+        v
+    } else {
+        0
+    };
+}
diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs
@@ -3,36 +3,36 @@
 
 fn main() {
     let x: Option<Vec<String>> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     match x {
-        //~^ ERROR: match can be simplified with `.unwrap_or_default()`
         Some(v) => v,
         None => Vec::default(),
     };
 
     let x: Option<Vec<String>> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     match x {
-        //~^ ERROR: match can be simplified with `.unwrap_or_default()`
         Some(v) => v,
         _ => Vec::default(),
     };
 
     let x: Option<String> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     match x {
-        //~^ ERROR: match can be simplified with `.unwrap_or_default()`
         Some(v) => v,
         None => String::new(),
     };
 
     let x: Option<Vec<String>> = None;
+    //~v ERROR: match can be simplified with `.unwrap_or_default()`
     match x {
-        //~^ ERROR: match can be simplified with `.unwrap_or_default()`
         None => Vec::default(),
         Some(v) => v,
     };
 
     let x: Option<Vec<String>> = None;
+    //~v ERROR: if let can be simplified with `.unwrap_or_default()`
     if let Some(v) = x {
-        //~^ ERROR: if let can be simplified with `.unwrap_or_default()`
         v
     } else {
         Vec::default()
@@ -41,8 +41,8 @@ fn main() {
 
 // Issue #12531
 unsafe fn no_deref_ptr(a: Option<i32>, b: *const Option<i32>) -> i32 {
+    // `*b` being correct depends on `a == Some(_)`
     match a {
-        // `*b` being correct depends on `a == Some(_)`
         Some(_) => match *b {
             Some(v) => v,
             _ => 0,
@@ -57,3 +57,50 @@ const fn issue_12568(opt: Option<bool>) -> bool {
         None => false,
     }
 }
+
+fn issue_12569() {
+    let match_none_se = match 1u32.checked_div(0) {
+        Some(v) => v,
+        None => {
+            println!("important");
+            0
+        }
+    };
+    let match_some_se = match 1u32.checked_div(0) {
+        Some(v) => {
+            println!("important");
+            v
+        },
+        None => 0
+    };
+    #[allow(clippy::manual_unwrap_or)]
+    let match_none_cm = match 1u32.checked_div(0) {
+        Some(v) => v,
+        None => {
+            // Important
+            0
+        }
+    };
+    let match_middle_cm = match 1u32.checked_div(0) {
+        Some(v) => v,
+        // Important
+        None => 0
+    };
+    let match_middle_doc_cm = match 1u32.checked_div(0) {
+        /// Important
+        Some(v) => v,
+        None => 0
+    };
+    let iflet_else_se = if let Some(v) = 1u32.checked_div(0) {
+        v
+    } else {
+        println!("important");
+        0
+    };
+    let iflet_then_cm = if let Some(v) = 1u32.checked_div(0) {
+        // Important
+        v
+    } else {
+        0
+    };
+}
diff --git a/tests/ui/manual_unwrap_or_default.stderr b/tests/ui/manual_unwrap_or_default.stderr
@@ -1,8 +1,7 @@
 error: match can be simplified with `.unwrap_or_default()`
-  --> tests/ui/manual_unwrap_or_default.rs:6:5
+  --> tests/ui/manual_unwrap_or_default.rs:7:5
    |
 LL | /     match x {
-LL | |
 LL | |         Some(v) => v,
 LL | |         None => Vec::default(),
 LL | |     };
@@ -12,40 +11,36 @@ LL | |     };
    = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or_default)]`
 
 error: match can be simplified with `.unwrap_or_default()`
-  --> tests/ui/manual_unwrap_or_default.rs:13:5
+  --> tests/ui/manual_unwrap_or_default.rs:14:5
    |
 LL | /     match x {
-LL | |
 LL | |         Some(v) => v,
 LL | |         _ => Vec::default(),
 LL | |     };
    | |_____^ help: replace it with: `x.unwrap_or_default()`
 
 error: match can be simplified with `.unwrap_or_default()`
-  --> tests/ui/manual_unwrap_or_default.rs:20:5
+  --> tests/ui/manual_unwrap_or_default.rs:21:5
    |
 LL | /     match x {
-LL | |
 LL | |         Some(v) => v,
 LL | |         None => String::new(),
 LL | |     };
    | |_____^ help: replace it with: `x.unwrap_or_default()`
 
 error: match can be simplified with `.unwrap_or_default()`
-  --> tests/ui/manual_unwrap_or_default.rs:27:5
+  --> tests/ui/manual_unwrap_or_default.rs:28:5
    |
 LL | /     match x {
-LL | |
 LL | |         None => Vec::default(),
 LL | |         Some(v) => v,
 LL | |     };
    | |_____^ 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:34:5
+  --> tests/ui/manual_unwrap_or_default.rs:35:5
    |
 LL | /     if let Some(v) = x {
-LL | |
 LL | |         v
 LL | |     } else {
 LL | |         Vec::default()
@@ -62,5 +57,30 @@ LL | |             _ => 0,
 LL | |         },
    | |_________^ help: replace it with: `(*b).unwrap_or_default()`
 
-error: aborting due to 6 previous errors
+error: this pattern reimplements `Option::unwrap_or`
+  --> tests/ui/manual_unwrap_or_default.rs:84:27
+   |
+LL |       let match_middle_cm = match 1u32.checked_div(0) {
+   |  ___________________________^
+LL | |         Some(v) => v,
+LL | |         // Important
+LL | |         None => 0
+LL | |     };
+   | |_____^ help: replace with: `1u32.checked_div(0).unwrap_or(0)`
+   |
+   = note: `-D clippy::manual-unwrap-or` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`
+
+error: this pattern reimplements `Option::unwrap_or`
+  --> tests/ui/manual_unwrap_or_default.rs:89:31
+   |
+LL |       let match_middle_doc_cm = match 1u32.checked_div(0) {
+   |  _______________________________^
+LL | |         /// Important
+LL | |         Some(v) => v,
+LL | |         None => 0
+LL | |     };
+   | |_____^ help: replace with: `1u32.checked_div(0).unwrap_or(0)`
+
+error: aborting due to 8 previous errors