Skip to content

Commit

Permalink
Rollup merge of rust-lang#5846 - dima74:map_flatten.map_to_option, r=…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
flip1995 authored Aug 4, 2020
2 parents ed967e6 + d4ba561 commit 617dc78
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 21 deletions.
40 changes: 27 additions & 13 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2584,34 +2584,48 @@ fn lint_ok_expect(cx: &LateContext<'_>, expr: &hir::Expr<'_>, ok_args: &[hir::Ex
fn lint_map_flatten<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, map_args: &'tcx [hir::Expr<'_>]) {
// lint if caller of `.map().flatten()` is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {
let msg = "called `map(..).flatten()` on an `Iterator`. \
This is more succinctly expressed by calling `.flat_map(..)`";
let self_snippet = snippet(cx, map_args[0].span, "..");
let map_closure_ty = cx.typeck_results().expr_ty(&map_args[1]);
let is_map_to_option = match map_closure_ty.kind {
ty::Closure(_, _) | ty::FnDef(_, _) | ty::FnPtr(_) => {
let map_closure_sig = match map_closure_ty.kind {
ty::Closure(_, substs) => substs.as_closure().sig(),
_ => map_closure_ty.fn_sig(cx.tcx),
};
let map_closure_return_ty = cx.tcx.erase_late_bound_regions(&map_closure_sig.output());
is_type_diagnostic_item(cx, map_closure_return_ty, sym!(option_type))
},
_ => false,
};

let method_to_use = if is_map_to_option {
// `(...).map(...)` has type `impl Iterator<Item=Option<...>>
"filter_map"
} else {
// `(...).map(...)` has type `impl Iterator<Item=impl Iterator<...>>
"flat_map"
};
let func_snippet = snippet(cx, map_args[1].span, "..");
let hint = format!("{0}.flat_map({1})", self_snippet, func_snippet);
let hint = format!(".{0}({1})", method_to_use, func_snippet);
span_lint_and_sugg(
cx,
MAP_FLATTEN,
expr.span,
msg,
"try using `flat_map` instead",
expr.span.with_lo(map_args[0].span.hi()),
"called `map(..).flatten()` on an `Iterator`",
&format!("try using `{}` instead", method_to_use),
hint,
Applicability::MachineApplicable,
);
}

// lint if caller of `.map().flatten()` is an Option
if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&map_args[0]), sym!(option_type)) {
let msg = "called `map(..).flatten()` on an `Option`. \
This is more succinctly expressed by calling `.and_then(..)`";
let self_snippet = snippet(cx, map_args[0].span, "..");
let func_snippet = snippet(cx, map_args[1].span, "..");
let hint = format!("{0}.and_then({1})", self_snippet, func_snippet);
let hint = format!(".and_then({})", func_snippet);
span_lint_and_sugg(
cx,
MAP_FLATTEN,
expr.span,
msg,
expr.span.with_lo(map_args[0].span.hi()),
"called `map(..).flatten()` on an `Option`",
"try using `and_then` instead",
hint,
Applicability::MachineApplicable,
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/map_flatten.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,20 @@
#![allow(clippy::map_identity)]

fn main() {
// mapping to Option on Iterator
fn option_id(x: i8) -> Option<i8> {
Some(x)
}
let option_id_ref: fn(i8) -> Option<i8> = option_id;
let option_id_closure = |x| Some(x);
let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id).collect();
let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id_ref).collect();
let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id_closure).collect();
let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(|x| x.checked_add(1)).collect();

// mapping to Iterator on Iterator
let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect();

// mapping to Option on Option
let _: Option<_> = (Some(Some(1))).and_then(|x| x);
}
14 changes: 14 additions & 0 deletions tests/ui/map_flatten.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,20 @@
#![allow(clippy::map_identity)]

fn main() {
// mapping to Option on Iterator
fn option_id(x: i8) -> Option<i8> {
Some(x)
}
let option_id_ref: fn(i8) -> Option<i8> = option_id;
let option_id_closure = |x| Some(x);
let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect();
let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect();
let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect();
let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect();

// mapping to Iterator on Iterator
let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();

// mapping to Option on Option
let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
}
40 changes: 32 additions & 8 deletions tests/ui/map_flatten.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,40 @@
error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)`
--> $DIR/map_flatten.rs:8:21
error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:14:46
|
LL | 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(|x| 0..x)`
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id)`
|
= note: `-D clippy::map-flatten` implied by `-D warnings`

error: called `map(..).flatten()` on an `Option`. This is more succinctly expressed by calling `.and_then(..)`
--> $DIR/map_flatten.rs:9:24
error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:15:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id_ref)`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:16:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id_closure)`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:17:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(|x| x.checked_add(1))`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:20:46
|
LL | 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)`

error: called `map(..).flatten()` on an `Option`
--> $DIR/map_flatten.rs:23:39
|
LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `(Some(Some(1))).and_then(|x| x)`
| ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `.and_then(|x| x)`

error: aborting due to 2 previous errors
error: aborting due to 6 previous errors

0 comments on commit 617dc78

Please sign in to comment.