Skip to content

Commit 6338925

Browse files
J-ZhengLiJ-ZhengLi
committed
fix [manual_unwrap_or_default] suggestion ignoring side-effects
1 parent 124e68b commit 6338925

File tree

4 files changed

+137
-24
lines changed

4 files changed

+137
-24
lines changed

‎clippy_lints/src/manual_unwrap_or_default.rs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ use clippy_utils::sugg::Sugg;
22
use rustc_errors::Applicability;
33
use rustc_hir::def::Res;
44
use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatKind, QPath};
5-
use rustc_lint::{LateContext, LateLintPass};
5+
use rustc_lint::{LateContext, LateLintPass, LintContext};
66
use rustc_session::declare_lint_pass;
77
use rustc_span::sym;
88

99
use clippy_utils::diagnostics::span_lint_and_sugg;
1010
use clippy_utils::source::snippet_opt;
1111
use clippy_utils::ty::implements_trait;
12-
use clippy_utils::{in_constant, is_default_equivalent};
12+
use clippy_utils::{in_constant, is_default_equivalent, peel_blocks_with_stmt, span_contains_comment};
1313

1414
declare_clippy_lint! {
1515
/// ### What it does
@@ -112,18 +112,19 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
112112
// We don't want conditions on the arms to simplify things.
113113
if arm1.guard.is_none()
114114
&& arm2.guard.is_none()
115+
&& !span_contains_comment(cx.sess().source_map(), expr.span)
115116
// We check that the returned type implements the `Default` trait.
116117
&& let match_ty = cx.typeck_results().expr_ty(expr)
117118
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
118119
&& implements_trait(cx, match_ty, default_trait_id, &[])
119120
// We now get the bodies for both the `Some` and `None` arms.
120121
&& let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2)
121122
// We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`.
122-
&& let ExprKind::Path(QPath::Resolved(_, path)) = body_some.peel_blocks().kind
123+
&& let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks_with_stmt(body_some).kind
123124
&& let Res::Local(local_id) = path.res
124125
&& local_id == binding_id
125126
// We now check the `None` arm is calling a method equivalent to `Default::default`.
126-
&& let body_none = body_none.peel_blocks()
127+
&& let body_none = peel_blocks_with_stmt(body_none)
127128
&& is_default_equivalent(cx, body_none)
128129
&& let Some(receiver) = Sugg::hir_opt(cx, match_expr).map(Sugg::maybe_par)
129130
{
@@ -144,17 +145,18 @@ fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
144145
if let ExprKind::If(cond, if_block, Some(else_expr)) = expr.kind
145146
&& let ExprKind::Let(let_) = cond.kind
146147
&& let ExprKind::Block(_, _) = else_expr.kind
148+
&& !span_contains_comment(cx.sess().source_map(), expr.span)
147149
// We check that the returned type implements the `Default` trait.
148150
&& let match_ty = cx.typeck_results().expr_ty(expr)
149151
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
150152
&& implements_trait(cx, match_ty, default_trait_id, &[])
151153
&& let Some(binding_id) = get_some(cx, let_.pat)
152154
// We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`.
153-
&& let ExprKind::Path(QPath::Resolved(_, path)) = if_block.peel_blocks().kind
155+
&& let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks_with_stmt(if_block).kind
154156
&& let Res::Local(local_id) = path.res
155157
&& local_id == binding_id
156158
// We now check the `None` arm is calling a method equivalent to `Default::default`.
157-
&& let body_else = else_expr.peel_blocks()
159+
&& let body_else = peel_blocks_with_stmt(else_expr)
158160
&& is_default_equivalent(cx, body_else)
159161
&& let Some(if_let_expr_snippet) = snippet_opt(cx, let_.init.span)
160162
{

‎tests/ui/manual_unwrap_or_default.fixed‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,25 +3,30 @@
33

44
fn main() {
55
let x: Option<Vec<String>> = None;
6+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
67
x.unwrap_or_default();
78

89
let x: Option<Vec<String>> = None;
10+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
911
x.unwrap_or_default();
1012

1113
let x: Option<String> = None;
14+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
1215
x.unwrap_or_default();
1316

1417
let x: Option<Vec<String>> = None;
18+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
1519
x.unwrap_or_default();
1620

1721
let x: Option<Vec<String>> = None;
22+
//~v ERROR: if let can be simplified with `.unwrap_or_default()`
1823
x.unwrap_or_default();
1924
}
2025

2126
// Issue #12531
2227
unsafe fn no_deref_ptr(a: Option<i32>, b: *const Option<i32>) -> i32 {
28+
// `*b` being correct depends on `a == Some(_)`
2329
match a {
24-
// `*b` being correct depends on `a == Some(_)`
2530
Some(_) => (*b).unwrap_or_default(),
2631
_ => 0,
2732
}
@@ -33,3 +38,42 @@ const fn issue_12568(opt: Option<bool>) -> bool {
3338
None => false,
3439
}
3540
}
41+
42+
fn issue_12569() {
43+
let match_none_se = match 1u32.checked_div(0) {
44+
Some(v) => v,
45+
None => {
46+
println!("important");
47+
0
48+
}
49+
};
50+
let match_some_se = match 1u32.checked_div(0) {
51+
Some(v) => {
52+
println!("important");
53+
v
54+
},
55+
None => 0
56+
};
57+
#[allow(clippy::manual_unwrap_or)]
58+
let match_none_cm = match 1u32.checked_div(0) {
59+
Some(v) => v,
60+
None => {
61+
// Important
62+
0
63+
}
64+
};
65+
let match_middle_cm = 1u32.checked_div(0).unwrap_or(0);
66+
let match_middle_doc_cm = 1u32.checked_div(0).unwrap_or(0);
67+
let iflet_else_se = if let Some(v) = 1u32.checked_div(0) {
68+
v
69+
} else {
70+
println!("important");
71+
0
72+
};
73+
let iflet_then_cm = if let Some(v) = 1u32.checked_div(0) {
74+
// Important
75+
v
76+
} else {
77+
0
78+
};
79+
}

‎tests/ui/manual_unwrap_or_default.rs‎

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,36 +3,36 @@
33

44
fn main() {
55
let x: Option<Vec<String>> = None;
6+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
67
match x {
7-
//~^ ERROR: match can be simplified with `.unwrap_or_default()`
88
Some(v) => v,
99
None => Vec::default(),
1010
};
1111

1212
let x: Option<Vec<String>> = None;
13+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
1314
match x {
14-
//~^ ERROR: match can be simplified with `.unwrap_or_default()`
1515
Some(v) => v,
1616
_ => Vec::default(),
1717
};
1818

1919
let x: Option<String> = None;
20+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
2021
match x {
21-
//~^ ERROR: match can be simplified with `.unwrap_or_default()`
2222
Some(v) => v,
2323
None => String::new(),
2424
};
2525

2626
let x: Option<Vec<String>> = None;
27+
//~v ERROR: match can be simplified with `.unwrap_or_default()`
2728
match x {
28-
//~^ ERROR: match can be simplified with `.unwrap_or_default()`
2929
None => Vec::default(),
3030
Some(v) => v,
3131
};
3232

3333
let x: Option<Vec<String>> = None;
34+
//~v ERROR: if let can be simplified with `.unwrap_or_default()`
3435
if let Some(v) = x {
35-
//~^ ERROR: if let can be simplified with `.unwrap_or_default()`
3636
v
3737
} else {
3838
Vec::default()
@@ -41,8 +41,8 @@ fn main() {
4141

4242
// Issue #12531
4343
unsafe fn no_deref_ptr(a: Option<i32>, b: *const Option<i32>) -> i32 {
44+
// `*b` being correct depends on `a == Some(_)`
4445
match a {
45-
// `*b` being correct depends on `a == Some(_)`
4646
Some(_) => match *b {
4747
Some(v) => v,
4848
_ => 0,
@@ -57,3 +57,50 @@ const fn issue_12568(opt: Option<bool>) -> bool {
5757
None => false,
5858
}
5959
}
60+
61+
fn issue_12569() {
62+
let match_none_se = match 1u32.checked_div(0) {
63+
Some(v) => v,
64+
None => {
65+
println!("important");
66+
0
67+
}
68+
};
69+
let match_some_se = match 1u32.checked_div(0) {
70+
Some(v) => {
71+
println!("important");
72+
v
73+
},
74+
None => 0
75+
};
76+
#[allow(clippy::manual_unwrap_or)]
77+
let match_none_cm = match 1u32.checked_div(0) {
78+
Some(v) => v,
79+
None => {
80+
// Important
81+
0
82+
}
83+
};
84+
let match_middle_cm = match 1u32.checked_div(0) {
85+
Some(v) => v,
86+
// Important
87+
None => 0
88+
};
89+
let match_middle_doc_cm = match 1u32.checked_div(0) {
90+
/// Important
91+
Some(v) => v,
92+
None => 0
93+
};
94+
let iflet_else_se = if let Some(v) = 1u32.checked_div(0) {
95+
v
96+
} else {
97+
println!("important");
98+
0
99+
};
100+
let iflet_then_cm = if let Some(v) = 1u32.checked_div(0) {
101+
// Important
102+
v
103+
} else {
104+
0
105+
};
106+
}

‎tests/ui/manual_unwrap_or_default.stderr‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
error: match can be simplified with `.unwrap_or_default()`
2-
--> tests/ui/manual_unwrap_or_default.rs:6:5
2+
--> tests/ui/manual_unwrap_or_default.rs:7:5
33
|
44
LL | / match x {
5-
LL | |
65
LL | | Some(v) => v,
76
LL | | None => Vec::default(),
87
LL | | };
@@ -12,40 +11,36 @@ LL | | };
1211
= help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or_default)]`
1312

1413
error: match can be simplified with `.unwrap_or_default()`
15-
--> tests/ui/manual_unwrap_or_default.rs:13:5
14+
--> tests/ui/manual_unwrap_or_default.rs:14:5
1615
|
1716
LL | / match x {
18-
LL | |
1917
LL | | Some(v) => v,
2018
LL | | _ => Vec::default(),
2119
LL | | };
2220
| |_____^ help: replace it with: `x.unwrap_or_default()`
2321

2422
error: match can be simplified with `.unwrap_or_default()`
25-
--> tests/ui/manual_unwrap_or_default.rs:20:5
23+
--> tests/ui/manual_unwrap_or_default.rs:21:5
2624
|
2725
LL | / match x {
28-
LL | |
2926
LL | | Some(v) => v,
3027
LL | | None => String::new(),
3128
LL | | };
3229
| |_____^ help: replace it with: `x.unwrap_or_default()`
3330

3431
error: match can be simplified with `.unwrap_or_default()`
35-
--> tests/ui/manual_unwrap_or_default.rs:27:5
32+
--> tests/ui/manual_unwrap_or_default.rs:28:5
3633
|
3734
LL | / match x {
38-
LL | |
3935
LL | | None => Vec::default(),
4036
LL | | Some(v) => v,
4137
LL | | };
4238
| |_____^ help: replace it with: `x.unwrap_or_default()`
4339

4440
error: if let can be simplified with `.unwrap_or_default()`
45-
--> tests/ui/manual_unwrap_or_default.rs:34:5
41+
--> tests/ui/manual_unwrap_or_default.rs:35:5
4642
|
4743
LL | / if let Some(v) = x {
48-
LL | |
4944
LL | | v
5045
LL | | } else {
5146
LL | | Vec::default()
@@ -62,5 +57,30 @@ LL | | _ => 0,
6257
LL | | },
6358
| |_________^ help: replace it with: `(*b).unwrap_or_default()`
6459

65-
error: aborting due to 6 previous errors
60+
error: this pattern reimplements `Option::unwrap_or`
61+
--> tests/ui/manual_unwrap_or_default.rs:84:27
62+
|
63+
LL | let match_middle_cm = match 1u32.checked_div(0) {
64+
| ___________________________^
65+
LL | | Some(v) => v,
66+
LL | | // Important
67+
LL | | None => 0
68+
LL | | };
69+
| |_____^ help: replace with: `1u32.checked_div(0).unwrap_or(0)`
70+
|
71+
= note: `-D clippy::manual-unwrap-or` implied by `-D warnings`
72+
= help: to override `-D warnings` add `#[allow(clippy::manual_unwrap_or)]`
73+
74+
error: this pattern reimplements `Option::unwrap_or`
75+
--> tests/ui/manual_unwrap_or_default.rs:89:31
76+
|
77+
LL | let match_middle_doc_cm = match 1u32.checked_div(0) {
78+
| _______________________________^
79+
LL | | /// Important
80+
LL | | Some(v) => v,
81+
LL | | None => 0
82+
LL | | };
83+
| |_____^ help: replace with: `1u32.checked_div(0).unwrap_or(0)`
84+
85+
error: aborting due to 8 previous errors
6686

0 commit comments

Comments
 (0)