Skip to content

Commit f2cc995

Browse files
Y-NakY-Nak
committed
Remove method_calls
1 parent 527fbbe commit f2cc995

File tree

5 files changed

+92
-55
lines changed

5 files changed

+92
-55
lines changed

‎clippy_lints/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1326,7 +1326,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
13261326
LintId::of(&missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS),
13271327
LintId::of(&missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS),
13281328
LintId::of(&modulo_arithmetic::MODULO_ARITHMETIC),
1329-
LintId::of(&needless_for_each::NEEDLESS_FOR_EACH),
13301329
LintId::of(&panic_in_result_fn::PANIC_IN_RESULT_FN),
13311330
LintId::of(&panic_unimplemented::PANIC),
13321331
LintId::of(&panic_unimplemented::TODO),
@@ -1409,6 +1408,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
14091408
LintId::of(&misc_early::UNSEPARATED_LITERAL_SUFFIX),
14101409
LintId::of(&mut_mut::MUT_MUT),
14111410
LintId::of(&needless_continue::NEEDLESS_CONTINUE),
1411+
LintId::of(&needless_for_each::NEEDLESS_FOR_EACH),
14121412
LintId::of(&needless_pass_by_value::NEEDLESS_PASS_BY_VALUE),
14131413
LintId::of(&non_expressive_names::SIMILAR_NAMES),
14141414
LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE),

‎clippy_lints/src/needless_for_each.rs‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,7 @@ use rustc_span::{source_map::Span, sym, Symbol};
1010

1111
use if_chain::if_chain;
1212

13-
use crate::utils::{
14-
has_iter_method, is_diagnostic_assoc_item, method_calls, snippet_with_applicability, span_lint_and_then,
15-
};
13+
use crate::utils::{has_iter_method, is_trait_method, snippet_with_applicability, span_lint_and_then};
1614

1715
declare_clippy_lint! {
1816
/// **What it does:** Checks for usage of `for_each` that would be more simply written as a
@@ -41,7 +39,7 @@ declare_clippy_lint! {
4139
/// }
4240
/// ```
4341
pub NEEDLESS_FOR_EACH,
44-
restriction,
42+
pedantic,
4543
"using `for_each` where a `for` loop would be simpler"
4644
}
4745

@@ -55,22 +53,28 @@ impl LateLintPass<'_> for NeedlessForEach {
5553
_ => return,
5654
};
5755

58-
// Max depth is set to 3 because we need to check the method chain length is just two.
59-
let (method_names, arg_lists, _) = method_calls(expr, 3);
60-
6156
if_chain! {
62-
// This assures the length of this method chain is two.
63-
if let [for_each_args, iter_args] = arg_lists.as_slice();
64-
if let Some(for_each_sym) = method_names.first();
65-
if *for_each_sym == Symbol::intern("for_each");
66-
if let Some(did) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
67-
if is_diagnostic_assoc_item(cx, did, sym::Iterator);
68-
// Checks the type of the first method receiver is NOT a user defined type.
69-
if has_iter_method(cx, cx.typeck_results().expr_ty(&iter_args[0])).is_some();
70-
if let ExprKind::Closure(_, _, body_id, ..) = for_each_args[1].kind;
71-
let body = cx.tcx.hir().body(body_id);
57+
// Check the method name is `for_each`.
58+
if let ExprKind::MethodCall(method_name, _, for_each_args, _) = expr.kind;
59+
if method_name.ident.name == Symbol::intern("for_each");
60+
// Check `for_each` is an associated function of `Iterator`.
61+
if is_trait_method(cx, expr, sym::Iterator);
62+
// Checks the receiver of `for_each` is also a method call.
63+
if let Some(for_each_receiver) = for_each_args.get(0);
64+
if let ExprKind::MethodCall(_, _, iter_args, _) = for_each_receiver.kind;
65+
// Skip the lint if the call chain is too long. e.g. `v.field.iter().for_each()` or
66+
// `v.foo().iter().for_each()` must be skipped.
67+
if let Some(iter_receiver) = iter_args.get(0);
68+
if matches!(
69+
iter_receiver.kind,
70+
ExprKind::Array(..) | ExprKind::Call(..) | ExprKind::Path(..)
71+
);
72+
// Checks the type of the `iter` method receiver is NOT a user defined type.
73+
if has_iter_method(cx, cx.typeck_results().expr_ty(&iter_receiver)).is_some();
7274
// Skip the lint if the body is not block because this is simpler than `for` loop.
7375
// e.g. `v.iter().for_each(f)` is simpler and clearer than using `for` loop.
76+
if let ExprKind::Closure(_, _, body_id, ..) = for_each_args[1].kind;
77+
let body = cx.tcx.hir().body(body_id);
7478
if let ExprKind::Block(..) = body.value.kind;
7579
then {
7680
let mut ret_collector = RetCollector::default();
@@ -99,7 +103,7 @@ impl LateLintPass<'_> for NeedlessForEach {
99103
)));
100104

101105
for span in &ret_collector.spans {
102-
suggs.push((*span, "return".to_string()));
106+
suggs.push((*span, "continue".to_string()));
103107
}
104108

105109
span_lint_and_then(

‎tests/ui/needless_for_each_fixable.fixed‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ fn should_lint() {
1414
acc += elem;
1515
}
1616

17+
for elem in [1, 2, 3].iter() {
18+
acc += elem;
19+
}
20+
1721
let mut hash_map: HashMap<i32, i32> = HashMap::new();
1822
for (k, v) in hash_map.iter() {
1923
acc += k + v;
@@ -46,11 +50,30 @@ fn should_not_lint() {
4650
}
4751
v.iter().for_each(print);
4852

53+
// User defined type.
54+
struct MyStruct {
55+
v: Vec<i32>,
56+
}
57+
impl MyStruct {
58+
fn iter(&self) -> impl Iterator<Item = &i32> {
59+
self.v.iter()
60+
}
61+
}
62+
let s = MyStruct { v: Vec::new() };
63+
s.iter().for_each(|elem| {
64+
acc += elem;
65+
});
66+
4967
// `for_each` follows long iterator chain.
50-
v.iter().chain(v.iter()).for_each(|v| println!("{}", v));
68+
v.iter().chain(v.iter()).for_each(|v| {
69+
acc += v;
70+
});
5171
v.as_slice().iter().for_each(|v| {
5272
acc += v;
5373
});
74+
s.v.iter().for_each(|v| {
75+
acc += v;
76+
});
5477

5578
// `return` is used in `Loop` of the closure.
5679
v.iter().for_each(|v| {
@@ -68,20 +91,6 @@ fn should_not_lint() {
6891
}
6992
});
7093

71-
// User defined type.
72-
struct MyStruct {
73-
v: Vec<i32>,
74-
}
75-
impl MyStruct {
76-
fn iter(&self) -> impl Iterator<Item = &i32> {
77-
self.v.iter()
78-
}
79-
}
80-
let s = MyStruct { v: Vec::new() };
81-
s.iter().for_each(|elem| {
82-
acc += elem;
83-
});
84-
8594
// Previously transformed iterator variable.
8695
let it = v.iter();
8796
it.chain(v.iter()).for_each(|elem| {

‎tests/ui/needless_for_each_fixable.rs‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ fn should_lint() {
1414
acc += elem;
1515
});
1616

17+
[1, 2, 3].iter().for_each(|elem| {
18+
acc += elem;
19+
});
20+
1721
let mut hash_map: HashMap<i32, i32> = HashMap::new();
1822
hash_map.iter().for_each(|(k, v)| {
1923
acc += k + v;
@@ -46,11 +50,30 @@ fn should_not_lint() {
4650
}
4751
v.iter().for_each(print);
4852

53+
// User defined type.
54+
struct MyStruct {
55+
v: Vec<i32>,
56+
}
57+
impl MyStruct {
58+
fn iter(&self) -> impl Iterator<Item = &i32> {
59+
self.v.iter()
60+
}
61+
}
62+
let s = MyStruct { v: Vec::new() };
63+
s.iter().for_each(|elem| {
64+
acc += elem;
65+
});
66+
4967
// `for_each` follows long iterator chain.
50-
v.iter().chain(v.iter()).for_each(|v| println!("{}", v));
68+
v.iter().chain(v.iter()).for_each(|v| {
69+
acc += v;
70+
});
5171
v.as_slice().iter().for_each(|v| {
5272
acc += v;
5373
});
74+
s.v.iter().for_each(|v| {
75+
acc += v;
76+
});
5477

5578
// `return` is used in `Loop` of the closure.
5679
v.iter().for_each(|v| {
@@ -68,20 +91,6 @@ fn should_not_lint() {
6891
}
6992
});
7093

71-
// User defined type.
72-
struct MyStruct {
73-
v: Vec<i32>,
74-
}
75-
impl MyStruct {
76-
fn iter(&self) -> impl Iterator<Item = &i32> {
77-
self.v.iter()
78-
}
79-
}
80-
let s = MyStruct { v: Vec::new() };
81-
s.iter().for_each(|elem| {
82-
acc += elem;
83-
});
84-
8594
// Previously transformed iterator variable.
8695
let it = v.iter();
8796
it.chain(v.iter()).for_each(|elem| {

‎tests/ui/needless_for_each_fixable.stderr‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,22 @@ LL | }
3030
|
3131

3232
error: needless use of `for_each`
33-
--> $DIR/needless_for_each_fixable.rs:18:5
33+
--> $DIR/needless_for_each_fixable.rs:17:5
34+
|
35+
LL | / [1, 2, 3].iter().for_each(|elem| {
36+
LL | | acc += elem;
37+
LL | | });
38+
| |_______^
39+
|
40+
help: try
41+
|
42+
LL | for elem in [1, 2, 3].iter() {
43+
LL | acc += elem;
44+
LL | }
45+
|
46+
47+
error: needless use of `for_each`
48+
--> $DIR/needless_for_each_fixable.rs:22:5
3449
|
3550
LL | / hash_map.iter().for_each(|(k, v)| {
3651
LL | | acc += k + v;
@@ -45,7 +60,7 @@ LL | }
4560
|
4661

4762
error: needless use of `for_each`
48-
--> $DIR/needless_for_each_fixable.rs:21:5
63+
--> $DIR/needless_for_each_fixable.rs:25:5
4964
|
5065
LL | / hash_map.iter_mut().for_each(|(k, v)| {
5166
LL | | acc += *k + *v;
@@ -60,7 +75,7 @@ LL | }
6075
|
6176

6277
error: needless use of `for_each`
63-
--> $DIR/needless_for_each_fixable.rs:24:5
78+
--> $DIR/needless_for_each_fixable.rs:28:5
6479
|
6580
LL | / hash_map.keys().for_each(|k| {
6681
LL | | acc += k;
@@ -75,7 +90,7 @@ LL | }
7590
|
7691

7792
error: needless use of `for_each`
78-
--> $DIR/needless_for_each_fixable.rs:27:5
93+
--> $DIR/needless_for_each_fixable.rs:31:5
7994
|
8095
LL | / hash_map.values().for_each(|v| {
8196
LL | | acc += v;
@@ -90,7 +105,7 @@ LL | }
90105
|
91106

92107
error: needless use of `for_each`
93-
--> $DIR/needless_for_each_fixable.rs:34:5
108+
--> $DIR/needless_for_each_fixable.rs:38:5
94109
|
95110
LL | / my_vec().iter().for_each(|elem| {
96111
LL | | acc += elem;
@@ -104,5 +119,5 @@ LL | acc += elem;
104119
LL | }
105120
|
106121

107-
error: aborting due to 7 previous errors
122+
error: aborting due to 8 previous errors
108123

0 commit comments

Comments
 (0)