Skip to content

Commit 6310134

Browse files
ada4aada4a
authored
Add a check for some followed by filter (#15745)
*[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/15745)* Add a check for some followed by filter. The program will suggest using the `then_some` method. changelog: [`filter_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some
2 parents 73f2ae7 + 436aad4 commit 6310134

12 files changed

Lines changed: 424 additions & 22 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7238,6 +7238,7 @@ Released 2018-09-13
72387238
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
72397239
[`sliced_string_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#sliced_string_as_bytes
72407240
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
7241+
[`some_filter`]: https://rust-lang.github.io/rust-clippy/master/index.html#some_filter
72417242
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
72427243
[`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc
72437244
[`std_instead_of_core`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_core

‎clippy_lints/src/declared_lints.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -474,6 +474,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
474474
crate::methods::SINGLE_CHAR_ADD_STR_INFO,
475475
crate::methods::SKIP_WHILE_NEXT_INFO,
476476
crate::methods::SLICED_STRING_AS_BYTES_INFO,
477+
crate::methods::SOME_FILTER_INFO,
477478
crate::methods::STABLE_SORT_PRIMITIVE_INFO,
478479
crate::methods::STR_SPLIT_AT_NEWLINE_INFO,
479480
crate::methods::STRING_EXTEND_CHARS_INFO,

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ mod should_implement_trait;
111111
mod single_char_add_str;
112112
mod skip_while_next;
113113
mod sliced_string_as_bytes;
114+
mod some_filter;
114115
mod stable_sort_primitive;
115116
mod str_split;
116117
mod str_splitn;
@@ -3577,6 +3578,29 @@ declare_clippy_lint! {
35773578
"slicing a string and immediately calling as_bytes is less efficient and can lead to panics"
35783579
}
35793580

3581+
declare_clippy_lint! {
3582+
/// ### What it does
3583+
/// Checks for usage of `Some(x).filter(|_| predicate)`.
3584+
///
3585+
/// ### Why is this bad?
3586+
/// Readability, this can be written more concisely as `predicate.then_some(x)`.
3587+
///
3588+
/// ### Example
3589+
/// ```no_run
3590+
/// let x = false;
3591+
/// Some(0).filter(|_| x);
3592+
/// ```
3593+
/// Use instead:
3594+
/// ```no_run
3595+
/// let x = false;
3596+
/// x.then_some(0);
3597+
/// ```
3598+
#[clippy::version = "1.97.0"]
3599+
pub SOME_FILTER,
3600+
complexity,
3601+
"using `Some(x).filter(|_| predicate)`, which is more succinctly expressed as `predicate.then(x)`"
3602+
}
3603+
35803604
declare_clippy_lint! {
35813605
/// ### What it does
35823606
/// When sorting primitive values (integers, bools, chars, as well
@@ -4900,6 +4924,7 @@ impl_lint_pass!(Methods => [
49004924
SINGLE_CHAR_ADD_STR,
49014925
SKIP_WHILE_NEXT,
49024926
SLICED_STRING_AS_BYTES,
4927+
SOME_FILTER,
49034928
STABLE_SORT_PRIMITIVE,
49044929
STRING_EXTEND_CHARS,
49054930
STRING_LIT_CHARS_ANY,
@@ -5307,6 +5332,7 @@ impl Methods {
53075332
// use the sourcemap to get the span of the closure
53085333
iter_filter::check(cx, expr, arg, span);
53095334
}
5335+
some_filter::check(cx, expr, recv, arg, self.msrv);
53105336
},
53115337
(sym::find, [arg]) => {
53125338
if let Some((sym::cloned, recv2, [], _span2, _)) = method_call(recv) {

‎clippy_lints/src/methods/some_filter.rs‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::msrvs::{self, Msrv};
3+
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
4+
use clippy_utils::{as_some_expr, pat_is_wild, peel_blocks, span_contains_comment};
5+
use rustc_ast::util::parser::ExprPrecedence;
6+
use rustc_errors::Applicability;
7+
use rustc_hir::{Body, Expr, ExprKind};
8+
use rustc_lint::LateContext;
9+
10+
use super::SOME_FILTER;
11+
12+
pub(super) fn check<'tcx>(
13+
cx: &LateContext<'tcx>,
14+
expr: &'tcx Expr<'tcx>,
15+
recv: &'tcx Expr<'tcx>,
16+
arg: &'tcx Expr<'tcx>,
17+
msrv: Msrv,
18+
) {
19+
let (condition, value) = if let Some(value) = as_some_expr(cx, recv)
20+
&& let ExprKind::Closure(c) = arg.kind
21+
&& let Body {
22+
params: [p],
23+
value: condition,
24+
} = cx.tcx.hir_body(c.body)
25+
&& pat_is_wild(cx, &p.pat.kind, arg)
26+
&& msrv.meets(cx, msrvs::BOOL_THEN_SOME)
27+
{
28+
(condition, value)
29+
} else {
30+
return;
31+
};
32+
span_lint_and_then(
33+
cx,
34+
SOME_FILTER,
35+
expr.span,
36+
"use of `Some(x).filter(|_| predicate)`",
37+
|diag| {
38+
let condition = if span_contains_comment(cx, condition.span) {
39+
condition
40+
} else {
41+
peel_blocks(condition)
42+
};
43+
let mut applicability = Applicability::MaybeIncorrect;
44+
let (condition_text, condition_is_macro) =
45+
snippet_with_context(cx, condition.span, arg.span.ctxt(), "_", &mut applicability);
46+
let parentheses = !condition_is_macro && cx.precedence(condition) < ExprPrecedence::Unambiguous;
47+
let value_text = snippet_with_applicability(cx, value.span, "_", &mut applicability);
48+
let sugg = format!(
49+
"{}{condition_text}{}.then_some({value_text})",
50+
if parentheses { "(" } else { "" },
51+
if parentheses { ")" } else { "" },
52+
);
53+
diag.span_suggestion_verbose(
54+
expr.span,
55+
"consider using `bool::then_some` instead",
56+
sugg,
57+
applicability,
58+
);
59+
diag.note(
60+
"this change will alter the order in which the condition and \
61+
the value are evaluated",
62+
);
63+
},
64+
);
65+
}

‎tests/ui/manual_filter.fixed‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#![allow(
33
unused_variables,
44
clippy::question_mark,
5+
clippy::some_filter,
56
clippy::useless_vec,
67
clippy::nonminimal_bool
78
)]

‎tests/ui/manual_filter.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#![allow(
33
unused_variables,
44
clippy::question_mark,
5+
clippy::some_filter,
56
clippy::useless_vec,
67
clippy::nonminimal_bool
78
)]

‎tests/ui/manual_filter.stderr‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: manual implementation of `Option::filter`
2-
--> tests/ui/manual_filter.rs:10:5
2+
--> tests/ui/manual_filter.rs:11:5
33
|
44
LL | / match Some(0) {
55
LL | |
@@ -14,7 +14,7 @@ LL | | };
1414
= help: to override `-D warnings` add `#[allow(clippy::manual_filter)]`
1515

1616
error: manual implementation of `Option::filter`
17-
--> tests/ui/manual_filter.rs:22:5
17+
--> tests/ui/manual_filter.rs:23:5
1818
|
1919
LL | / match Some(1) {
2020
LL | |
@@ -26,7 +26,7 @@ LL | | };
2626
| |_____^ help: try: `Some(1).filter(|&x| x <= 0)`
2727

2828
error: manual implementation of `Option::filter`
29-
--> tests/ui/manual_filter.rs:34:5
29+
--> tests/ui/manual_filter.rs:35:5
3030
|
3131
LL | / match Some(2) {
3232
LL | |
@@ -38,7 +38,7 @@ LL | | };
3838
| |_____^ help: try: `Some(2).filter(|&x| x <= 0)`
3939

4040
error: manual implementation of `Option::filter`
41-
--> tests/ui/manual_filter.rs:46:5
41+
--> tests/ui/manual_filter.rs:47:5
4242
|
4343
LL | / match Some(3) {
4444
LL | |
@@ -50,7 +50,7 @@ LL | | };
5050
| |_____^ help: try: `Some(3).filter(|&x| x > 0)`
5151

5252
error: manual implementation of `Option::filter`
53-
--> tests/ui/manual_filter.rs:59:5
53+
--> tests/ui/manual_filter.rs:60:5
5454
|
5555
LL | / match y {
5656
LL | |
@@ -62,7 +62,7 @@ LL | | };
6262
| |_____^ help: try: `y.filter(|&x| x <= 0)`
6363

6464
error: manual implementation of `Option::filter`
65-
--> tests/ui/manual_filter.rs:72:5
65+
--> tests/ui/manual_filter.rs:73:5
6666
|
6767
LL | / match Some(5) {
6868
LL | |
@@ -74,7 +74,7 @@ LL | | };
7474
| |_____^ help: try: `Some(5).filter(|&x| x > 0)`
7575

7676
error: manual implementation of `Option::filter`
77-
--> tests/ui/manual_filter.rs:84:5
77+
--> tests/ui/manual_filter.rs:85:5
7878
|
7979
LL | / match Some(6) {
8080
LL | |
@@ -86,7 +86,7 @@ LL | | };
8686
| |_____^ help: try: `Some(6).as_ref().filter(|&x| x > &0)`
8787

8888
error: manual implementation of `Option::filter`
89-
--> tests/ui/manual_filter.rs:97:5
89+
--> tests/ui/manual_filter.rs:98:5
9090
|
9191
LL | / match Some(String::new()) {
9292
LL | |
@@ -98,7 +98,7 @@ LL | | };
9898
| |_____^ help: try: `Some(String::new()).filter(|x| external_cond)`
9999

100100
error: manual implementation of `Option::filter`
101-
--> tests/ui/manual_filter.rs:109:5
101+
--> tests/ui/manual_filter.rs:110:5
102102
|
103103
LL | / if let Some(x) = Some(7) {
104104
LL | |
@@ -109,7 +109,7 @@ LL | | };
109109
| |_____^ help: try: `Some(7).filter(|&x| external_cond)`
110110

111111
error: manual implementation of `Option::filter`
112-
--> tests/ui/manual_filter.rs:116:5
112+
--> tests/ui/manual_filter.rs:117:5
113113
|
114114
LL | / match &Some(8) {
115115
LL | |
@@ -121,7 +121,7 @@ LL | | };
121121
| |_____^ help: try: `Some(8).filter(|&x| x != 0)`
122122

123123
error: manual implementation of `Option::filter`
124-
--> tests/ui/manual_filter.rs:128:5
124+
--> tests/ui/manual_filter.rs:129:5
125125
|
126126
LL | / match Some(9) {
127127
LL | |
@@ -133,7 +133,7 @@ LL | | };
133133
| |_____^ help: try: `Some(9).filter(|&x| x > 10 && x < 100)`
134134

135135
error: manual implementation of `Option::filter`
136-
--> tests/ui/manual_filter.rs:155:5
136+
--> tests/ui/manual_filter.rs:156:5
137137
|
138138
LL | / match Some(11) {
139139
LL | |
@@ -153,7 +153,7 @@ LL ~ });
153153
|
154154

155155
error: manual implementation of `Option::filter`
156-
--> tests/ui/manual_filter.rs:200:13
156+
--> tests/ui/manual_filter.rs:201:13
157157
|
158158
LL | let _ = match Some(14) {
159159
| _____________^
@@ -166,7 +166,7 @@ LL | | };
166166
| |_____^ help: try: `Some(14).filter(|&x| unsafe { f(x) })`
167167

168168
error: manual implementation of `Option::filter`
169-
--> tests/ui/manual_filter.rs:211:13
169+
--> tests/ui/manual_filter.rs:212:13
170170
|
171171
LL | let _ = match Some(15) {
172172
| _____________^
@@ -177,7 +177,7 @@ LL | | };
177177
| |_____^ help: try: `Some(15).filter(|&x| unsafe { f(x) })`
178178

179179
error: manual implementation of `Option::filter`
180-
--> tests/ui/manual_filter.rs:220:12
180+
--> tests/ui/manual_filter.rs:221:12
181181
|
182182
LL | } else if let Some(x) = Some(16) {
183183
| ____________^
@@ -190,31 +190,31 @@ LL | | };
190190
| |_____^ help: try: `{ Some(16).filter(|&x| x % 2 == 0) }`
191191

192192
error: manual implementation of `Option::filter`
193-
--> tests/ui/manual_filter.rs:303:9
193+
--> tests/ui/manual_filter.rs:304:9
194194
|
195195
LL | opt.and_then(|x| if x == 0 { None } else { Some(x) });
196196
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter(|&x| x != 0)`
197197

198198
error: manual implementation of `Option::filter`
199-
--> tests/ui/manual_filter.rs:307:9
199+
--> tests/ui/manual_filter.rs:308:9
200200
|
201201
LL | opt.and_then(move |x| if x == y { Some(x) } else { None });
202202
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter(move |&x| x == y)`
203203

204204
error: manual implementation of `Option::filter`
205-
--> tests/ui/manual_filter.rs:311:10
205+
--> tests/ui/manual_filter.rs:312:10
206206
|
207207
LL | opt1.and_then(|s| if s.len() > 2 { Some(s) } else { None });
208208
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter(|s| s.len() > 2)`
209209

210210
error: manual implementation of `Option::filter`
211-
--> tests/ui/manual_filter.rs:317:9
211+
--> tests/ui/manual_filter.rs:318:9
212212
|
213213
LL | opt.and_then(|x| if unsafe { f(x as u32) } { Some(x) } else { None });
214214
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter(|&x| unsafe { f(x as u32) })`
215215

216216
error: manual implementation of `Option::filter`
217-
--> tests/ui/manual_filter.rs:319:9
217+
--> tests/ui/manual_filter.rs:320:9
218218
|
219219
LL | opt.and_then(|x| unsafe { if f(x as u32) { Some(x) } else { None } });
220220
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `filter(|&x| unsafe { f(x as u32) })`

‎tests/ui/option_filter_map.fixed‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::option_filter_map)]
2-
#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
2+
#![allow(clippy::map_flatten, clippy::some_filter, clippy::unnecessary_map_on_constructor)]
33

44
fn main() {
55
let _ = Some(Some(1)).flatten();

‎tests/ui/option_filter_map.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::option_filter_map)]
2-
#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
2+
#![allow(clippy::map_flatten, clippy::some_filter, clippy::unnecessary_map_on_constructor)]
33

44
fn main() {
55
let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);

0 commit comments

Comments
 (0)