Skip to content

Commit 88c8afd

Browse files
JarredAllenJarredAllen
committed
Handle ref, mut, &, and &mut on the option
1 parent b85796f commit 88c8afd

File tree

4 files changed

+158
-25
lines changed

4 files changed

+158
-25
lines changed

‎clippy_lints/src/option_if_let_else.rs

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -140,18 +140,24 @@ fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool {
140140
/// this function returns an OptionIfLetElseOccurence struct with details if
141141
/// this construct is found, or None if this construct is not found.
142142
fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option<OptionIfLetElseOccurence> {
143-
//(String, String, String, String)> {
144143
if_chain! {
145-
// if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly
144+
if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly
146145
if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
147146
if arms.len() == 2;
148-
if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION);
147+
// if type_is_option(cx, &cx.tables.expr_ty(let_body).kind);
149148
if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already
150-
if let PatKind::TupleStruct(_, &[inner_pat], _) = &arms[0].pat.kind;
151-
if let PatKind::Binding(_, _, id, _) = &inner_pat.kind;
149+
if let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind;
150+
if utils::match_qpath(struct_qpath, &paths::OPTION_SOME);
151+
if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind;
152152
if !contains_return_break_continue(arms[0].body);
153153
if !contains_return_break_continue(arms[1].body);
154154
then {
155+
let (capture_mut, capture_ref, capture_ref_mut) = match bind_annotation {
156+
BindingAnnotation::Unannotated => (false, false, false),
157+
BindingAnnotation::Mutable => (true, false, false),
158+
BindingAnnotation::Ref => (false, true, false),
159+
BindingAnnotation::RefMut => (false, false, true),
160+
};
155161
let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
156162
= &arms[0].body.kind {
157163
if let &[] = &statements {
@@ -189,21 +195,23 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -
189195
}
190196
}
191197
});
192-
let parens_around_option = match &let_body.kind {
198+
let (parens_around_option, as_ref, as_mut, let_body) = match &let_body.kind {
193199
ExprKind::Call(..)
194200
| ExprKind::MethodCall(..)
195201
| ExprKind::Loop(..)
196202
| ExprKind::Match(..)
197203
| ExprKind::Block(..)
198204
| ExprKind::Field(..)
199205
| ExprKind::Path(_)
200-
=> false,
201-
_ => true,
206+
=> (false, capture_ref, capture_ref_mut, let_body),
207+
ExprKind::Unary(UnOp::UnDeref, expr) => (false, capture_ref, capture_ref_mut, expr),
208+
ExprKind::AddrOf(_, mutability, expr) => (false, mutability == &Mutability::Not, mutability == &Mutability::Mut, expr),
209+
_ => (true, capture_ref, capture_ref_mut, let_body),
202210
};
203211
Some(OptionIfLetElseOccurence {
204-
option: format!("{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }),
212+
option: format!("{}{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }, if as_mut { ".as_mut()" } else if as_ref { ".as_ref()" } else { "" }),
205213
method_sugg: format!("{}", method_sugg),
206-
some_expr: format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
214+
some_expr: format!("|{}{}{}| {}", if false { "ref " } else { "" }, if capture_mut { "mut " } else { "" }, capture_name, Sugg::hir(cx, some_body, "..")),
207215
none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")),
208216
wrap_braces,
209217
})

‎tests/ui/option_if_let_else.fixed

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,22 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
1111
} else { string.map_or(Some((false, "")), |x| Some((true, x))) }
1212
}
1313

14-
fn unop_bad(string: &Option<&str>) -> usize {
15-
(*string).map_or(0, |s| s.len())
14+
fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
15+
let _ = string.map_or(0, |s| s.len());
16+
let _ = num.as_ref().map_or(&0, |s| s);
17+
let _ = num.as_mut().map_or(&mut 0, |s| {
18+
*s += 1;
19+
s
20+
});
21+
let _ = num.as_ref().map_or(&0, |s| s);
22+
let _ = num.map_or(0, |mut s| {
23+
s += 1;
24+
s
25+
});
26+
let _ = num.as_mut().map_or(&mut 0, |s| {
27+
*s += 1;
28+
s
29+
});
1630
}
1731

1832
fn longer_body(arg: Option<u32>) -> u32 {
@@ -53,7 +67,7 @@ fn main() {
5367
let _ = optional.map_or(5, |x| x + 2);
5468
let _ = bad1(None);
5569
let _ = else_if_option(None);
56-
let _ = unop_bad(&None);
70+
unop_bad(&None, None);
5771
let _ = longer_body(None);
5872
test_map_or_else(None);
5973
let _ = negative_tests(None);

‎tests/ui/option_if_let_else.rs

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,40 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
1919
}
2020
}
2121

22-
fn unop_bad(string: &Option<&str>) -> usize {
23-
if let Some(s) = *string {
22+
fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
23+
let _ = if let Some(s) = *string {
2424
s.len()
2525
} else {
2626
0
27-
}
27+
};
28+
let _ = if let Some(s) = &num {
29+
s
30+
} else {
31+
&0
32+
};
33+
let _ = if let Some(s) = &mut num {
34+
*s += 1;
35+
s
36+
} else {
37+
&mut 0
38+
};
39+
let _ = if let Some(ref s) = num {
40+
s
41+
} else {
42+
&0
43+
};
44+
let _ = if let Some(mut s) = num {
45+
s += 1;
46+
s
47+
} else {
48+
0
49+
};
50+
let _ = if let Some(ref mut s) = num {
51+
*s += 1;
52+
s
53+
} else {
54+
&mut 0
55+
};
2856
}
2957

3058
fn longer_body(arg: Option<u32>) -> u32 {
@@ -69,7 +97,7 @@ fn main() {
6997
let _ = if let Some(x) = optional { x + 2 } else { 5 };
7098
let _ = bad1(None);
7199
let _ = else_if_option(None);
72-
let _ = unop_bad(&None);
100+
unop_bad(&None, None);
73101
let _ = longer_body(None);
74102
test_map_or_else(None);
75103
let _ = negative_tests(None);

‎tests/ui/option_if_let_else.stderr

Lines changed: 91 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,100 @@ LL | | }
2222
| |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }`
2323

2424
error: use Option::map_or instead of an if let/else
25-
--> $DIR/option_if_let_else.rs:23:5
25+
--> $DIR/option_if_let_else.rs:23:13
2626
|
27-
LL | / if let Some(s) = *string {
27+
LL | let _ = if let Some(s) = *string {
28+
| _____________^
2829
LL | | s.len()
2930
LL | | } else {
3031
LL | | 0
31-
LL | | }
32-
| |_____^ help: try: `(*string).map_or(0, |s| s.len())`
32+
LL | | };
33+
| |_____^ help: try: `string.map_or(0, |s| s.len())`
34+
35+
error: use Option::map_or instead of an if let/else
36+
--> $DIR/option_if_let_else.rs:28:13
37+
|
38+
LL | let _ = if let Some(s) = &num {
39+
| _____________^
40+
LL | | s
41+
LL | | } else {
42+
LL | | &0
43+
LL | | };
44+
| |_____^ help: try: `num.as_ref().map_or(&0, |s| s)`
45+
46+
error: use Option::map_or instead of an if let/else
47+
--> $DIR/option_if_let_else.rs:33:13
48+
|
49+
LL | let _ = if let Some(s) = &mut num {
50+
| _____________^
51+
LL | | *s += 1;
52+
LL | | s
53+
LL | | } else {
54+
LL | | &mut 0
55+
LL | | };
56+
| |_____^
57+
|
58+
help: try
59+
|
60+
LL | let _ = num.as_mut().map_or(&mut 0, |s| {
61+
LL | *s += 1;
62+
LL | s
63+
LL | });
64+
|
65+
66+
error: use Option::map_or instead of an if let/else
67+
--> $DIR/option_if_let_else.rs:39:13
68+
|
69+
LL | let _ = if let Some(ref s) = num {
70+
| _____________^
71+
LL | | s
72+
LL | | } else {
73+
LL | | &0
74+
LL | | };
75+
| |_____^ help: try: `num.as_ref().map_or(&0, |s| s)`
76+
77+
error: use Option::map_or instead of an if let/else
78+
--> $DIR/option_if_let_else.rs:44:13
79+
|
80+
LL | let _ = if let Some(mut s) = num {
81+
| _____________^
82+
LL | | s += 1;
83+
LL | | s
84+
LL | | } else {
85+
LL | | 0
86+
LL | | };
87+
| |_____^
88+
|
89+
help: try
90+
|
91+
LL | let _ = num.map_or(0, |mut s| {
92+
LL | s += 1;
93+
LL | s
94+
LL | });
95+
|
96+
97+
error: use Option::map_or instead of an if let/else
98+
--> $DIR/option_if_let_else.rs:50:13
99+
|
100+
LL | let _ = if let Some(ref mut s) = num {
101+
| _____________^
102+
LL | | *s += 1;
103+
LL | | s
104+
LL | | } else {
105+
LL | | &mut 0
106+
LL | | };
107+
| |_____^
108+
|
109+
help: try
110+
|
111+
LL | let _ = num.as_mut().map_or(&mut 0, |s| {
112+
LL | *s += 1;
113+
LL | s
114+
LL | });
115+
|
33116

34117
error: use Option::map_or instead of an if let/else
35-
--> $DIR/option_if_let_else.rs:31:5
118+
--> $DIR/option_if_let_else.rs:59:5
36119
|
37120
LL | / if let Some(x) = arg {
38121
LL | | let y = x * x;
@@ -51,7 +134,7 @@ LL | })
51134
|
52135

53136
error: use Option::map_or_else instead of an if let/else
54-
--> $DIR/option_if_let_else.rs:40:13
137+
--> $DIR/option_if_let_else.rs:68:13
55138
|
56139
LL | let _ = if let Some(x) = arg {
57140
| _____________^
@@ -74,10 +157,10 @@ LL | }, |x| x * x * x * x);
74157
|
75158

76159
error: use Option::map_or instead of an if let/else
77-
--> $DIR/option_if_let_else.rs:69:13
160+
--> $DIR/option_if_let_else.rs:97:13
78161
|
79162
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
80163
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
81164

82-
error: aborting due to 6 previous errors
165+
error: aborting due to 11 previous errors
83166

0 commit comments

Comments
 (0)