Skip to content

Commit 9e54ff2

Browse files
borsbors
committed
Auto merge of #12906 - lochetti:manual_unwrap_or_if_let, r=y21
Lint `manual_unwrap_or` for it let cases This PR modifies `manual_unwrap_or` to lint for `if let` cases as well. This effort is part of the fixes desired by rust-lang/rust-clippy#12618 changelog:[`manual_unwrap_or`]: Lint for `if let` cases.
2 parents 51c5eee + 70ca9a1 commit 9e54ff2

13 files changed

+276
-77
lines changed

‎clippy_lints/src/matches/manual_unwrap_or.rs

+79-32
Original file line numberDiff line numberDiff line change
@@ -3,46 +3,78 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
33
use clippy_utils::source::{indent_of, reindent_multiline, snippet_opt};
44
use clippy_utils::ty::is_type_diagnostic_item;
55
use clippy_utils::usage::contains_return_break_continue_macro;
6-
use clippy_utils::{is_res_lang_ctor, path_to_local_id, sugg};
6+
use clippy_utils::{is_res_lang_ctor, path_to_local_id, peel_blocks, sugg};
77
use rustc_errors::Applicability;
88
use rustc_hir::def::{DefKind, Res};
99
use rustc_hir::LangItem::{OptionNone, ResultErr};
10-
use rustc_hir::{Arm, Expr, PatKind};
10+
use rustc_hir::{Arm, Expr, Pat, PatKind};
1111
use rustc_lint::LateContext;
12+
use rustc_middle::ty::Ty;
1213
use rustc_span::sym;
1314

1415
use super::MANUAL_UNWRAP_OR;
1516

16-
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, scrutinee: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) {
17+
pub(super) fn check_match<'tcx>(
18+
cx: &LateContext<'tcx>,
19+
expr: &'tcx Expr<'tcx>,
20+
scrutinee: &'tcx Expr<'_>,
21+
arms: &'tcx [Arm<'_>],
22+
) {
1723
let ty = cx.typeck_results().expr_ty(scrutinee);
18-
if let Some(ty_name) = if is_type_diagnostic_item(cx, ty, sym::Option) {
24+
if let Some((or_arm, unwrap_arm)) = applicable_or_arm(cx, arms) {
25+
check_and_lint(cx, expr, unwrap_arm.pat, scrutinee, unwrap_arm.body, or_arm.body, ty);
26+
}
27+
}
28+
29+
pub(super) fn check_if_let<'tcx>(
30+
cx: &LateContext<'tcx>,
31+
expr: &'tcx Expr<'_>,
32+
let_pat: &'tcx Pat<'_>,
33+
let_expr: &'tcx Expr<'_>,
34+
then_expr: &'tcx Expr<'_>,
35+
else_expr: &'tcx Expr<'_>,
36+
) {
37+
let ty = cx.typeck_results().expr_ty(let_expr);
38+
check_and_lint(cx, expr, let_pat, let_expr, then_expr, peel_blocks(else_expr), ty);
39+
}
40+
41+
fn check_and_lint<'tcx>(
42+
cx: &LateContext<'tcx>,
43+
expr: &'tcx Expr<'_>,
44+
let_pat: &'tcx Pat<'_>,
45+
let_expr: &'tcx Expr<'_>,
46+
then_expr: &'tcx Expr<'_>,
47+
else_expr: &'tcx Expr<'_>,
48+
ty: Ty<'tcx>,
49+
) {
50+
if let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = let_pat.kind
51+
&& let Res::Def(DefKind::Ctor(..), ctor_id) = cx.qpath_res(qpath, let_pat.hir_id)
52+
&& let Some(variant_id) = cx.tcx.opt_parent(ctor_id)
53+
&& (cx.tcx.lang_items().option_some_variant() == Some(variant_id)
54+
|| cx.tcx.lang_items().result_ok_variant() == Some(variant_id))
55+
&& let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind
56+
&& path_to_local_id(peel_blocks(then_expr), binding_hir_id)
57+
&& cx.typeck_results().expr_adjustments(then_expr).is_empty()
58+
&& let Some(ty_name) = find_type_name(cx, ty)
59+
&& let Some(or_body_snippet) = snippet_opt(cx, else_expr.span)
60+
&& let Some(indent) = indent_of(cx, expr.span)
61+
&& constant_simple(cx, cx.typeck_results(), else_expr).is_some()
62+
{
63+
lint(cx, expr, let_expr, ty_name, or_body_snippet, indent);
64+
}
65+
}
66+
67+
fn find_type_name<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'static str> {
68+
if is_type_diagnostic_item(cx, ty, sym::Option) {
1969
Some("Option")
2070
} else if is_type_diagnostic_item(cx, ty, sym::Result) {
2171
Some("Result")
2272
} else {
2373
None
24-
} && let Some(or_arm) = applicable_or_arm(cx, arms)
25-
&& let Some(or_body_snippet) = snippet_opt(cx, or_arm.body.span)
26-
&& let Some(indent) = indent_of(cx, expr.span)
27-
&& constant_simple(cx, cx.typeck_results(), or_arm.body).is_some()
28-
{
29-
let reindented_or_body = reindent_multiline(or_body_snippet.into(), true, Some(indent));
30-
31-
let mut app = Applicability::MachineApplicable;
32-
let suggestion = sugg::Sugg::hir_with_context(cx, scrutinee, expr.span.ctxt(), "..", &mut app).maybe_par();
33-
span_lint_and_sugg(
34-
cx,
35-
MANUAL_UNWRAP_OR,
36-
expr.span,
37-
format!("this pattern reimplements `{ty_name}::unwrap_or`"),
38-
"replace with",
39-
format!("{suggestion}.unwrap_or({reindented_or_body})",),
40-
app,
41-
);
4274
}
4375
}
4476

45-
fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> {
77+
fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<(&'a Arm<'a>, &'a Arm<'a>)> {
4678
if arms.len() == 2
4779
&& arms.iter().all(|arm| arm.guard.is_none())
4880
&& let Some((idx, or_arm)) = arms.iter().enumerate().find(|(_, arm)| match arm.pat.kind {
@@ -54,18 +86,33 @@ fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<&'
5486
_ => false,
5587
})
5688
&& let unwrap_arm = &arms[1 - idx]
57-
&& let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = unwrap_arm.pat.kind
58-
&& let Res::Def(DefKind::Ctor(..), ctor_id) = cx.qpath_res(qpath, unwrap_arm.pat.hir_id)
59-
&& let Some(variant_id) = cx.tcx.opt_parent(ctor_id)
60-
&& (cx.tcx.lang_items().option_some_variant() == Some(variant_id)
61-
|| cx.tcx.lang_items().result_ok_variant() == Some(variant_id))
62-
&& let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind
63-
&& path_to_local_id(unwrap_arm.body, binding_hir_id)
64-
&& cx.typeck_results().expr_adjustments(unwrap_arm.body).is_empty()
6589
&& !contains_return_break_continue_macro(or_arm.body)
6690
{
67-
Some(or_arm)
91+
Some((or_arm, unwrap_arm))
6892
} else {
6993
None
7094
}
7195
}
96+
97+
fn lint<'tcx>(
98+
cx: &LateContext<'tcx>,
99+
expr: &Expr<'tcx>,
100+
scrutinee: &'tcx Expr<'_>,
101+
ty_name: &str,
102+
or_body_snippet: String,
103+
indent: usize,
104+
) {
105+
let reindented_or_body = reindent_multiline(or_body_snippet.into(), true, Some(indent));
106+
107+
let mut app = Applicability::MachineApplicable;
108+
let suggestion = sugg::Sugg::hir_with_context(cx, scrutinee, expr.span.ctxt(), "..", &mut app).maybe_par();
109+
span_lint_and_sugg(
110+
cx,
111+
MANUAL_UNWRAP_OR,
112+
expr.span,
113+
format!("this pattern reimplements `{ty_name}::unwrap_or`"),
114+
"replace with",
115+
format!("{suggestion}.unwrap_or({reindented_or_body})",),
116+
app,
117+
);
118+
}

‎clippy_lints/src/matches/mod.rs

+9-1
Original file line numberDiff line numberDiff line change
@@ -1069,7 +1069,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10691069
redundant_guards::check(cx, arms, &self.msrv);
10701070

10711071
if !in_constant(cx, expr.hir_id) {
1072-
manual_unwrap_or::check(cx, expr, ex, arms);
1072+
manual_unwrap_or::check_match(cx, expr, ex, arms);
10731073
manual_map::check_match(cx, expr, ex, arms);
10741074
manual_filter::check_match(cx, ex, arms, expr);
10751075
}
@@ -1097,6 +1097,14 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10971097
);
10981098
}
10991099
if !in_constant(cx, expr.hir_id) {
1100+
manual_unwrap_or::check_if_let(
1101+
cx,
1102+
expr,
1103+
if_let.let_pat,
1104+
if_let.let_expr,
1105+
if_let.if_then,
1106+
else_expr,
1107+
);
11001108
manual_map::check_if_let(cx, expr, if_let.let_pat, if_let.let_expr, if_let.if_then, else_expr);
11011109
manual_filter::check_if_let(
11021110
cx,

‎tests/ui/manual_unwrap_or.fixed

+52
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,32 @@ fn option_unwrap_or() {
6868
Some(s) => s,
6969
None => &format!("{} {}!", "hello", "world"),
7070
};
71+
72+
Some(1).unwrap_or(42);
73+
74+
//don't lint
75+
if let Some(x) = Some(1) {
76+
x + 1
77+
} else {
78+
42
79+
};
80+
if let Some(x) = Some(1) {
81+
x
82+
} else {
83+
return;
84+
};
85+
for j in 0..4 {
86+
if let Some(x) = Some(j) {
87+
x
88+
} else {
89+
continue;
90+
};
91+
if let Some(x) = Some(j) {
92+
x
93+
} else {
94+
break;
95+
};
96+
}
7197
}
7298

7399
fn result_unwrap_or() {
@@ -138,6 +164,32 @@ fn result_unwrap_or() {
138164
Ok(s) => s,
139165
Err(s) => "Bob",
140166
};
167+
168+
Ok::<i32, i32>(1).unwrap_or(42);
169+
170+
//don't lint
171+
if let Ok(x) = Ok::<i32, i32>(1) {
172+
x + 1
173+
} else {
174+
42
175+
};
176+
if let Ok(x) = Ok::<i32, i32>(1) {
177+
x
178+
} else {
179+
return;
180+
};
181+
for j in 0..4 {
182+
if let Ok(x) = Ok::<i32, i32>(j) {
183+
x
184+
} else {
185+
continue;
186+
};
187+
if let Ok(x) = Ok::<i32, i32>(j) {
188+
x
189+
} else {
190+
break;
191+
};
192+
}
141193
}
142194

143195
// don't lint in const fn

‎tests/ui/manual_unwrap_or.rs

+60
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,36 @@ fn option_unwrap_or() {
8383
Some(s) => s,
8484
None => &format!("{} {}!", "hello", "world"),
8585
};
86+
87+
if let Some(x) = Some(1) {
88+
x
89+
} else {
90+
42
91+
};
92+
93+
//don't lint
94+
if let Some(x) = Some(1) {
95+
x + 1
96+
} else {
97+
42
98+
};
99+
if let Some(x) = Some(1) {
100+
x
101+
} else {
102+
return;
103+
};
104+
for j in 0..4 {
105+
if let Some(x) = Some(j) {
106+
x
107+
} else {
108+
continue;
109+
};
110+
if let Some(x) = Some(j) {
111+
x
112+
} else {
113+
break;
114+
};
115+
}
86116
}
87117

88118
fn result_unwrap_or() {
@@ -177,6 +207,36 @@ fn result_unwrap_or() {
177207
Ok(s) => s,
178208
Err(s) => "Bob",
179209
};
210+
211+
if let Ok(x) = Ok::<i32, i32>(1) {
212+
x
213+
} else {
214+
42
215+
};
216+
217+
//don't lint
218+
if let Ok(x) = Ok::<i32, i32>(1) {
219+
x + 1
220+
} else {
221+
42
222+
};
223+
if let Ok(x) = Ok::<i32, i32>(1) {
224+
x
225+
} else {
226+
return;
227+
};
228+
for j in 0..4 {
229+
if let Ok(x) = Ok::<i32, i32>(j) {
230+
x
231+
} else {
232+
continue;
233+
};
234+
if let Ok(x) = Ok::<i32, i32>(j) {
235+
x
236+
} else {
237+
break;
238+
};
239+
}
180240
}
181241

182242
// don't lint in const fn

0 commit comments

Comments
 (0)