Skip to content

Commit 2cc25c3

Browse files
tnielenstnielens
committed
option_if_let_else / distinguish pure from impure else expressions / pr remarks
1 parent eb9d3fc commit 2cc25c3

File tree

6 files changed

+111
-107
lines changed

6 files changed

+111
-107
lines changed

‎clippy_lints/src/methods/bind_instead_of_map.rs

+33-17
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use rustc_middle::hir::map::Map;
1212
use rustc_span::Span;
1313

1414
pub(crate) struct OptionAndThenSome;
15+
1516
impl BindInsteadOfMap for OptionAndThenSome {
1617
const TYPE_NAME: &'static str = "Option";
1718
const TYPE_QPATH: &'static [&'static str] = &paths::OPTION;
@@ -24,6 +25,7 @@ impl BindInsteadOfMap for OptionAndThenSome {
2425
}
2526

2627
pub(crate) struct ResultAndThenOk;
28+
2729
impl BindInsteadOfMap for ResultAndThenOk {
2830
const TYPE_NAME: &'static str = "Result";
2931
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
@@ -36,6 +38,7 @@ impl BindInsteadOfMap for ResultAndThenOk {
3638
}
3739

3840
pub(crate) struct ResultOrElseErrInfo;
41+
3942
impl BindInsteadOfMap for ResultOrElseErrInfo {
4043
const TYPE_NAME: &'static str = "Result";
4144
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
@@ -57,6 +60,10 @@ pub(crate) trait BindInsteadOfMap {
5760

5861
const GOOD_METHOD_NAME: &'static str;
5962

63+
fn is_applicable(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>], dry_run: bool) -> bool {
64+
Self::lint(cx, expr, args, dry_run)
65+
}
66+
6067
fn no_op_msg() -> String {
6168
format!(
6269
"using `{}.{}({})`, which is a no-op",
@@ -82,6 +89,7 @@ pub(crate) trait BindInsteadOfMap {
8289
args: &[hir::Expr<'_>],
8390
closure_expr: &hir::Expr<'_>,
8491
closure_args_span: Span,
92+
dry_run: bool,
8593
) -> bool {
8694
if_chain! {
8795
if let hir::ExprKind::Call(ref some_expr, ref some_args) = closure_expr.kind;
@@ -104,6 +112,7 @@ pub(crate) trait BindInsteadOfMap {
104112
let closure_args_snip = snippet(cx, closure_args_span, "..");
105113
let option_snip = snippet(cx, args[0].span, "..");
106114
let note = format!("{}.{}({} {})", option_snip, Self::GOOD_METHOD_NAME, closure_args_snip, some_inner_snip);
115+
if !dry_run{
107116
span_lint_and_sugg(
108117
cx,
109118
BIND_INSTEAD_OF_MAP,
@@ -113,16 +122,17 @@ pub(crate) trait BindInsteadOfMap {
113122
note,
114123
Applicability::MachineApplicable,
115124
);
125+
}
116126
true
117127
} else {
118128
false
119129
}
120130
}
121131
}
122132

123-
fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) {
133+
fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>, dry_run: bool) -> bool {
124134
let mut suggs = Vec::new();
125-
let can_sugg = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
135+
let can_sugg: bool = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
126136
if_chain! {
127137
if !in_macro(ret_expr.span);
128138
if let hir::ExprKind::Call(ref func_path, ref args) = ret_expr.kind;
@@ -139,7 +149,7 @@ pub(crate) trait BindInsteadOfMap {
139149
}
140150
});
141151

142-
if can_sugg {
152+
if can_sugg && !dry_run {
143153
span_lint_and_then(cx, BIND_INSTEAD_OF_MAP, expr.span, Self::lint_msg().as_ref(), |diag| {
144154
multispan_sugg_with_applicability(
145155
diag,
@@ -153,36 +163,42 @@ pub(crate) trait BindInsteadOfMap {
153163
)
154164
});
155165
}
166+
can_sugg
156167
}
157168

158169
/// Lint use of `_.and_then(|x| Some(y))` for `Option`s
159-
fn lint(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
170+
fn lint(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>], dry_run: bool) -> bool {
160171
if !match_type(cx, cx.typeck_results().expr_ty(&args[0]), Self::TYPE_QPATH) {
161-
return;
172+
return false;
162173
}
163174

164175
match args[1].kind {
165176
hir::ExprKind::Closure(_, _, body_id, closure_args_span, _) => {
166177
let closure_body = cx.tcx.hir().body(body_id);
167178
let closure_expr = remove_blocks(&closure_body.value);
168179

169-
if !Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span) {
170-
Self::lint_closure(cx, expr, closure_expr);
180+
if Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span, dry_run) {
181+
true
182+
} else {
183+
Self::lint_closure(cx, expr, closure_expr, dry_run)
171184
}
172185
},
173186
// `_.and_then(Some)` case, which is no-op.
174187
hir::ExprKind::Path(ref qpath) if match_qpath(qpath, Self::BAD_VARIANT_QPATH) => {
175-
span_lint_and_sugg(
176-
cx,
177-
BIND_INSTEAD_OF_MAP,
178-
expr.span,
179-
Self::no_op_msg().as_ref(),
180-
"use the expression directly",
181-
snippet(cx, args[0].span, "..").into(),
182-
Applicability::MachineApplicable,
183-
);
188+
if !dry_run {
189+
span_lint_and_sugg(
190+
cx,
191+
BIND_INSTEAD_OF_MAP,
192+
expr.span,
193+
Self::no_op_msg().as_ref(),
194+
"use the expression directly",
195+
snippet(cx, args[0].span, "..").into(),
196+
Applicability::MachineApplicable,
197+
);
198+
}
199+
true
184200
},
185-
_ => {},
201+
_ => false,
186202
}
187203
}
188204
}

‎clippy_lints/src/methods/mod.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -1461,12 +1461,12 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
14611461
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
14621462
["and_then", ..] => {
14631463
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "and");
1464-
bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0]);
1465-
bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0]);
1464+
bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0], false);
1465+
bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0], false);
14661466
},
14671467
["or_else", ..] => {
14681468
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "or");
1469-
bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0]);
1469+
bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0], false);
14701470
},
14711471
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
14721472
["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),

‎clippy_lints/src/methods/unnecessary_lazy_eval.rs

+10
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use crate::methods::{bind_instead_of_map, BindInsteadOfMap};
12
use crate::utils::{eager_or_lazy, usage};
23
use crate::utils::{is_type_diagnostic_item, snippet, span_lint_and_sugg};
34
use rustc_errors::Applicability;
@@ -17,6 +18,15 @@ pub(super) fn lint<'tcx>(
1718
let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&args[0]), sym!(option_type));
1819
let is_result = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&args[0]), sym!(result_type));
1920

21+
// give priority to bind_instead_of_map because it is more specific
22+
if is_option && bind_instead_of_map::OptionAndThenSome::lint(cx, expr, args, true)
23+
|| is_result
24+
&& (bind_instead_of_map::ResultAndThenOk::lint(cx, expr, args, true)
25+
|| bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, args, true))
26+
{
27+
return;
28+
}
29+
2030
if is_option || is_result {
2131
if let hir::ExprKind::Closure(_, _, eid, _, _) = args[1].kind {
2232
let body = cx.tcx.hir().body(eid);

‎tests/ui/unnecessary_lazy_eval.fixed

+23-10
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ fn main() {
2929
let ext_opt = Some(42);
3030
let nested_opt = Some(Some(42));
3131
let nested_tuple_opt = Some(Some((42, 43)));
32-
let mut deep = Deep(Some(42));
3332

3433
// Should lint - Option
3534
let _ = opt.unwrap_or(2);
@@ -42,10 +41,6 @@ fn main() {
4241
let _ = opt.get_or_insert(2);
4342
let _ = opt.ok_or(2);
4443
let _ = opt.ok_or_else(|| ext_arr[0]);
45-
let _ = nested_tuple_opt.unwrap_or(Some((1, 2)));
46-
let _: Option<usize> = None.or(Some(3));
47-
let _ = deep.0.or(Some(3));
48-
let _ = opt.or(Some(3));
4944

5045
// Cases when unwrap is not called on a simple variable
5146
let _ = Some(10).unwrap_or(2);
@@ -55,6 +50,7 @@ fn main() {
5550
let _: Result<usize, usize> = None.ok_or(2);
5651
let _: Option<usize> = None.or(None);
5752

53+
let mut deep = Deep(Some(42));
5854
let _ = deep.0.unwrap_or(2);
5955
let _ = deep.0.and(ext_opt);
6056
let _ = deep.0.or(None);
@@ -64,6 +60,7 @@ fn main() {
6460
// Should not lint - Option
6561
let _ = opt.unwrap_or_else(|| ext_str.return_some_field());
6662
let _ = nested_opt.unwrap_or_else(|| Some(some_call()));
63+
let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2)));
6764
let _ = nested_tuple_opt.unwrap_or_else(|| Some((some_call(), some_call())));
6865
let _ = opt.or_else(some_call);
6966
let _ = opt.or_else(|| some_call());
@@ -72,8 +69,13 @@ fn main() {
7269
let _ = deep.0.get_or_insert_with(|| some_call());
7370
let _ = deep.0.or_else(some_call);
7471
let _ = deep.0.or_else(|| some_call());
72+
73+
// These are handled by bind_instead_of_map
7574
let _ = Some(10).and_then(|idx| Some(ext_arr[idx]));
7675
let _ = Some(10).and_then(|idx| Some(idx));
76+
let _: Option<usize> = None.or_else(|| Some(3));
77+
let _ = deep.0.or_else(|| Some(3));
78+
let _ = opt.or_else(|| Some(3));
7779

7880
// Should lint - Result
7981
let res: Result<usize, usize> = Err(5);
@@ -96,9 +98,20 @@ fn main() {
9698
let _: Result<usize, usize> = res.or_else(|err| Ok(err));
9799
let _: Result<usize, usize> = res.or_else(|err| Err(err));
98100

99-
// These are also handled by bind_instead_of_map
100-
let _: Result<usize, usize> = res.and(Ok(2));
101-
let _: Result<usize, usize> = res.and(Err(2));
102-
let _: Result<usize, usize> = res.or(Ok(2));
103-
let _: Result<usize, usize> = res.or(Err(2));
101+
// These are handled by bind_instead_of_map
102+
let _: Result<usize, usize> = res.and_then(|_| Ok(2));
103+
let _: Result<usize, usize> = res.and_then(|_| Ok(astronomers_pi));
104+
let _: Result<usize, usize> = res.and_then(|_| Ok(ext_str.some_field));
105+
106+
let _: Result<usize, usize> = res.and_then(|_| Err(2));
107+
let _: Result<usize, usize> = res.and_then(|_| Err(astronomers_pi));
108+
let _: Result<usize, usize> = res.and_then(|_| Err(ext_str.some_field));
109+
110+
let _: Result<usize, usize> = res.or_else(|_| Ok(2));
111+
let _: Result<usize, usize> = res.or_else(|_| Ok(astronomers_pi));
112+
let _: Result<usize, usize> = res.or_else(|_| Ok(ext_str.some_field));
113+
114+
let _: Result<usize, usize> = res.or_else(|_| Err(2));
115+
let _: Result<usize, usize> = res.or_else(|_| Err(astronomers_pi));
116+
let _: Result<usize, usize> = res.or_else(|_| Err(ext_str.some_field));
104117
}

‎tests/ui/unnecessary_lazy_eval.rs

+19-6
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ fn main() {
2929
let ext_opt = Some(42);
3030
let nested_opt = Some(Some(42));
3131
let nested_tuple_opt = Some(Some((42, 43)));
32-
let mut deep = Deep(Some(42));
3332

3433
// Should lint - Option
3534
let _ = opt.unwrap_or_else(|| 2);
@@ -42,10 +41,6 @@ fn main() {
4241
let _ = opt.get_or_insert_with(|| 2);
4342
let _ = opt.ok_or_else(|| 2);
4443
let _ = opt.ok_or_else(|| ext_arr[0]);
45-
let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2)));
46-
let _: Option<usize> = None.or_else(|| Some(3));
47-
let _ = deep.0.or_else(|| Some(3));
48-
let _ = opt.or_else(|| Some(3));
4944

5045
// Cases when unwrap is not called on a simple variable
5146
let _ = Some(10).unwrap_or_else(|| 2);
@@ -55,6 +50,7 @@ fn main() {
5550
let _: Result<usize, usize> = None.ok_or_else(|| 2);
5651
let _: Option<usize> = None.or_else(|| None);
5752

53+
let mut deep = Deep(Some(42));
5854
let _ = deep.0.unwrap_or_else(|| 2);
5955
let _ = deep.0.and_then(|_| ext_opt);
6056
let _ = deep.0.or_else(|| None);
@@ -64,6 +60,7 @@ fn main() {
6460
// Should not lint - Option
6561
let _ = opt.unwrap_or_else(|| ext_str.return_some_field());
6662
let _ = nested_opt.unwrap_or_else(|| Some(some_call()));
63+
let _ = nested_tuple_opt.unwrap_or_else(|| Some((1, 2)));
6764
let _ = nested_tuple_opt.unwrap_or_else(|| Some((some_call(), some_call())));
6865
let _ = opt.or_else(some_call);
6966
let _ = opt.or_else(|| some_call());
@@ -72,8 +69,13 @@ fn main() {
7269
let _ = deep.0.get_or_insert_with(|| some_call());
7370
let _ = deep.0.or_else(some_call);
7471
let _ = deep.0.or_else(|| some_call());
72+
73+
// These are handled by bind_instead_of_map
7574
let _ = Some(10).and_then(|idx| Some(ext_arr[idx]));
7675
let _ = Some(10).and_then(|idx| Some(idx));
76+
let _: Option<usize> = None.or_else(|| Some(3));
77+
let _ = deep.0.or_else(|| Some(3));
78+
let _ = opt.or_else(|| Some(3));
7779

7880
// Should lint - Result
7981
let res: Result<usize, usize> = Err(5);
@@ -96,9 +98,20 @@ fn main() {
9698
let _: Result<usize, usize> = res.or_else(|err| Ok(err));
9799
let _: Result<usize, usize> = res.or_else(|err| Err(err));
98100

99-
// These are also handled by bind_instead_of_map
101+
// These are handled by bind_instead_of_map
100102
let _: Result<usize, usize> = res.and_then(|_| Ok(2));
103+
let _: Result<usize, usize> = res.and_then(|_| Ok(astronomers_pi));
104+
let _: Result<usize, usize> = res.and_then(|_| Ok(ext_str.some_field));
105+
101106
let _: Result<usize, usize> = res.and_then(|_| Err(2));
107+
let _: Result<usize, usize> = res.and_then(|_| Err(astronomers_pi));
108+
let _: Result<usize, usize> = res.and_then(|_| Err(ext_str.some_field));
109+
102110
let _: Result<usize, usize> = res.or_else(|_| Ok(2));
111+
let _: Result<usize, usize> = res.or_else(|_| Ok(astronomers_pi));
112+
let _: Result<usize, usize> = res.or_else(|_| Ok(ext_str.some_field));
113+
103114
let _: Result<usize, usize> = res.or_else(|_| Err(2));
115+
let _: Result<usize, usize> = res.or_else(|_| Err(astronomers_pi));
116+
let _: Result<usize, usize> = res.or_else(|_| Err(ext_str.some_field));
104117
}

0 commit comments

Comments
 (0)