Auto merge of #12579 - J-ZhengLi:issue12569, r=Alexendoo

fix [`manual_unwrap_or_default`] suggestion ignoring side-effects

fixes: #12569
closes: #12580

change applicability to `MaybeIncorrect` base on suggestion in [this zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.60manual_unwrap_or_default.60.20suggestion.20removes.20comments)

---

changelog: fix [`manual_unwrap_or_default`] suggestion ignoring side-effects, and adjust its applicability.

Author: bors

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, span_contains_comment};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -119,22 +119,27 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
         // 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(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(body_none)
         && is_default_equivalent(cx, body_none)
         && let Some(receiver) = Sugg::hir_opt(cx, match_expr).map(Sugg::maybe_par)
     {
+        let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) {
+            Applicability::MaybeIncorrect
+        } else {
+            Applicability::MachineApplicable
+        };
         span_lint_and_sugg(
             cx,
             MANUAL_UNWRAP_OR_DEFAULT,
             expr.span,
             "match can be simplified with `.unwrap_or_default()`",
             "replace it with",
             format!("{receiver}.unwrap_or_default()"),
-            Applicability::MachineApplicable,
+            applicability,
         );
     }
     true
@@ -150,22 +155,27 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
         && 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(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(else_expr)
         && is_default_equivalent(cx, body_else)
         && let Some(if_let_expr_snippet) = snippet_opt(cx, let_.init.span)
     {
+        let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) {
+            Applicability::MaybeIncorrect
+        } else {
+            Applicability::MachineApplicable
+        };
         span_lint_and_sugg(
             cx,
             MANUAL_UNWRAP_OR_DEFAULT,
             expr.span,
             "if let can be simplified with `.unwrap_or_default()`",
             "replace it with",
             format!("{if_let_expr_snippet}.unwrap_or_default()"),
-            Applicability::MachineApplicable,
+            applicability,
         );
     }
 }

tests/ui/manual_unwrap_or_default.fixed

@@ -33,3 +33,32 @@ 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,
+    };
+    let iflet_else_se = if let Some(v) = 1u32.checked_div(0) {
+        v
+    } else {
+        println!("important");
+        0
+    };
+    let iflet_then_se = if let Some(v) = 1u32.checked_div(0) {
+        println!("important");
+        v
+    } else {
+        0
+    };
+}

tests/ui/manual_unwrap_or_default.rs

@@ -57,3 +57,32 @@ 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,
+    };
+    let iflet_else_se = if let Some(v) = 1u32.checked_div(0) {
+        v
+    } else {
+        println!("important");
+        0
+    };
+    let iflet_then_se = if let Some(v) = 1u32.checked_div(0) {
+        println!("important");
+        v
+    } else {
+        0
+    };
+}