New Lint: needless_for_each #6706

Y-Nak · 2021-02-09T12:00:29Z

resolves: #6543

changelog: Added pedantic lint: needless_for_each

rust-highfive · 2021-02-09T12:00:32Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

clippy_lints/src/methods/mod.rs

clippy_lints/src/methods/excessive_for_each.rs

Y-Nak · 2021-02-09T14:48:44Z

Changes:

Change a category from style to restriction.
Remove a redundant condition: if !match_trait_method(cx, for_each_receiver, &paths::ITERATOR);.

tests/ui/excessive_for_each.rs

Y-Nak · 2021-02-09T15:55:55Z

Changes:

Trigger the lint if closure's body is ExprKind::Block.

camsteffen · 2021-02-09T16:10:56Z

I'm a little iffy on the return within inner loop case (should not lint). There's an argument to be made that it is better style to avoid multi-level control flow (continue 'label) if possible.

Y-Nak · 2021-02-09T16:22:31Z

Yes, I think so too... if for_each is used to avoid multi-level control flow, it should be acceptable.
@llogiq what do you think about this?

bors · 2021-02-10T14:07:43Z

☔ The latest upstream changes (presumably #6695) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2021-02-10T19:32:31Z

I completely agree. Requiring adding a label is likely bad style, and I've seen compiler code introducing a closure that is directly called to avoid labels.

Y-Nak · 2021-02-11T03:54:05Z

Changes:

Skip the lint if return is used in Loop.
Resolve conflicts.

flip1995

impl LGTM, but code duplication can be reduced.

clippy_lints/src/methods/excessive_for_each.rs

Y-Nak · 2021-02-22T12:47:17Z

Changes:

Rewrite is_target_ty to utils::has_iter_method.
Format codes inside then block of if_chain
Remove unnecessary format usage.

Y-Nak · 2021-02-23T12:56:24Z

Changes: Add comments to clarify why RetCollector is needed.

Y-Nak · 2021-02-25T14:18:05Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

clippy_lints/src/methods/mod.rs

bors · 2021-02-27T01:52:11Z

☔ The latest upstream changes (presumably #6730) made this pull request unmergeable. Please resolve the merge conflicts.

Y-Nak · 2021-02-27T04:59:50Z

Changes:

Improve document (including typo fix).
Change to use .. as the default value of snippet.
Resolve conflicts.

camsteffen

Filling in for @flip1995 with a review!

Need to remove the empty file clippy_lints/src/iter_for_each.rs.

clippy_lints/src/methods/excessive_for_each.rs

clippy_lints/src/methods/mod.rs

clippy_lints/src/methods/excessive_for_each.rs

clippy_lints/src/methods/mod.rs

tests/ui/excessive_for_each.rs

clippy_lints/src/methods/excessive_for_each.rs

Y-Nak · 2021-03-10T14:33:38Z

@camsteffen Thanks for your review! Reviewed points make perfect sense to me, I'll fix them.

giraffate · 2021-03-29T13:59:48Z

ping from triage @flip1995 @camsteffen. Can we need to review this further?

flip1995 · 2021-03-31T14:42:05Z

Thanks! I didn't follow this PR, so I just

@bors r=camsteffen

Skimming over it, I couldn't find any obvious flaws, so I trust your review.

bors · 2021-03-31T14:49:37Z

📌 Commit 0e42112 has been approved by camsteffen

New Lint: needless_for_each resolves: #6543 changelog: Added pedantic lint: `needless_for_each`

bors · 2021-03-31T14:54:47Z

⌛ Testing commit 0e42112 with merge a743553...

bors · 2021-03-31T15:02:37Z

💔 Test failed - checks-action_test

flip1995 · 2021-03-31T15:06:08Z

The CI failures are because crate::utils::* are clippy_utils::* now.

Y-Nak · 2021-03-31T15:10:39Z

Fixed!

flip1995 · 2021-03-31T15:57:30Z

@bors r=camsteffen

Thanks!

bors · 2021-03-31T15:58:34Z

📌 Commit e61f978 has been approved by camsteffen

bors · 2021-03-31T15:59:59Z

⌛ Testing commit e61f978 with merge c1021b8...

bors · 2021-03-31T16:19:04Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing c1021b8 to master...

bors · 2021-03-31T16:19:09Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing c1021b8 to master...

rust-highfive assigned flip1995 Feb 9, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 9, 2021

Y-Nak force-pushed the excessive-for-each branch from 0cbec62 to 7acb8f8 Compare February 9, 2021 12:20

matthiaskrgr reviewed Feb 9, 2021

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

Y-Nak commented Feb 9, 2021

View reviewed changes

clippy_lints/src/methods/excessive_for_each.rs Outdated Show resolved Hide resolved

camsteffen reviewed Feb 9, 2021

View reviewed changes

tests/ui/excessive_for_each.rs Outdated Show resolved Hide resolved

Y-Nak force-pushed the excessive-for-each branch from 8f50346 to ffcd033 Compare February 11, 2021 03:51

flip1995 reviewed Feb 21, 2021

View reviewed changes

clippy_lints/src/methods/excessive_for_each.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/excessive_for_each.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/excessive_for_each.rs Outdated Show resolved Hide resolved

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 21, 2021

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 25, 2021

matthiaskrgr reviewed Feb 26, 2021

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

Y-Nak force-pushed the excessive-for-each branch 2 times, most recently from 3bdbe72 to dbadac7 Compare February 27, 2021 04:58

camsteffen suggested changes Mar 9, 2021

View reviewed changes

camsteffen approved these changes Mar 29, 2021

View reviewed changes

bors added a commit that referenced this pull request Mar 31, 2021

Auto merge of #6706 - Y-Nak:excessive-for-each, r=camsteffen

a743553

New Lint: needless_for_each resolves: #6543 changelog: Added pedantic lint: `needless_for_each`

Y-Nak added 13 commits April 1, 2021 00:05

New Lint: excessive_for_each

5029dc8

Fix codes that fails dogfood

3095253

Change a category of excessive_for_each: Style -> Restriction

54a0471

Trigger the lint iff exposure's body is ExprKind::Block.

5bb0f16

Avoid to suggest using label

90cbbb2

Refactor: Remove duplicated codes from excessive_for_each

ccd7a60

Add comments to clarify why RetCollector is needed

25d8b94

Improve the document of excessive_for_each

3843171

Use ".." as default value of snippet in excessive_for_each

5543c34

Refactor excessive_for_each

527fbbe

Remove method_calls

f2cc995

Fix codes that make dogfood fail

1109dc8

Skip needless_for_each if an input stmt is local

bf1e3f7

Tweak a suggestion message of needless_for_each

e61f978

Y-Nak force-pushed the excessive-for-each branch from 0e42112 to e61f978 Compare March 31, 2021 15:10

bors merged commit c1021b8 into rust-lang:master Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Lint: needless_for_each #6706

New Lint: needless_for_each #6706

Y-Nak commented Feb 9, 2021 •

edited

Loading

rust-highfive commented Feb 9, 2021

Y-Nak commented Feb 9, 2021

Y-Nak commented Feb 9, 2021 •

edited

Loading

camsteffen commented Feb 9, 2021

Y-Nak commented Feb 9, 2021

bors commented Feb 10, 2021

llogiq commented Feb 10, 2021

Y-Nak commented Feb 11, 2021 •

edited

Loading

flip1995 left a comment

Y-Nak commented Feb 22, 2021

Y-Nak commented Feb 23, 2021

Y-Nak commented Feb 25, 2021

bors commented Feb 27, 2021

Y-Nak commented Feb 27, 2021

camsteffen left a comment

Y-Nak commented Mar 10, 2021

giraffate commented Mar 29, 2021

flip1995 commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

flip1995 commented Mar 31, 2021 •

edited

Loading

Y-Nak commented Mar 31, 2021

flip1995 commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

New Lint: needless_for_each #6706

New Lint: needless_for_each #6706

Conversation

Y-Nak commented Feb 9, 2021 • edited Loading

rust-highfive commented Feb 9, 2021

Y-Nak commented Feb 9, 2021

Y-Nak commented Feb 9, 2021 • edited Loading

camsteffen commented Feb 9, 2021

Y-Nak commented Feb 9, 2021

bors commented Feb 10, 2021

llogiq commented Feb 10, 2021

Y-Nak commented Feb 11, 2021 • edited Loading

flip1995 left a comment

Choose a reason for hiding this comment

Y-Nak commented Feb 22, 2021

Y-Nak commented Feb 23, 2021

Y-Nak commented Feb 25, 2021

bors commented Feb 27, 2021

Y-Nak commented Feb 27, 2021

camsteffen left a comment

Choose a reason for hiding this comment

Y-Nak commented Mar 10, 2021

giraffate commented Mar 29, 2021

flip1995 commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

flip1995 commented Mar 31, 2021 • edited Loading

Y-Nak commented Mar 31, 2021

flip1995 commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

bors commented Mar 31, 2021

Y-Nak commented Feb 9, 2021 •

edited

Loading

Y-Nak commented Feb 9, 2021 •

edited

Loading

Y-Nak commented Feb 11, 2021 •

edited

Loading

flip1995 commented Mar 31, 2021 •

edited

Loading