Skip to content

Commit 6ac42fe

Browse files
borsbors
committed
Auto merge of rust-lang#7971 - togami2864:fix/option-map-or-none, r=llogiq
fix suggestion in option_map_or_none fix: rust-lang#7960 changelog: change suggestion in the lint rule `option_map_or_none`
2 parents d550e5f + 8e317f5 commit 6ac42fe

File tree

4 files changed

+141
-69
lines changed

4 files changed

+141
-69
lines changed

‎clippy_lints/src/methods/option_map_or_none.rs

+89-50
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::is_lang_ctor;
32
use clippy_utils::source::snippet;
43
use clippy_utils::ty::is_type_diagnostic_item;
4+
use clippy_utils::{is_lang_ctor, single_segment_path};
55
use rustc_errors::Applicability;
66
use rustc_hir as hir;
77
use rustc_hir::LangItem::{OptionNone, OptionSome};
@@ -11,6 +11,28 @@ use rustc_span::symbol::sym;
1111
use super::OPTION_MAP_OR_NONE;
1212
use super::RESULT_MAP_OR_INTO_OPTION;
1313

14+
// The expression inside a closure may or may not have surrounding braces
15+
// which causes problems when generating a suggestion.
16+
fn reduce_unit_expression<'a>(
17+
cx: &LateContext<'_>,
18+
expr: &'a hir::Expr<'_>,
19+
) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> {
20+
match expr.kind {
21+
hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)),
22+
hir::ExprKind::Block(block, _) => {
23+
match (block.stmts, block.expr) {
24+
(&[], Some(inner_expr)) => {
25+
// If block only contains an expression,
26+
// reduce `|x| { x + 1 }` to `|x| x + 1`
27+
reduce_unit_expression(cx, inner_expr)
28+
},
29+
_ => None,
30+
}
31+
},
32+
_ => None,
33+
}
34+
}
35+
1436
/// lint use of `_.map_or(None, _)` for `Option`s and `Result`s
1537
pub(super) fn check<'tcx>(
1638
cx: &LateContext<'tcx>,
@@ -31,58 +53,75 @@ pub(super) fn check<'tcx>(
3153
return;
3254
}
3355

34-
let (lint_name, msg, instead, hint) = {
35-
let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind {
36-
is_lang_ctor(cx, qpath, OptionNone)
37-
} else {
38-
return;
39-
};
56+
let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind {
57+
is_lang_ctor(cx, qpath, OptionNone)
58+
} else {
59+
return;
60+
};
4061

41-
if !default_arg_is_none {
42-
// nothing to lint!
43-
return;
44-
}
62+
if !default_arg_is_none {
63+
// nothing to lint!
64+
return;
65+
}
4566

46-
let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind {
47-
is_lang_ctor(cx, qpath, OptionSome)
48-
} else {
49-
false
50-
};
67+
let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind {
68+
is_lang_ctor(cx, qpath, OptionSome)
69+
} else {
70+
false
71+
};
72+
73+
if is_option {
74+
let self_snippet = snippet(cx, recv.span, "..");
75+
if_chain! {
76+
if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind;
77+
let arg_snippet = snippet(cx, span, "..");
78+
let body = cx.tcx.hir().body(id);
79+
if let Some((func, arg_char)) = reduce_unit_expression(cx, &body.value);
80+
if arg_char.len() == 1;
81+
if let hir::ExprKind::Path(ref qpath) = func.kind;
82+
if let Some(segment) = single_segment_path(qpath);
83+
if segment.ident.name == sym::Some;
84+
then {
85+
let func_snippet = snippet(cx, arg_char[0].span, "..");
86+
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
87+
`map(..)` instead";
88+
return span_lint_and_sugg(
89+
cx,
90+
OPTION_MAP_OR_NONE,
91+
expr.span,
92+
msg,
93+
"try using `map` instead",
94+
format!("{0}.map({1} {2})", self_snippet, arg_snippet,func_snippet),
95+
Applicability::MachineApplicable,
96+
);
97+
}
5198

52-
if is_option {
53-
let self_snippet = snippet(cx, recv.span, "..");
54-
let func_snippet = snippet(cx, map_arg.span, "..");
55-
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
56-
`and_then(..)` instead";
57-
(
58-
OPTION_MAP_OR_NONE,
59-
msg,
60-
"try using `and_then` instead",
61-
format!("{0}.and_then({1})", self_snippet, func_snippet),
62-
)
63-
} else if f_arg_is_some {
64-
let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \
65-
`ok()` instead";
66-
let self_snippet = snippet(cx, recv.span, "..");
67-
(
68-
RESULT_MAP_OR_INTO_OPTION,
69-
msg,
70-
"try using `ok` instead",
71-
format!("{0}.ok()", self_snippet),
72-
)
73-
} else {
74-
// nothing to lint!
75-
return;
7699
}
77-
};
78100

79-
span_lint_and_sugg(
80-
cx,
81-
lint_name,
82-
expr.span,
83-
msg,
84-
instead,
85-
hint,
86-
Applicability::MachineApplicable,
87-
);
101+
let func_snippet = snippet(cx, map_arg.span, "..");
102+
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
103+
`and_then(..)` instead";
104+
return span_lint_and_sugg(
105+
cx,
106+
OPTION_MAP_OR_NONE,
107+
expr.span,
108+
msg,
109+
"try using `and_then` instead",
110+
format!("{0}.and_then({1})", self_snippet, func_snippet),
111+
Applicability::MachineApplicable,
112+
);
113+
} else if f_arg_is_some {
114+
let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \
115+
`ok()` instead";
116+
let self_snippet = snippet(cx, recv.span, "..");
117+
return span_lint_and_sugg(
118+
cx,
119+
RESULT_MAP_OR_INTO_OPTION,
120+
expr.span,
121+
msg,
122+
"try using `ok` instead",
123+
format!("{0}.ok()", self_snippet),
124+
Applicability::MachineApplicable,
125+
);
126+
}
88127
}

‎tests/ui/option_map_or_none.fixed

+10-4
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,19 @@
44

55
fn main() {
66
let opt = Some(1);
7+
let bar = |_| Some(1);
78

89
// Check `OPTION_MAP_OR_NONE`.
910
// Single line case.
10-
let _ = opt.and_then(|x| Some(x + 1));
11+
let _: Option<i32> = opt.map(|x| x + 1);
1112
// Multi-line case.
1213
#[rustfmt::skip]
13-
let _ = opt.and_then(|x| {
14-
Some(x + 1)
15-
});
14+
let _: Option<i32> = opt.map(|x| x + 1);
15+
// function returning `Option`
16+
let _: Option<i32> = opt.and_then(bar);
17+
let _: Option<i32> = opt.and_then(|x| {
18+
let offset = 0;
19+
let height = x;
20+
Some(offset + height)
21+
});
1622
}

‎tests/ui/option_map_or_none.rs

+10-2
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,21 @@
44

55
fn main() {
66
let opt = Some(1);
7+
let bar = |_| Some(1);
78

89
// Check `OPTION_MAP_OR_NONE`.
910
// Single line case.
10-
let _ = opt.map_or(None, |x| Some(x + 1));
11+
let _: Option<i32> = opt.map_or(None, |x| Some(x + 1));
1112
// Multi-line case.
1213
#[rustfmt::skip]
13-
let _ = opt.map_or(None, |x| {
14+
let _: Option<i32> = opt.map_or(None, |x| {
1415
Some(x + 1)
1516
});
17+
// function returning `Option`
18+
let _: Option<i32> = opt.map_or(None, bar);
19+
let _: Option<i32> = opt.map_or(None, |x| {
20+
let offset = 0;
21+
let height = x;
22+
Some(offset + height)
23+
});
1624
}

‎tests/ui/option_map_or_none.stderr

+32-13
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,45 @@
1-
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
2-
--> $DIR/option_map_or_none.rs:10:13
1+
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
2+
--> $DIR/option_map_or_none.rs:11:26
33
|
4-
LL | let _ = opt.map_or(None, |x| Some(x + 1));
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(|x| Some(x + 1))`
4+
LL | let _: Option<i32> = opt.map_or(None, |x| Some(x + 1));
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| x + 1)`
66
|
77
= note: `-D clippy::option-map-or-none` implied by `-D warnings`
88

9-
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
10-
--> $DIR/option_map_or_none.rs:13:13
9+
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
10+
--> $DIR/option_map_or_none.rs:14:26
1111
|
12-
LL | let _ = opt.map_or(None, |x| {
13-
| _____________^
12+
LL | let _: Option<i32> = opt.map_or(None, |x| {
13+
| __________________________^
1414
LL | | Some(x + 1)
1515
LL | | });
16-
| |_________________________^
16+
| |_________________________^ help: try using `map` instead: `opt.map(|x| x + 1)`
17+
18+
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
19+
--> $DIR/option_map_or_none.rs:18:26
20+
|
21+
LL | let _: Option<i32> = opt.map_or(None, bar);
22+
| ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)`
23+
24+
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
25+
--> $DIR/option_map_or_none.rs:19:26
26+
|
27+
LL | let _: Option<i32> = opt.map_or(None, |x| {
28+
| __________________________^
29+
LL | | let offset = 0;
30+
LL | | let height = x;
31+
LL | | Some(offset + height)
32+
LL | | });
33+
| |______^
1734
|
1835
help: try using `and_then` instead
1936
|
20-
LL ~ let _ = opt.and_then(|x| {
21-
LL + Some(x + 1)
22-
LL ~ });
37+
LL ~ let _: Option<i32> = opt.and_then(|x| {
38+
LL + let offset = 0;
39+
LL + let height = x;
40+
LL + Some(offset + height)
41+
LL ~ });
2342
|
2443

25-
error: aborting due to 2 previous errors
44+
error: aborting due to 4 previous errors
2645

0 commit comments

Comments
 (0)