From a427c99f3d2a0b2c55d19af73bcad81f1dc761ab Mon Sep 17 00:00:00 2001 From: Dmitry Murzin Date: Sat, 25 Jul 2020 20:04:59 +0300 Subject: [PATCH] Handle mapping to Option in `map_flatten` lint --- clippy_lints/src/methods/mod.rs | 26 ++++++++++++++++++++++---- tests/ui/map_flatten.fixed | 1 + tests/ui/map_flatten.rs | 1 + tests/ui/map_flatten.stderr | 16 +++++++++++----- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9edcdd979..3f62a3cab 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2569,17 +2569,35 @@ 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 map_closure_ty = cx.typeck_results().expr_ty(&map_args[1]); + let is_map_to_option = if let ty::Closure(_def_id, substs) = map_closure_ty.kind { + let map_closure_return_ty = cx.tcx.erase_late_bound_regions(&substs.as_closure().sig().output()); + is_type_diagnostic_item(cx, map_closure_return_ty, sym!(option_type)) + } else { + false + }; + + let method_to_use = if is_map_to_option { + // `(...).map(...)` has type `impl Iterator> + "filter_map" + } else { + // `(...).map(...)` has type `impl Iterator> + "flat_map" + }; + let msg = &format!( + "called `map(..).flatten()` on an `Iterator`. \ + This is more succinctly expressed by calling `.{}(..)`", + method_to_use + ); let self_snippet = snippet(cx, map_args[0].span, ".."); 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}({2})", self_snippet, method_to_use, func_snippet); span_lint_and_sugg( cx, MAP_FLATTEN, expr.span, msg, - "try using `flat_map` instead", + &format!("try using `{}` instead", method_to_use), hint, Applicability::MachineApplicable, ); diff --git a/tests/ui/map_flatten.fixed b/tests/ui/map_flatten.fixed index 4171d80f4..684a28aeb 100644 --- a/tests/ui/map_flatten.fixed +++ b/tests/ui/map_flatten.fixed @@ -5,6 +5,7 @@ #![allow(clippy::map_identity)] fn main() { + let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(|x| x.checked_add(1)).collect(); let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect(); let _: Option<_> = (Some(Some(1))).and_then(|x| x); } diff --git a/tests/ui/map_flatten.rs b/tests/ui/map_flatten.rs index 16a0fd090..05789ee52 100644 --- a/tests/ui/map_flatten.rs +++ b/tests/ui/map_flatten.rs @@ -5,6 +5,7 @@ #![allow(clippy::map_identity)] fn main() { + let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect(); let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect(); let _: Option<_> = (Some(Some(1))).map(|x| x).flatten(); } diff --git a/tests/ui/map_flatten.stderr b/tests/ui/map_flatten.stderr index 00bc41c15..d2d15362a 100644 --- a/tests/ui/map_flatten.stderr +++ b/tests/ui/map_flatten.stderr @@ -1,16 +1,22 @@ -error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` +error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.filter_map(..)` --> $DIR/map_flatten.rs:8:21 | -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(|x| x.checked_add(1)).flatten().collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `vec![5_i8; 6].into_iter().filter_map(|x| x.checked_add(1))` | = note: `-D clippy::map-flatten` implied by `-D warnings` +error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` + --> $DIR/map_flatten.rs:9:21 + | +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)` + error: called `map(..).flatten()` on an `Option`. This is more succinctly expressed by calling `.and_then(..)` - --> $DIR/map_flatten.rs:9:24 + --> $DIR/map_flatten.rs:10:24 | LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `(Some(Some(1))).and_then(|x| x)` -error: aborting due to 2 previous errors +error: aborting due to 3 previous errors