Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: inspect-then-for_each on iterators should be simplified #5209

Closed
scottmcm opened this issue Feb 20, 2020 · 6 comments
Closed

New lint: inspect-then-for_each on iterators should be simplified #5209

scottmcm opened this issue Feb 20, 2020 · 6 comments
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@scottmcm
Copy link
Member

Inspired by the change made in https://github.com/rust-lang/rust/pull/69022/files#diff-d1ff3aad2daeb77432493d91c41b722bL147

When an inspect is followed by for_each, that's the same as just doing the thing in the inspect at the beginning of the closure in the for_each.

It's possible that this should only be suggested when a brace-bodied closure is already being passed to for_each, like

    .inspect(|x| do_something(x, 2))
    .for_each(|x| {
        do_more(x);
        update_something(x);
    });

which can just be

    .for_each(|x| {
        do_something(x, 2);
        do_more(x);
        update_something(x);
    });

But not for something like .inspect(foo).for_each(bar), where it's not obvious that the closure is better. (It's not bad, but not an obvious improvement.)

@flip1995 flip1995 added L-complexity Lint: Belongs in the complexity lint group L-suggestion Lint: Improving, adding or fixing lint suggestions good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints labels Feb 21, 2020
@ebroto
Copy link
Member

ebroto commented Oct 8, 2020

Sure, go ahead!

@nahuakang
Copy link
Contributor

nahuakang commented Jan 5, 2021

Looking for a new issue. If the original volunteer isn't working on this, I'd like to take a look at this issue this weekend :) @ebroto @flip1995

@flip1995
Copy link
Member

flip1995 commented Jan 5, 2021

Go ahead! :)

@nahuakang
Copy link
Contributor

nahuakang commented Jan 5, 2021

@flip1995 Off your mind, do you remember a lint with similar logic that I can study a bit? 🙏

(Update: Started researching on Jan 10th, 2021).

@flip1995
Copy link
Member

flip1995 commented Jan 5, 2021

Take a look at the methods module. There is also the infrastructure already implemented, you'll need to implement this lint.

bors added a commit that referenced this issue Jan 19, 2021
New Lint: inspect_then_for_each

**Work In Progress**

This PR addresses [Issue 5209](#5209) and adds a new lint called `inspect_then_for_each`.

Current seek some guidance.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

---

changelog: Add [`inspect_for_each`] lint for the use of `inspect().for_each()` on `Iterators`.
@joao-conde
Copy link

Was this not closed by #6577 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

5 participants