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

Make find_map lint be more conservative #4193

Closed
yaahc opened this issue Jun 10, 2019 · 3 comments · Fixed by #6591
Closed

Make find_map lint be more conservative #4193

yaahc opened this issue Jun 10, 2019 · 3 comments · Fixed by #6591
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages

Comments

@yaahc
Copy link
Member

yaahc commented Jun 10, 2019

The find_map lint will trigger if you do a filter followed by a map but its not always preferable.

---- dogfood stdout ----
status: exit code: 101
stdout:
stderr:     Blocking waiting for file lock on package cache lock
    Checking clippy v0.0.212 (/home/jlusby/git/rust/clippy)
error: called `find(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
   --> src/driver.rs:129:9
    |
129 | /         LINT_LEVELS
130 | |             .iter()
131 | |             .find(|level_mapping| level_mapping.0 == lint.group)
132 | |             .map(|(_, level)| level)
    | |____________________________________^
    |
    = note: `-D clippy::find-map` implied by `-D clippy::pedantic`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#find_map

error: called `filter(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.filter_map(..)` instead.
   --> src/driver.rs:205:24
    |
205 |               let desc = lints
    |  ________________________^
206 | |                 .iter()
207 | |                 .filter(|&lint| lint.group == group)
208 | |                 .map(|lint| lint.name)
    | |______________________________________^
    |
    = note: `-D clippy::filter-map` implied by `-D clippy::pedantic`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#filter_map

error: called `find(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
   --> src/driver.rs:129:9
    |
129 | /         LINT_LEVELS
130 | |             .iter()
131 | |             .find(|level_mapping| level_mapping.0 == lint.group)
132 | |             .map(|(_, level)| level)
    | |____________________________________^
    |
    = note: `-D clippy::find-map` implied by `-D clippy::pedantic`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#find_map

error: called `filter(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.filter_map(..)` instead.
   --> src/driver.rs:205:24
    |
205 |               let desc = lints
    |  ________________________^
206 | |                 .iter()
207 | |                 .filter(|&lint| lint.group == group)
208 | |                 .map(|lint| lint.name)
    | |______________________________________^
    |
    = note: `-D clippy::filter-map` implied by `-D clippy::pedantic`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#filter_map

error: aborting due to 2 previous errors

In the case of the first lint at least, you'll run into lifetime issues if you try to replace lines 131 and 132 with a find_map call.

        LINT_LEVELS
            .iter()
            .find_map(|level_mapping| {
                if level_mapping.0 == lint.group {
                    Some(level_mapping.1)
                } else {
                    None
                }
            })
            .map(|level| match level {
                Level::Allow => "allow",
                Level::Warn => "warn",
                Level::Deny => "deny",
            })
            .unwrap()
master ✗ $ cargo check
    Checking clippy v0.0.212 (/home/jlusby/git/rust/clippy)
error[E0507]: cannot move out of `level_mapping.1` which is behind a shared reference
   --> src/driver.rs:133:26
    |
133 |                     Some(level_mapping.1)
    |                          ^^^^^^^^^^^^^^^ move occurs because `level_mapping.1` has type `lintlist::lint::Level`, which does not implement the `Copy` trait

error: aborting due to previous error

For more information about this error, try `rustc --explain E0507`.
error: Could not compile `clippy`.

To learn more, run the command again with --verbose.

And even if it didn't run into the borrow error I personally feel like the find_map version is less concise than the filter -> map version.

If the Item type of the iterator was already a Try type then I can see it being easy enough to write a concise find_map impl, but with the boolean as the only starting point for the closure outputting Some or None I don't think it works as nicely as filter and map.

@yaahc
Copy link
Member Author

yaahc commented Jun 10, 2019

clippy::filter_map might also need the same treatment.

@ghost
Copy link

ghost commented Jun 11, 2019

I opened issue #3188 for filter_map a while ago.

@flip1995 flip1995 added the C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages label Jun 11, 2019
@jakubadamw
Copy link
Contributor

jakubadamw commented Aug 24, 2019

Personally, I think this lint is not useful and should be removed. The find_map() combinator in place of the combination of find() and map() makes the code more succinct only in a certain class of cases. Clippy is not in a position to distinguish them from the majority, where using the combinator is not desired, and so I am not sure what making the lint more conservative would in fact entail.

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
Projects
None yet
3 participants