Skip to content

Commit eaf25c2

Browse files
JarredAllenJarredAllen
committed
Bugfixes
1 parent ddbd7eb commit eaf25c2

File tree

4 files changed

+119
-49
lines changed

4 files changed

+119
-49
lines changed

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,6 +1014,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10141014
store.register_early_pass(move || box excessive_bools::ExcessiveBools::new(max_struct_bools, max_fn_params_bools));
10151015
store.register_early_pass(|| box option_env_unwrap::OptionEnvUnwrap);
10161016
store.register_late_pass(|| box wildcard_imports::WildcardImports);
1017+
store.register_late_pass(|| box option_if_let_else::OptionIfLetElse);
10171018

10181019
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
10191020
LintId::of(&arithmetic::FLOAT_ARITHMETIC),

‎clippy_lints/src/option_if_let_else.rs

Lines changed: 64 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::utils::sugg::Sugg;
2-
use crate::utils::{match_qpath, match_type, paths, span_lint, span_lint_and_sugg};
2+
use crate::utils::{match_qpath, match_type, paths, span_lint_and_sugg};
33
use if_chain::if_chain;
44

55
use rustc_errors::Applicability;
@@ -16,13 +16,38 @@ declare_clippy_lint! {
1616
/// Using the dedicated function in the Option class is clearer and
1717
/// more concise than an if let
1818
///
19-
/// **Known problems:** None.
19+
/// **Known problems:**
20+
/// Using `Option::map_or` requires it to compute the or value every
21+
/// function call, even if the value is unneeded. In some circumstances,
22+
/// it might be better to use Option::map_or_else. For example,
23+
/// ```rust
24+
/// # let optional = Some(0);
25+
/// let _ = if let Some(x) = optional {
26+
/// x
27+
/// } else {
28+
/// let y = 0;
29+
/// /* Some expensive computation or state-changing operation */
30+
/// y
31+
/// };
32+
/// ```
33+
/// would be better as
34+
/// ```rust
35+
/// # let optional = Some(0);
36+
/// let _ = optional.map_or_else(|| { let y = 0; /* some expensive computation or
37+
/// state-changing operation */ y }, |x| x);
38+
/// ```
39+
/// instead of the suggested
40+
/// ```rust
41+
/// # let optional = Some(0);
42+
/// let _ = optional.map_or({ let y = 0; /* some expensive computation or state-changing
43+
/// operation */ y }, |x| x);
44+
/// ```
2045
///
2146
/// **Example:**
2247
///
2348
/// ```rust
24-
/// let var: Option<u32> = Some(5u32);
25-
/// let _ = if let Some(foo) = var {
49+
/// # let optional: Option<u32> = Some(0);
50+
/// let _ = if let Some(foo) = optional {
2651
/// foo
2752
/// } else {
2853
/// 5
@@ -32,8 +57,8 @@ declare_clippy_lint! {
3257
/// should be
3358
///
3459
/// ```rust
35-
/// let var: Option<u32> = Some(5u32);
36-
/// let _ = var.map_or(5, |foo| foo);
60+
/// # let optional: Option<u32> = Some(0);
61+
/// let _ = optional.map_or(5, |foo| foo);
3762
/// ```
3863
pub OPTION_IF_LET_ELSE,
3964
style,
@@ -45,57 +70,52 @@ declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]);
4570
/// If this is the option if let/else thing we're detecting, then this
4671
/// function returns Some(Option<_> compared, expression if Option is
4772
/// Some, expression if Option is None). Otherwise, it returns None
48-
fn detect_option_if_let_else<'a>(
49-
cx: &LateContext<'_, '_>,
50-
expr: &'a Expr<'_>,
51-
) -> Option<(&'a Expr<'a>, &'a Expr<'a>, &'a Expr<'a>)> {
52-
if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind {
53-
dbg!("Found if let/else stmt");
54-
if arms.len() == 2 {
55-
dbg!("If let/else statement has two arms");
56-
if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION) {
57-
dbg!("rhs of if let is Option<T>");
58-
if let PatKind::TupleStruct(path, &[inner_pat], _) = &arms[0].pat.kind {
59-
dbg!("arm0 is TupleStruct");
60-
if let PatKind::Wild | PatKind::Binding(..) = &inner_pat.kind {
61-
dbg!("inside of Some(x) matches anything");
62-
let (some_body, none_body) = if match_qpath(path, &paths::OPTION_SOME) {
63-
(arms[0].body, arms[1].body)
64-
} else {
65-
(arms[1].body, arms[0].body)
66-
};
67-
return Some((let_body, some_body, none_body));
68-
}
69-
}
70-
}
73+
fn detect_option_if_let_else<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'_>) -> Option<(String, String, String)> {
74+
if_chain! {
75+
if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
76+
if arms.len() == 2;
77+
if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION);
78+
if let PatKind::TupleStruct(path, &[inner_pat], _) = &arms[0].pat.kind;
79+
if let PatKind::Binding(_, _, id, _) = &inner_pat.kind;
80+
then {
81+
let (mut some_body, mut none_body) = if match_qpath(path, &paths::OPTION_SOME) {
82+
(arms[0].body, arms[1].body)
83+
} else {
84+
(arms[1].body, arms[0].body)
85+
};
86+
some_body = if let ExprKind::Block(Block { stmts: &[], expr: Some(expr), .. }, _) = &some_body.kind {
87+
expr
88+
} else {
89+
some_body
90+
};
91+
none_body = if let ExprKind::Block(Block { stmts: &[], expr: Some(expr), .. }, _) = &none_body.kind {
92+
expr
93+
} else {
94+
none_body
95+
};
96+
let capture_name = id.name.to_ident_string();
97+
Some((
98+
format!("{}", Sugg::hir(cx, let_body, "..")),
99+
format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")),
100+
format!("{}", Sugg::hir(cx, none_body, ".."))
101+
))
102+
} else {
103+
None
71104
}
72105
}
73-
None
74106
}
75107

76108
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse {
77109
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
78-
span_lint(
79-
cx,
80-
OPTION_IF_LET_ELSE,
81-
expr.span,
82-
"OptionIfLetElse::check_expr was run on this span",
83-
);
84-
return;
85110
if let Some((option, map, else_func)) = detect_option_if_let_else(cx, expr) {
86111
span_lint_and_sugg(
87112
cx,
88113
OPTION_IF_LET_ELSE,
89114
expr.span,
90115
"use Option::map_or_else instead of an if let/else",
91116
"try",
92-
format!(
93-
"{}.map_or_else({}, {})",
94-
Sugg::hir(cx, option, ".."),
95-
Sugg::hir(cx, else_func, ".."),
96-
Sugg::hir(cx, map, "..")
97-
),
98-
Applicability::MachineApplicable,
117+
format!("{}.map_or_else({}, {})", option, else_func, map),
118+
Applicability::MaybeIncorrect,
99119
);
100120
}
101121
}

‎tests/ui/option_if_let_else.rs

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,22 @@ fn bad1(string: Option<&str>) -> (bool, &str) {
88
}
99
}
1010

11+
fn longer_body(arg: Option<u32>) -> u32 {
12+
if let Some(x) = arg {
13+
let y = x * x;
14+
y * y
15+
} else {
16+
13
17+
}
18+
}
19+
20+
fn negative_tests(arg: Option<u32>) {
21+
let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
22+
}
23+
1124
fn main() {
1225
let optional = Some(5);
13-
let _ = if let Some(x) = optional {
14-
x + 2
15-
} else {
16-
5
17-
};
26+
let _ = if let Some(x) = optional { x + 2 } else { 5 };
1827
let _ = bad1(None);
28+
let _ = longer_body(None);
1929
}

‎tests/ui/option_if_let_else.stderr

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
error: use Option::map_or_else instead of an if let/else
2+
--> $DIR/option_if_let_else.rs:4:5
3+
|
4+
LL | / if let Some(x) = string {
5+
LL | | (true, x)
6+
LL | | } else {
7+
LL | | (false, "hello")
8+
LL | | }
9+
| |_____^ help: try: `string.map_or_else((false, "hello"), |x| (true, x))`
10+
|
11+
= note: `-D clippy::option-if-let-else` implied by `-D warnings`
12+
13+
error: use Option::map_or_else instead of an if let/else
14+
--> $DIR/option_if_let_else.rs:12:5
15+
|
16+
LL | / if let Some(x) = arg {
17+
LL | | let y = x * x;
18+
LL | | y * y
19+
LL | | } else {
20+
LL | | 13
21+
LL | | }
22+
| |_____^
23+
|
24+
help: try
25+
|
26+
LL | arg.map_or_else(13, |x| {
27+
LL | let y = x * x;
28+
LL | y * y
29+
LL | })
30+
|
31+
32+
error: use Option::map_or_else instead of an if let/else
33+
--> $DIR/option_if_let_else.rs:26:13
34+
|
35+
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
36+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or_else(5, |x| x + 2)`
37+
38+
error: aborting due to 3 previous errors
39+

0 commit comments

Comments
 (0)