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

Suggest removing filter_map for Iterator::filter_map(|x| Some(x)) #12556

Open
m-rph opened this issue Mar 25, 2024 · 9 comments · May be fixed by #12766
Open

Suggest removing filter_map for Iterator::filter_map(|x| Some(x)) #12556

m-rph opened this issue Mar 25, 2024 · 9 comments · May be fixed by #12766
Assignees
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy

Comments

@m-rph
Copy link
Contributor

m-rph commented Mar 25, 2024

Summary

When we encounter a filter_map on what is effectively an identity function followed by Some, we shouldn't recommend map(identity), but we should instead recommend completely removing filter_map.

Many thanks to @Centri3 for finding this and #12501 .

Reproducer

I tried this code:

fn main() {
    let _= vec![Some(10), None].into_iter().filter_map(|x| Some(x));
}

What I saw:

    Checking playground v0.0.1 (/playground)
warning: this `.filter_map` can be written more simply using `.map`
 --> src/main.rs:4:12
  |
4 |     let _= vec![Some(10), None].into_iter().filter_map(|x| Some(x));
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
  = note: `#[warn(clippy::unnecessary_filter_map)]` on by default

warning: redundant closure
 --> src/main.rs:4:56
  |
4 |     let _= vec![Some(10), None].into_iter().filter_map(|x| Some(x));
  |                                                        ^^^^^^^^^^^ help: replace the closure with the function itself: `Some`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
  = note: `#[warn(clippy::redundant_closure)]` on by default

warning: `playground` (bin "playground") generated 2 warnings (run `cargo clippy --fix --bin "playground"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.26s

What I expected to see:

    Checking playground v0.0.1 (/playground)
warning: this `.filter_map` for `|x| Some(x)` can be more succinctly written as
 --> src/main.rs:4:12
  |
4 |     let _= vec![Some(10), None].into_iter();
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
  = note: `#[warn(clippy::unnecessary_filter_map)]` on by default

This is a follow up from #12501's discussion.

Version

Latest Nightly on playground

Additional Labels

No response

@m-rph m-rph added the C-bug Category: Clippy is not doing the correct thing label Mar 25, 2024
@m-rph m-rph changed the title Clippy should suggest removing filter_map for Iter::filter_map(|x| Some(x)) Clippy should suggest removing filter_map for ::filter_map(|x| Some(x)) Mar 25, 2024
@Centri3
Copy link
Member

Centri3 commented Mar 25, 2024

PS - The original idea was for removing the Some instead of changig filter_map -> map if it's Option<Option<T>> (including references), but this is a good addition too :3

@m-rph
Copy link
Contributor Author

m-rph commented Mar 25, 2024

Is that machine applicable? I was thinking that removing Some for Option<Option<_>> means that it can't be followed by a map with closure Option<_> -> T, so we can't replace it.

I think it's the same rationale as to why iter_filter_is_some is not machine applicable.

@m-rph
Copy link
Contributor Author

m-rph commented Mar 25, 2024

I don't think this is a clippy bug, but more like an enhancement?

@Centri3
Copy link
Member

Centri3 commented Mar 25, 2024

It isn't machine applicable, yes

@Centri3 Centri3 added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages and removed C-bug Category: Clippy is not doing the correct thing labels Mar 25, 2024
@m-rph
Copy link
Contributor Author

m-rph commented Mar 25, 2024

@rustbot claim

Assigning this to me as I am working on the sibling issue.

@m-rph m-rph changed the title Clippy should suggest removing filter_map for ::filter_map(|x| Some(x)) Suggest removing filter_map for ::filter_map(|x| Some(x)) Mar 26, 2024
@m-rph m-rph changed the title Suggest removing filter_map for ::filter_map(|x| Some(x)) Suggest removing filter_map for Iterator::filter_map(|x| Some(x)) Mar 27, 2024
@m-rph
Copy link
Contributor Author

m-rph commented Mar 29, 2024

This should be under unnecessary_filter_map, and should not be treated as a special case of filter_map_identity.

This should be straightforward; Introduce a check in unnecessary_filter_map to see whether the closure is |x| Some(x) or simply a path to Some.

@m-rph m-rph added the good-first-issue These issues are a good way to get started with Clippy label Mar 29, 2024
@m-rph m-rph removed their assignment Mar 29, 2024
@omer-shtivi
Copy link

@rustbot claim

@belyakov-am
Copy link

@omer-shtivi are you planning on opening PR for this issue? I'm willing to take over this issue if you don't have time to do this/changed you mind

@omer-shtivi
Copy link

Hi @belyakov-am I'm still planning to do it, but it will just take me some time as I'm learning how to contribute to clippy

@omer-shtivi omer-shtivi linked a pull request May 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants