-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Handle mapping to Option in map_flatten
lint
#5846
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (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. |
r? @flip1995 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is
map_flatten
lint intentionally inpedantic
category, or could it be moved tocomplexity
?
Yes this is intentional.
- I would like to change suggestion range to cover only
.map(...).flatten()
You can get the spans of all methods involved like here:
rust-clippy/clippy_lints/src/methods/mod.rs
Line 1415 in 2e0f8b6
["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0], method_spans[0]), |
rust-clippy/clippy_lints/src/methods/mod.rs
Lines 2200 to 2203 in 2e0f8b6
span_lint_and_sugg( | |
cx, | |
UNNECESSARY_FOLD, | |
fold_span.with_hi(expr.span.hi()), |
- Currently I use return type of closure inside
map
, but probably it is not good way
I think this is indeed the best way, let's add some more tests for it.
@flip1995 Thank you for review! I update code. I am not sure why CI is not passed, couldn't find anything related to my changes in logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why CI failed. it also passes locally on my end. 🤔
clippy_lints/src/methods/mod.rs
Outdated
span_lint_and_sugg( | ||
cx, | ||
MAP_FLATTEN, | ||
expr.span, | ||
expr.span.trim_start(map_args[0].span).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer, if you use map_args[0].span.to_hi(expr.span.hi())
. That way, no unwrap
is necessary.
clippy_lints/src/methods/mod.rs
Outdated
// `(...).map(...)` has type `impl Iterator<Item=impl Iterator<...>> | ||
"flat_map" | ||
}; | ||
let msg = "called `map(..).flatten()` on an `Iterator`"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binding this isn't necessary anymore.
@bors r+ rollup=always |
📌 Commit d4ba561 has been approved by |
…flip1995 Handle mapping to Option in `map_flatten` lint Fixes rust-lang#4496 The existing [`map_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten) lint suggests changing `expr.map(...).flatten()` to `expr.flat_map(...)` when `expr` is `Iterator`. This PR changes suggestion to `filter_map` instead of `flat_map` when mapping to `Option`, because it is more natural Also here are some questions: * If expression has type which implements `Iterator` trait (`match_trait_method(cx, expr, &paths::ITERATOR) == true`), how can I get type of iterator elements? Currently I use return type of closure inside `map`, but probably it is not good way * I would like to change suggestion range to cover only `.map(...).flatten()`, that is from: ``` let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map ``` to ``` let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `.flat_map(|x| 0..x)` ``` Is it ok? * Is `map_flatten` lint intentionally in `pedantic` category, or could it be moved to `complexity`? changelog: Handle mapping to Option in [`map_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten) lint
Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup
Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup
Rollup of 5 pull requests Successful merges: - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost
Rollup of 5 pull requests Successful merges: - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup
Fixes #4496
The existing
map_flatten
lint suggests changingexpr.map(...).flatten()
toexpr.flat_map(...)
whenexpr
isIterator
. This PR changes suggestion tofilter_map
instead offlat_map
when mapping toOption
, because it is more naturalAlso here are some questions:
Iterator
trait (match_trait_method(cx, expr, &paths::ITERATOR) == true
), how can I get type of iterator elements? Currently I use return type of closure insidemap
, but probably it is not good way.map(...).flatten()
, that is from:to
Is it ok?
map_flatten
lint intentionally inpedantic
category, or could it be moved tocomplexity
?changelog: Handle mapping to Option in
map_flatten
lint