Skip to content

Commit 9cf8ea8

Browse files
author
ceptontech
committed
feat(manual_is_variant): Check for .iter().any()
Now the linter checks for calling the `iter` or `into_iter` method followed by calling the `all` or `any` method. We have used these methods by mistake ourselves. The developer was not familiar with the `is_some_and` method. He tried to use the `any` method on an option. The compiler suggested using `iter` first. The developer did not get to learn about the idiomatic method. changelog: [`manual_is_variant`]: check for calling the `iter` or `into_iter` method followed by calling the `all` or `any` method
1 parent e341245 commit 9cf8ea8

File tree

5 files changed

+595
-42
lines changed

5 files changed

+595
-42
lines changed

‎clippy_lints/src/methods/manual_is_variant_and.rs‎

Lines changed: 82 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::msrvs::{self, Msrv};
33
use clippy_utils::res::MaybeDef;
44
use clippy_utils::source::{snippet, snippet_with_applicability};
5-
use clippy_utils::sugg::Sugg;
5+
use clippy_utils::sugg::{DerefClosure, Sugg, deref_closure_args};
6+
use clippy_utils::ty::is_copy;
67
use clippy_utils::{get_parent_expr, sym};
78
use rustc_ast::LitKind;
89
use rustc_errors::Applicability;
@@ -12,9 +13,9 @@ use rustc_lint::LateContext;
1213
use rustc_middle::ty;
1314
use rustc_span::{Span, Symbol};
1415

15-
use super::MANUAL_IS_VARIANT_AND;
16+
use super::{MANUAL_IS_VARIANT_AND, method_call};
1617

17-
pub(super) fn check(
18+
pub(super) fn check_map_unwrap_or_default(
1819
cx: &LateContext<'_>,
1920
expr: &Expr<'_>,
2021
map_recv: &Expr<'_>,
@@ -217,3 +218,80 @@ pub(super) fn check_map(cx: &LateContext<'_>, expr: &Expr<'_>) {
217218
}
218219
}
219220
}
221+
222+
/// Check for things like `.into_iter().all()`.
223+
pub(super) fn check_iter_reduce(
224+
cx: &LateContext<'_>,
225+
expr: &Expr<'_>,
226+
recv: &Expr<'_>,
227+
arg: &Expr<'_>,
228+
second_call: Span,
229+
msrv: Msrv,
230+
method: Symbol,
231+
) {
232+
// Find how the iterator is created.
233+
let (mut referenced, first_recv, first_call) = match method_call(recv) {
234+
Some((sym::into_iter, first_recv, [], _, first_call)) => (false, first_recv, first_call),
235+
Some((sym::iter, first_recv, [], _, first_call)) => (true, first_recv, first_call),
236+
_ => {
237+
return;
238+
},
239+
};
240+
241+
// Find the method to suggest.
242+
let recv_type = cx.typeck_results().expr_ty(first_recv);
243+
let (replacement_method, since_version) = match recv_type.kind() {
244+
ty::Adt(adt, _) if cx.tcx.is_diagnostic_item(sym::Result, adt.did()) && method == sym::any => {
245+
(sym::is_ok_and, msrvs::OPTION_RESULT_IS_VARIANT_AND)
246+
},
247+
ty::Adt(adt, _) if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) => match method {
248+
sym::all => (sym::is_none_or, msrvs::IS_NONE_OR),
249+
sym::any => (sym::is_some_and, msrvs::OPTION_RESULT_IS_VARIANT_AND),
250+
_ => panic!("The method is checked in the caller."),
251+
},
252+
_ => return,
253+
};
254+
if !msrv.meets(cx, since_version) {
255+
return;
256+
}
257+
258+
// The replacement methods pass the value to the closure. The original
259+
// methods may pass the reference. If possible, the program modifies the
260+
// closure to take the value.
261+
let (replacement_closure, applicability) = if referenced
262+
&& is_copy(cx, recv_type)
263+
&& let Some(DerefClosure {
264+
suggestion,
265+
applicability,
266+
}) = deref_closure_args(cx, arg)
267+
{
268+
referenced = false;
269+
(Some((arg.span, suggestion)), applicability)
270+
} else {
271+
(None, Applicability::MachineApplicable)
272+
};
273+
// If it is not possible to modify the closure, the program suggests the
274+
// `as_ref` method.
275+
let replacement_call = if referenced {
276+
format!("as_ref().{replacement_method}")
277+
} else {
278+
replacement_method.to_string()
279+
};
280+
281+
let mut suggestion = vec![
282+
// Remove the call to get the iterator.
283+
(first_call.with_lo(first_recv.span.hi()), String::new()),
284+
// Replace the call to reduce it.
285+
(second_call, replacement_call),
286+
];
287+
suggestion.extend(replacement_closure);
288+
span_lint_and_then(
289+
cx,
290+
MANUAL_IS_VARIANT_AND,
291+
expr.span,
292+
format!("use of `option::Iter::{method}`"),
293+
|diag| {
294+
diag.multipart_suggestion(format!("use `{replacement_method}` instead"), suggestion, applicability);
295+
},
296+
);
297+
}

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3915,6 +3915,10 @@ declare_clippy_lint! {
39153915
/// result.map(|a| a > 10) == Ok(true);
39163916
/// option.map(|a| a > 10) != Some(true);
39173917
/// result.map(|a| a > 10) != Ok(true);
3918+
///
3919+
/// option.into_iter().any(|a| a > 10);
3920+
/// result.into_iter().any(|a| a > 10);
3921+
/// option.into_iter().all(|a| a > 10);
39183922
/// ```
39193923
/// Use instead:
39203924
/// ```no_run
@@ -3927,6 +3931,10 @@ declare_clippy_lint! {
39273931
/// result.is_ok_and(|a| a > 10);
39283932
/// option.is_none_or(|a| a > 10);
39293933
/// !result.is_ok_and(|a| a > 10);
3934+
///
3935+
/// option.is_some_and(|a| a > 10);
3936+
/// result.is_ok_and(|a| a > 10);
3937+
/// option.is_none_or(|a| a > 10);
39303938
/// ```
39313939
#[clippy::version = "1.77.0"]
39323940
pub MANUAL_IS_VARIANT_AND,
@@ -5025,6 +5033,7 @@ impl Methods {
50255033
zst_offset::check(cx, expr, recv);
50265034
},
50275035
(sym::all, [arg]) => {
5036+
manual_is_variant_and::check_iter_reduce(cx, expr, recv, arg, span, self.msrv, name);
50285037
needless_character_iteration::check(cx, expr, recv, arg, true);
50295038
match method_call(recv) {
50305039
Some((sym::cloned, recv2, [], _, _)) => {
@@ -5054,6 +5063,7 @@ impl Methods {
50545063
}
50555064
},
50565065
(sym::any, [arg]) => {
5066+
manual_is_variant_and::check_iter_reduce(cx, expr, recv, arg, span, self.msrv, name);
50575067
needless_character_iteration::check(cx, expr, recv, arg, false);
50585068
match method_call(recv) {
50595069
Some((sym::cloned, recv2, [], _, _)) => iter_overeager_cloned::check(
@@ -5596,7 +5606,7 @@ impl Methods {
55965606
(sym::unwrap_or_default, []) => {
55975607
match method_call(recv) {
55985608
Some((sym::map, m_recv, [arg], span, _)) => {
5599-
manual_is_variant_and::check(cx, expr, m_recv, arg, span, self.msrv);
5609+
manual_is_variant_and::check_map_unwrap_or_default(cx, expr, m_recv, arg, span, self.msrv);
56005610
},
56015611
Some((then_method @ (sym::then | sym::then_some), t_recv, [t_arg], _, _)) => {
56025612
obfuscated_if_else::check(

‎tests/ui/manual_is_variant_and.fixed‎

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ macro_rules! some_false {
2727
};
2828
}
2929

30+
macro_rules! into_iter {
31+
($x:expr) => {
32+
$x.into_iter()
33+
};
34+
}
35+
3036
macro_rules! mac {
3137
(some $e:expr) => {
3238
Some($e)
@@ -42,8 +48,51 @@ macro_rules! mac {
4248
};
4349
}
4450

51+
#[clippy::msrv = "1.69"]
52+
fn under_msrv_is_some_and(opt: Option<u32>, res: Result<u32, ()>) {
53+
let _ = res.into_iter().any(|x| x != 0);
54+
let _ = opt.map(|x| x != 0).unwrap_or_default();
55+
let _ = opt.iter().all(|x| *x != 0);
56+
let _ = opt.iter().any(|&x| x != 0);
57+
}
58+
59+
#[clippy::msrv = "1.70"]
60+
fn meets_msrv_is_some_and(opt: Option<u32>, res: Result<u32, ()>) {
61+
let _ = res.is_ok_and(|x| x != 0);
62+
//~^ manual_is_variant_and
63+
let _ = opt.is_some_and(|x| x != 0);
64+
//~^ manual_is_variant_and
65+
let _ = opt.iter().all(|x| *x != 0);
66+
let _ = opt.is_some_and(|x| x != 0);
67+
//~^ manual_is_variant_and
68+
}
69+
70+
#[clippy::msrv = "1.81"]
71+
fn under_msrv_is_none_or(opt: Option<u32>, res: Result<u32, ()>) {
72+
let _ = res.is_ok_and(|x| x != 0);
73+
//~^ manual_is_variant_and
74+
let _ = opt.is_some_and(|x| x != 0);
75+
//~^ manual_is_variant_and
76+
let _ = opt.iter().all(|x| *x != 0);
77+
let _ = opt.is_some_and(|x| x != 0);
78+
//~^ manual_is_variant_and
79+
}
80+
81+
#[clippy::msrv = "1.82"]
82+
fn meets_msrv_is_none_or(opt: Option<u32>, res: Result<u32, ()>) {
83+
let _ = res.is_ok_and(|x| x != 0);
84+
//~^ manual_is_variant_and
85+
let _ = opt.is_some_and(|x| x != 0);
86+
//~^ manual_is_variant_and
87+
let _ = opt.is_none_or(|x| x != 0);
88+
//~^ manual_is_variant_and
89+
let _ = opt.is_some_and(|x| x != 0);
90+
//~^ manual_is_variant_and
91+
}
92+
4593
#[rustfmt::skip]
4694
fn option_methods() {
95+
let opt_non_copy: Option<String> = None;
4796
let opt = Some(1);
4897

4998
// Check for `option.map(_).unwrap_or_default()` use.
@@ -67,6 +116,36 @@ fn option_methods() {
67116
//~^ manual_is_variant_and
68117
let _ = Some(2).is_none_or(|x| x % 2 == 0);
69118
//~^ manual_is_variant_and
119+
let _ = opt.is_none_or(|x| x != 0);
120+
//~^ manual_is_variant_and
121+
let _ = (opt).is_none_or(|x| x != 0);
122+
//~^ manual_is_variant_and
123+
let _ = opt.is_some_and(i32::is_negative);
124+
//~^ manual_is_variant_and
125+
let _ = opt.is_some_and(|x| x != 0);
126+
//~^ manual_is_variant_and
127+
128+
// The type does not copy. The linter should suggest `as_ref`.
129+
let _ = (opt_non_copy).as_ref().is_some_and(|x| x.is_empty());
130+
//~^ manual_is_variant_and
131+
let _ = opt_non_copy.as_ref().is_some_and(|x| x.is_empty());
132+
//~^ manual_is_variant_and
133+
let _ = opt_non_copy.as_ref().is_some_and(|x| ".." == *x);
134+
//~^ manual_is_variant_and
135+
// The type copies. The linter should not suggest `as_ref`.
136+
let _ = opt.is_none_or(|x| x != 0);
137+
//~^ manual_is_variant_and
138+
let _ = opt.as_ref().is_none_or(|_| false);
139+
//~^ manual_is_variant_and
140+
let _ = opt.is_some_and(|x| x != 0);
141+
//~^ manual_is_variant_and
142+
let _ = opt.is_none_or(|_| false);
143+
//~^ manual_is_variant_and
144+
// The argument is defined elsewhere. It cannot be changed to take the
145+
// value. The linter should suggest `as_ref`.
146+
let zerop = |x: &i32| *x == 0;
147+
let _ = opt.as_ref().is_none_or(zerop);
148+
//~^ manual_is_variant_and
70149

71150
// won't fix because the return type of the closure is not `bool`
72151
let _ = opt.map(|x| x + 1).unwrap_or_default();
@@ -83,10 +162,13 @@ fn option_methods() {
83162
let _ = mac!(some 2).map(|x| x % 2 == 0) == Some(true);
84163
let _ = mac!(some_map 2) == Some(true);
85164
let _ = mac!(map Some(2)) == Some(true);
165+
// The first method call is elsewhere. The linter cannot change it.
166+
let _ = into_iter!(opt).any(|x| x != 0);
86167
}
87168

88169
#[rustfmt::skip]
89170
fn result_methods() {
171+
let res_non_copy: Result<i32, String> = Ok(1);
90172
let res: Result<i32, ()> = Ok(1);
91173

92174
// multi line cases
@@ -102,14 +184,24 @@ fn result_methods() {
102184
//~^ manual_is_variant_and
103185
let _ = !Ok::<usize, ()>(2).is_ok_and(|x| x.is_multiple_of(2));
104186
//~^ manual_is_variant_and
187+
let _ = res.is_ok_and(i32::is_negative);
188+
//~^ manual_is_variant_and
189+
let _ = res_non_copy.as_ref().is_ok_and(|x| *x != 0);
190+
//~^ manual_is_variant_and
191+
let _ = res.is_ok_and(|x| x != 0);
192+
//~^ manual_is_variant_and
105193

106194
// won't fix because the return type of the closure is not `bool`
107195
let _ = res.map(|x| x + 1).unwrap_or_default();
108196

109197
let res2: Result<char, ()> = Ok('a');
110198
let _ = res2.is_ok_and(char::is_alphanumeric); // should lint
111199
//~^ manual_is_variant_and
112-
let _ = opt_map!(res2, |x| x == 'a').unwrap_or_default(); // should not lint
200+
201+
// should not lint
202+
let _ = opt_map!(res2, |x| x == 'a').unwrap_or_default();
203+
// The result type comes with no shorthand for all.
204+
let _ = res.into_iter().all(i32::is_negative);
113205
}
114206

115207
fn main() {

0 commit comments

Comments
 (0)