Skip to content

Commit 71b57c0

Browse files
JarredAllenJarredAllen
committed
Refactoring
1 parent 1543e76 commit 71b57c0

File tree

3 files changed

+123
-108
lines changed

3 files changed

+123
-108
lines changed

‎clippy_lints/src/option_if_let_else.rs

Lines changed: 106 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use if_chain::if_chain;
55

66
use rustc_errors::Applicability;
77
use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
8-
use rustc_hir::*;
8+
use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp};
99
use rustc_lint::{LateContext, LateLintPass};
1010
use rustc_middle::hir::map::Map;
1111
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -69,17 +69,12 @@ declare_clippy_lint! {
6969

7070
declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]);
7171

72-
/// Returns true iff the given expression is the result of calling Result::ok
72+
/// Returns true iff the given expression is the result of calling `Result::ok`
7373
fn is_result_ok(cx: &LateContext<'_, '_>, expr: &'_ Expr<'_>) -> bool {
74-
if_chain! {
75-
if let ExprKind::MethodCall(ref path, _, &[ref receiver]) = &expr.kind;
76-
if path.ident.name.to_ident_string() == "ok";
77-
if match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT);
78-
then {
79-
true
80-
} else {
81-
false
82-
}
74+
if let ExprKind::MethodCall(ref path, _, &[ref receiver]) = &expr.kind {
75+
path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT)
76+
} else {
77+
false
8378
}
8479
}
8580

@@ -136,82 +131,129 @@ fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool {
136131
recursive_visitor.seen_return_break_continue
137132
}
138133

134+
/// Extracts the body of a given arm. If the arm contains only an expression,
135+
/// then it returns the expression. Otherwise, it returns the entire block
136+
fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> {
137+
if let ExprKind::Block(
138+
Block {
139+
stmts: statements,
140+
expr: Some(expr),
141+
..
142+
},
143+
_,
144+
) = &arm.body.kind
145+
{
146+
if let [] = statements {
147+
Some(&expr)
148+
} else {
149+
Some(&arm.body)
150+
}
151+
} else {
152+
None
153+
}
154+
}
155+
156+
/// If this is the else body of an if/else expression, then we need to wrap
157+
/// it in curcly braces. Otherwise, we don't.
158+
fn should_wrap_in_braces(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
159+
utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| {
160+
if let Some(Expr {
161+
kind:
162+
ExprKind::Match(
163+
_,
164+
arms,
165+
MatchSource::IfDesugar {
166+
contains_else_clause: true,
167+
}
168+
| MatchSource::IfLetDesugar {
169+
contains_else_clause: true,
170+
},
171+
),
172+
..
173+
}) = parent.expr
174+
{
175+
expr.hir_id == arms[1].body.hir_id
176+
} else {
177+
false
178+
}
179+
})
180+
}
181+
182+
fn format_option_in_sugg(
183+
cx: &LateContext<'_, '_>,
184+
cond_expr: &Expr<'_>,
185+
parens_around_option: bool,
186+
as_ref: bool,
187+
as_mut: bool,
188+
) -> String {
189+
format!(
190+
"{}{}{}{}",
191+
if parens_around_option { "(" } else { "" },
192+
Sugg::hir(cx, cond_expr, ".."),
193+
if parens_around_option { ")" } else { "" },
194+
if as_mut {
195+
".as_mut()"
196+
} else if as_ref {
197+
".as_ref()"
198+
} else {
199+
""
200+
}
201+
)
202+
}
203+
139204
/// If this expression is the option if let/else construct we're detecting, then
140-
/// this function returns an OptionIfLetElseOccurence struct with details if
205+
/// this function returns an `OptionIfLetElseOccurence` struct with details if
141206
/// this construct is found, or None if this construct is not found.
142207
fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option<OptionIfLetElseOccurence> {
143208
if_chain! {
144209
if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly
145-
if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
210+
if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
146211
if arms.len() == 2;
147-
// if type_is_option(cx, &cx.tables.expr_ty(let_body).kind);
148-
if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already
212+
if !is_result_ok(cx, cond_expr); // Don't lint on Result::ok because a different lint does it already
149213
if let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind;
150214
if utils::match_qpath(struct_qpath, &paths::OPTION_SOME);
151215
if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind;
152216
if !contains_return_break_continue(arms[0].body);
153217
if !contains_return_break_continue(arms[1].body);
154218
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-
};
161-
let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
162-
= &arms[0].body.kind {
163-
if let &[] = &statements {
164-
expr
165-
} else {
166-
&arms[0].body
167-
}
168-
} else {
169-
return None;
170-
};
171-
let (none_body, method_sugg) = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _)
172-
= &arms[1].body.kind {
173-
if let &[] = &statements {
174-
(expr, "map_or")
175-
} else {
176-
(&arms[1].body, "map_or_else")
177-
}
178-
} else {
179-
return None;
219+
let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" };
220+
let some_body = extract_body_from_arm(&arms[0])?;
221+
let none_body = extract_body_from_arm(&arms[1])?;
222+
let method_sugg = match &none_body.kind {
223+
ExprKind::Block(..) => "map_or_else",
224+
_ => "map_or",
180225
};
181226
let capture_name = id.name.to_ident_string();
182-
let wrap_braces = utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| {
183-
if_chain! {
184-
if let Some(Expr { kind: ExprKind::Match(
185-
_,
186-
arms,
187-
MatchSource::IfDesugar{contains_else_clause: true}
188-
| MatchSource::IfLetDesugar{contains_else_clause: true}),
189-
.. } ) = parent.expr;
190-
if expr.hir_id == arms[1].body.hir_id;
191-
then {
192-
true
193-
} else {
194-
false
195-
}
196-
}
197-
});
198-
let (parens_around_option, as_ref, as_mut, let_body) = match &let_body.kind {
227+
let wrap_braces = should_wrap_in_braces(cx, expr);
228+
let (as_ref, as_mut) = match &cond_expr.kind {
229+
ExprKind::AddrOf(_, Mutability::Not, _) => (true, false),
230+
ExprKind::AddrOf(_, Mutability::Mut, _) => (false, true),
231+
_ => (bind_annotation == &BindingAnnotation::Ref, bind_annotation == &BindingAnnotation::RefMut),
232+
};
233+
let parens_around_option = match &cond_expr.kind {
234+
// Put parens around the option expression if not doing so might
235+
// mess up the order of operations.
199236
ExprKind::Call(..)
200237
| ExprKind::MethodCall(..)
201238
| ExprKind::Loop(..)
202239
| ExprKind::Match(..)
203240
| ExprKind::Block(..)
204241
| ExprKind::Field(..)
205242
| ExprKind::Path(_)
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),
243+
| ExprKind::Unary(UnOp::UnDeref, _)
244+
| ExprKind::AddrOf(..)
245+
=> false,
246+
_ => true,
247+
};
248+
let cond_expr = match &cond_expr.kind {
249+
// Pointer dereferencing happens automatically, so we can omit it in the suggestion
250+
ExprKind::Unary(UnOp::UnDeref, expr)|ExprKind::AddrOf(_, _, expr) => expr,
251+
_ => cond_expr,
210252
};
211253
Some(OptionIfLetElseOccurence {
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 { "" }),
213-
method_sugg: format!("{}", method_sugg),
214-
some_expr: format!("|{}{}{}| {}", if false { "ref " } else { "" }, if capture_mut { "mut " } else { "" }, capture_name, Sugg::hir(cx, some_body, "..")),
254+
option: format_option_in_sugg(cx, cond_expr, parens_around_option, as_ref, as_mut),
255+
method_sugg: method_sugg.to_string(),
256+
some_expr: format!("|{}{}| {}", capture_mut, capture_name, Sugg::hir(cx, some_body, "..")),
215257
none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")),
216258
wrap_braces,
217259
})

‎tests/ui/option_if_let_else.rs

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,27 +20,15 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
2020
}
2121

2222
fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
23-
let _ = if let Some(s) = *string {
24-
s.len()
25-
} else {
26-
0
27-
};
28-
let _ = if let Some(s) = &num {
29-
s
30-
} else {
31-
&0
32-
};
23+
let _ = if let Some(s) = *string { s.len() } else { 0 };
24+
let _ = if let Some(s) = &num { s } else { &0 };
3325
let _ = if let Some(s) = &mut num {
3426
*s += 1;
3527
s
3628
} else {
3729
&mut 0
3830
};
39-
let _ = if let Some(ref s) = num {
40-
s
41-
} else {
42-
&0
43-
};
31+
let _ = if let Some(ref s) = num { s } else { &0 };
4432
let _ = if let Some(mut s) = num {
4533
s += 1;
4634
s

‎tests/ui/option_if_let_else.stderr

Lines changed: 14 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -24,27 +24,17 @@ LL | | }
2424
error: use Option::map_or instead of an if let/else
2525
--> $DIR/option_if_let_else.rs:23:13
2626
|
27-
LL | let _ = if let Some(s) = *string {
28-
| _____________^
29-
LL | | s.len()
30-
LL | | } else {
31-
LL | | 0
32-
LL | | };
33-
| |_____^ help: try: `string.map_or(0, |s| s.len())`
27+
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
28+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
3429

3530
error: use Option::map_or instead of an if let/else
36-
--> $DIR/option_if_let_else.rs:28:13
31+
--> $DIR/option_if_let_else.rs:24:13
3732
|
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)`
33+
LL | let _ = if let Some(s) = &num { s } else { &0 };
34+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
4535

4636
error: use Option::map_or instead of an if let/else
47-
--> $DIR/option_if_let_else.rs:33:13
37+
--> $DIR/option_if_let_else.rs:25:13
4838
|
4939
LL | let _ = if let Some(s) = &mut num {
5040
| _____________^
@@ -64,18 +54,13 @@ LL | });
6454
|
6555

6656
error: use Option::map_or instead of an if let/else
67-
--> $DIR/option_if_let_else.rs:39:13
57+
--> $DIR/option_if_let_else.rs:31:13
6858
|
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)`
59+
LL | let _ = if let Some(ref s) = num { s } else { &0 };
60+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
7661

7762
error: use Option::map_or instead of an if let/else
78-
--> $DIR/option_if_let_else.rs:44:13
63+
--> $DIR/option_if_let_else.rs:32:13
7964
|
8065
LL | let _ = if let Some(mut s) = num {
8166
| _____________^
@@ -95,7 +80,7 @@ LL | });
9580
|
9681

9782
error: use Option::map_or instead of an if let/else
98-
--> $DIR/option_if_let_else.rs:50:13
83+
--> $DIR/option_if_let_else.rs:38:13
9984
|
10085
LL | let _ = if let Some(ref mut s) = num {
10186
| _____________^
@@ -115,7 +100,7 @@ LL | });
115100
|
116101

117102
error: use Option::map_or instead of an if let/else
118-
--> $DIR/option_if_let_else.rs:59:5
103+
--> $DIR/option_if_let_else.rs:47:5
119104
|
120105
LL | / if let Some(x) = arg {
121106
LL | | let y = x * x;
@@ -134,7 +119,7 @@ LL | })
134119
|
135120

136121
error: use Option::map_or_else instead of an if let/else
137-
--> $DIR/option_if_let_else.rs:68:13
122+
--> $DIR/option_if_let_else.rs:56:13
138123
|
139124
LL | let _ = if let Some(x) = arg {
140125
| _____________^
@@ -157,7 +142,7 @@ LL | }, |x| x * x * x * x);
157142
|
158143

159144
error: use Option::map_or instead of an if let/else
160-
--> $DIR/option_if_let_else.rs:97:13
145+
--> $DIR/option_if_let_else.rs:85:13
161146
|
162147
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
163148
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`

0 commit comments

Comments
 (0)