Skip to content

Commit 622a77d

Browse files
author
ceptontech
committed
feat(search_is_some): Fix when the closure spans multiple lines
Previously the program only fixed the code when the closure supplied to the method contained only 1 line. This patch removes the restriction. The code already works. This patch only removes the extra check that causes the restriction. The test cases that can now be fixed are moved into the files containing tests cases that can be fixed. The unnecessary check has survived in the code this way. - In Dec 2015, patch a6bd2d0, pull request 524. The lint was first added. The program did not support fixing code automatically yet. So the suggested fix was printed as a part of the diagnostic message. When the original code contained multiple lines, the suggested fix was omitted in order to keep the diagnostic message concise. - In May 2019, patch bd0b75f, pull request 4049. Logic was added to strip the reference in the closure when the suggested replacement method required it. Because the fix was still only printed when the code contained a single line, the new transformation was only done when the code contained a single line. - In Aug 2019, patch 945d4cf, pull request 4454. The lint was updated to fix code automatically. Because the fixed code had only been printed in the diagnostic message for a single line, the fix was only added for a single line. - In Nov 2021, patch 092fe20, pull request 7463. The logic for transforming the closure was moved into another file. A comment was added saying that it was only good for a single line because it had only been used for a single line. changelog: [`search_is_some`] now fixes code spanning multiple lines
1 parent d1b51ea commit 622a77d

10 files changed

+398
-315
lines changed

‎clippy_lints/src/methods/search_is_some.rs‎

Lines changed: 55 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
22
use clippy_utils::res::{MaybeDef, MaybeTypeckRes};
33
use clippy_utils::source::{snippet, snippet_with_applicability};
44
use clippy_utils::sugg::deref_closure_args;
@@ -34,79 +34,67 @@ pub(super) fn check<'tcx>(
3434
{
3535
let msg = format!("called `{option_check_method}()` after searching an `Iterator` with `{search_method}`");
3636
let search_snippet = snippet(cx, search_arg.span, "..");
37-
if search_snippet.lines().count() <= 1 {
38-
// suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()`
39-
// suggest `any(|..| *..)` instead of `any(|..| **..)` for `find(|..| **..).is_some()`
40-
let mut applicability = Applicability::MachineApplicable;
41-
let any_search_snippet = if search_method == sym::find
42-
&& let ExprKind::Closure(&hir::Closure { body, .. }) = search_arg.kind
43-
&& let closure_body = cx.tcx.hir_body(body)
44-
&& let Some(closure_arg) = closure_body.params.first()
45-
{
46-
if let PatKind::Ref(..) = closure_arg.pat.kind {
47-
Some(search_snippet.replacen('&', "", 1))
48-
} else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind {
49-
// `find()` provides a reference to the item, but `any` does not,
50-
// so we should fix item usages for suggestion
51-
if let Some(closure_sugg) = deref_closure_args(cx, search_arg) {
52-
applicability = closure_sugg.applicability;
53-
Some(closure_sugg.suggestion)
54-
} else {
55-
Some(search_snippet.to_string())
56-
}
37+
// suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()`
38+
// suggest `any(|..| *..)` instead of `any(|..| **..)` for `find(|..| **..).is_some()`
39+
let mut applicability = Applicability::MachineApplicable;
40+
let any_search_snippet = if search_method == sym::find
41+
&& let ExprKind::Closure(&hir::Closure { body, .. }) = search_arg.kind
42+
&& let closure_body = cx.tcx.hir_body(body)
43+
&& let Some(closure_arg) = closure_body.params.first()
44+
{
45+
if let PatKind::Ref(..) = closure_arg.pat.kind {
46+
Some(search_snippet.replacen('&', "", 1))
47+
} else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind {
48+
// `find()` provides a reference to the item, but `any` does not,
49+
// so we should fix item usages for suggestion
50+
if let Some(closure_sugg) = deref_closure_args(cx, search_arg) {
51+
applicability = closure_sugg.applicability;
52+
Some(closure_sugg.suggestion)
5753
} else {
58-
None
54+
Some(search_snippet.to_string())
5955
}
6056
} else {
6157
None
62-
};
63-
// add note if not multi-line
64-
if is_some {
65-
span_lint_and_sugg(
66-
cx,
67-
SEARCH_IS_SOME,
68-
method_span.with_hi(expr.span.hi()),
69-
msg,
70-
"consider using",
71-
format!(
72-
"any({})",
73-
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
74-
),
75-
applicability,
76-
);
77-
} else {
78-
let iter = snippet(cx, search_recv.span, "..");
79-
let sugg = if is_receiver_of_method_call(cx, expr) {
80-
format!(
81-
"(!{iter}.any({}))",
82-
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
83-
)
84-
} else {
85-
format!(
86-
"!{iter}.any({})",
87-
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
88-
)
89-
};
90-
span_lint_and_sugg(
91-
cx,
92-
SEARCH_IS_SOME,
93-
expr.span,
94-
msg,
95-
"consider using",
96-
sugg,
97-
applicability,
98-
);
9958
}
10059
} else {
101-
let hint = format!(
102-
"this is more succinctly expressed by calling `any()`{}",
103-
if option_check_method == "is_none" {
104-
" with negation"
105-
} else {
106-
""
107-
}
60+
None
61+
};
62+
// add note if not multi-line
63+
if is_some {
64+
span_lint_and_sugg(
65+
cx,
66+
SEARCH_IS_SOME,
67+
method_span.with_hi(expr.span.hi()),
68+
msg,
69+
"consider using",
70+
format!(
71+
"any({})",
72+
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
73+
),
74+
applicability,
75+
);
76+
} else {
77+
let iter = snippet(cx, search_recv.span, "..");
78+
let sugg = if is_receiver_of_method_call(cx, expr) {
79+
format!(
80+
"(!{iter}.any({}))",
81+
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
82+
)
83+
} else {
84+
format!(
85+
"!{iter}.any({})",
86+
any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
87+
)
88+
};
89+
span_lint_and_sugg(
90+
cx,
91+
SEARCH_IS_SOME,
92+
expr.span,
93+
msg,
94+
"consider using",
95+
sugg,
96+
applicability,
10897
);
109-
span_lint_and_help(cx, SEARCH_IS_SOME, expr.span, msg, None, hint);
11098
}
11199
}
112100
// lint if `find()` is called by `String` or `&str`

‎clippy_utils/src/sugg.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -765,7 +765,7 @@ pub struct DerefClosure {
765765
/// such as explicit deref and borrowing cases.
766766
/// Returns `None` if no such use cases have been triggered in closure body
767767
///
768-
/// note: this only works on single line immutable closures with exactly one input parameter.
768+
/// note: This only works on immutable closures with exactly one input parameter.
769769
pub fn deref_closure_args(cx: &LateContext<'_>, closure: &hir::Expr<'_>) -> Option<DerefClosure> {
770770
if let ExprKind::Closure(&Closure {
771771
fn_decl, def_id, body, ..

‎tests/ui/search_is_some.rs‎

Lines changed: 0 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -8,31 +8,6 @@ use option_helpers::IteratorFalsePositives;
88
//@no-rustfix
99
#[rustfmt::skip]
1010
fn main() {
11-
let v = vec![3, 2, 1, 0, -1, -2, -3];
12-
let y = &&42;
13-
14-
15-
// Check `find().is_some()`, multi-line case.
16-
let _ = v.iter().find(|&x| {
17-
//~^ search_is_some
18-
*x < 0
19-
}
20-
).is_some();
21-
22-
// Check `position().is_some()`, multi-line case.
23-
let _ = v.iter().position(|&x| {
24-
//~^ search_is_some
25-
x < 0
26-
}
27-
).is_some();
28-
29-
// Check `rposition().is_some()`, multi-line case.
30-
let _ = v.iter().rposition(|&x| {
31-
//~^ search_is_some
32-
x < 0
33-
}
34-
).is_some();
35-
3611
// Check that we don't lint if the caller is not an `Iterator` or string
3712
let falsepos = IteratorFalsePositives { foo: 0 };
3813
let _ = falsepos.find().is_some();
@@ -49,31 +24,6 @@ fn main() {
4924

5025
#[rustfmt::skip]
5126
fn is_none() {
52-
let v = vec![3, 2, 1, 0, -1, -2, -3];
53-
let y = &&42;
54-
55-
56-
// Check `find().is_none()`, multi-line case.
57-
let _ = v.iter().find(|&x| {
58-
//~^ search_is_some
59-
*x < 0
60-
}
61-
).is_none();
62-
63-
// Check `position().is_none()`, multi-line case.
64-
let _ = v.iter().position(|&x| {
65-
//~^ search_is_some
66-
x < 0
67-
}
68-
).is_none();
69-
70-
// Check `rposition().is_none()`, multi-line case.
71-
let _ = v.iter().rposition(|&x| {
72-
//~^ search_is_some
73-
x < 0
74-
}
75-
).is_none();
76-
7727
// Check that we don't lint if the caller is not an `Iterator` or string
7828
let falsepos = IteratorFalsePositives { foo: 0 };
7929
let _ = falsepos.find().is_none();
@@ -87,18 +37,3 @@ fn is_none() {
8737
let _ = (0..1).find(some_closure).is_none();
8838
//~^ search_is_some
8939
}
90-
91-
#[allow(clippy::match_like_matches_macro)]
92-
fn issue15102() {
93-
let values = [None, Some(3)];
94-
let has_even = values
95-
//~^ search_is_some
96-
.iter()
97-
.find(|v| match v {
98-
Some(x) if x % 2 == 0 => true,
99-
_ => false,
100-
})
101-
.is_some();
102-
103-
println!("{has_even}");
104-
}

‎tests/ui/search_is_some.stderr‎

Lines changed: 5 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -1,109 +1,17 @@
11
error: called `is_some()` after searching an `Iterator` with `find`
2-
--> tests/ui/search_is_some.rs:16:13
3-
|
4-
LL | let _ = v.iter().find(|&x| {
5-
| _____________^
6-
LL | |
7-
LL | | *x < 0
8-
LL | | }
9-
LL | | ).is_some();
10-
| |______________________________^
11-
|
12-
= help: this is more succinctly expressed by calling `any()`
13-
= note: `-D clippy::search-is-some` implied by `-D warnings`
14-
= help: to override `-D warnings` add `#[allow(clippy::search_is_some)]`
15-
16-
error: called `is_some()` after searching an `Iterator` with `position`
17-
--> tests/ui/search_is_some.rs:23:13
18-
|
19-
LL | let _ = v.iter().position(|&x| {
20-
| _____________^
21-
LL | |
22-
LL | | x < 0
23-
LL | | }
24-
LL | | ).is_some();
25-
| |______________________________^
26-
|
27-
= help: this is more succinctly expressed by calling `any()`
28-
29-
error: called `is_some()` after searching an `Iterator` with `rposition`
30-
--> tests/ui/search_is_some.rs:30:13
31-
|
32-
LL | let _ = v.iter().rposition(|&x| {
33-
| _____________^
34-
LL | |
35-
LL | | x < 0
36-
LL | | }
37-
LL | | ).is_some();
38-
| |______________________________^
39-
|
40-
= help: this is more succinctly expressed by calling `any()`
41-
42-
error: called `is_some()` after searching an `Iterator` with `find`
43-
--> tests/ui/search_is_some.rs:46:20
2+
--> tests/ui/search_is_some.rs:21:20
443
|
454
LL | let _ = (0..1).find(some_closure).is_some();
465
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `any(some_closure)`
47-
48-
error: called `is_none()` after searching an `Iterator` with `find`
49-
--> tests/ui/search_is_some.rs:57:13
506
|
51-
LL | let _ = v.iter().find(|&x| {
52-
| _____________^
53-
LL | |
54-
LL | | *x < 0
55-
LL | | }
56-
LL | | ).is_none();
57-
| |______________________________^
58-
|
59-
= help: this is more succinctly expressed by calling `any()` with negation
60-
61-
error: called `is_none()` after searching an `Iterator` with `position`
62-
--> tests/ui/search_is_some.rs:64:13
63-
|
64-
LL | let _ = v.iter().position(|&x| {
65-
| _____________^
66-
LL | |
67-
LL | | x < 0
68-
LL | | }
69-
LL | | ).is_none();
70-
| |______________________________^
71-
|
72-
= help: this is more succinctly expressed by calling `any()` with negation
73-
74-
error: called `is_none()` after searching an `Iterator` with `rposition`
75-
--> tests/ui/search_is_some.rs:71:13
76-
|
77-
LL | let _ = v.iter().rposition(|&x| {
78-
| _____________^
79-
LL | |
80-
LL | | x < 0
81-
LL | | }
82-
LL | | ).is_none();
83-
| |______________________________^
84-
|
85-
= help: this is more succinctly expressed by calling `any()` with negation
7+
= note: `-D clippy::search-is-some` implied by `-D warnings`
8+
= help: to override `-D warnings` add `#[allow(clippy::search_is_some)]`
869

8710
error: called `is_none()` after searching an `Iterator` with `find`
88-
--> tests/ui/search_is_some.rs:87:13
11+
--> tests/ui/search_is_some.rs:37:13
8912
|
9013
LL | let _ = (0..1).find(some_closure).is_none();
9114
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `!(0..1).any(some_closure)`
9215

93-
error: called `is_some()` after searching an `Iterator` with `find`
94-
--> tests/ui/search_is_some.rs:94:20
95-
|
96-
LL | let has_even = values
97-
| ____________________^
98-
LL | |
99-
LL | | .iter()
100-
LL | | .find(|v| match v {
101-
... |
102-
LL | | })
103-
LL | | .is_some();
104-
| |__________________^
105-
|
106-
= help: this is more succinctly expressed by calling `any()`
107-
108-
error: aborting due to 9 previous errors
16+
error: aborting due to 2 previous errors
10917

‎tests/ui/search_is_some_fixable_none.fixed‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,32 @@ fn main() {
2424
let _ = !(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0);
2525
//~^ search_is_some
2626
let _ = !(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1);
27+
// Check `find().is_none()`, multi-line case.
28+
let _ = !v
29+
//~^ search_is_some
30+
.iter().any(|x| {
31+
*x < 0 //
32+
});
2733

2834
// Check `position().is_none()`, single-line case.
2935
let _ = !v.iter().any(|&x| x < 0);
3036
//~^ search_is_some
37+
// Check `position().is_none()`, multi-line case.
38+
let _ = !v
39+
//~^ search_is_some
40+
.iter().any(|&x| {
41+
x < 0 //
42+
});
3143

3244
// Check `rposition().is_none()`, single-line case.
3345
let _ = !v.iter().any(|&x| x < 0);
3446
//~^ search_is_some
47+
// Check `rposition().is_none()`, multi-line case.
48+
let _ = !v
49+
//~^ search_is_some
50+
.iter().any(|&x| {
51+
x < 0 //
52+
});
3553

3654
let s1 = String::from("hello world");
3755
let s2 = String::from("world");

0 commit comments

Comments
 (0)