diff --git a/CHANGELOG.md b/CHANGELOG.md index 3710342d841a..8a06eba8adc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -944,6 +944,7 @@ Released 2018-09-13 [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes [`fallible_impl_from`]: https://rust-lang.github.io/rust-clippy/master/index.html#fallible_impl_from [`filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map +[`filter_map_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_map [`filter_map_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_next [`filter_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_next [`find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#find_map diff --git a/README.md b/README.md index 51f618da4b78..c1a25aa13006 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 306 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b56413fc2c44..6585b6fbb6dd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -633,6 +633,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { loops::EXPLICIT_ITER_LOOP, matches::SINGLE_MATCH_ELSE, methods::FILTER_MAP, + methods::FILTER_MAP_MAP, methods::FILTER_MAP_NEXT, methods::FIND_MAP, methods::MAP_FLATTEN, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 07eb70a33acf..82993c3bd535 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -13,6 +13,7 @@ use rustc::hir::intravisit::{self, Visitor}; use rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass}; use rustc::ty::{self, Predicate, Ty}; use rustc::{declare_lint_pass, declare_tool_lint}; +use crate::rustc_data_structures::fx::FxHashMap; use rustc_errors::Applicability; use syntax::ast; use syntax::source_map::{BytePos, Span}; @@ -272,8 +273,70 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for usage of `_.filter(_).map(_)`, - /// `_.filter(_).flat_map(_)`, `_.filter_map(_).flat_map(_)` and similar. + /// **What it does:** Checks for usage of `_.filter_map(_).map(_)`, which can be more concisely + /// written as `_.filter_map(_)`. + /// + /// **Why is this bad?** Readability, this can be written more concisely as a + /// single method call. + /// + /// ** Example:** + /// ```rust + /// let ws: Vec = ["is", "this", "shizzle", "for", "rizzle"] + /// .iter() + /// .map(|s| s.to_string()) + /// .collect(); + /// + /// let correct: Vec = ws + /// .iter() + /// .filter_map(|s| if s.contains("izzle") { Some(s) } else { None }) + /// .map(|s| format!("{}{}", s, s)) + /// .collect(); + /// + /// let more_correct: Vec = ws + /// .iter() + /// .filter_map(|s| { + /// if s.contains("izzle") { + /// Some(format!("{}{}", s, s)) + /// } else { + /// None + /// } + /// }) + /// .collect(); + /// + /// println!("{:?}", correct); + /// assert_eq!(correct, more_correct); + /// ``` + /// .iter() + /// .map(|s| s.to_string()) + /// .collect(); + /// + /// let correct: Vec = ws.iter() + /// .filter_map(|s| if s.contains("izzle"){ + /// Some(s) + /// } else { + /// None + /// }) + /// .map(|s| format!("{}{}", s, s)) + /// .collect(); + /// + /// let more_correct: Vec = ws.iter() + /// .filter_map(|s| if s.contains("izzle"){ + /// Some(format!("{}{}", s, s)) + /// } else { + /// None + /// }) + /// .collect(); + /// + /// println!("{:?}", correct); + /// assert_eq!(correct, more_correct); + pub FILTER_MAP_MAP, + pedantic, + "using `filter_map` followed by `map` can more concisely be written as a single method call" +} + +declare_clippy_lint! { + /// **What it does:** Checks for usage of `_.filter(_).map(_)` to destructure an iterator of + /// Options or Results instead of a single `_.filter_map(_)` call. /// /// **Why is this bad?** Readability, this can be written more concisely as a /// single method call. @@ -287,7 +350,7 @@ declare_clippy_lint! { /// ``` pub FILTER_MAP, pedantic, - "using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call" + "using `filter` and `map` to destructure an iterator of Options or Results instead of a single `filter_map` call" } declare_clippy_lint! { @@ -842,6 +905,7 @@ declare_lint_pass!(Methods => [ SEARCH_IS_SOME, TEMPORARY_CSTRING_AS_PTR, FILTER_NEXT, + FILTER_MAP_MAP, FILTER_MAP, FILTER_MAP_NEXT, FIND_MAP, @@ -882,8 +946,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]), ["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1]), ["map", "find"] => lint_find_map(cx, expr, arg_lists[1], arg_lists[0]), - ["flat_map", "filter"] => lint_filter_flat_map(cx, expr, arg_lists[1], arg_lists[0]), - ["flat_map", "filter_map"] => lint_filter_map_flat_map(cx, expr, arg_lists[1], arg_lists[0]), ["flatten", "map"] => lint_map_flatten(cx, expr, arg_lists[1]), ["is_some", "find"] => lint_search_is_some(cx, expr, "find", arg_lists[1], arg_lists[0]), ["is_some", "position"] => lint_search_is_some(cx, expr, "position", arg_lists[1], arg_lists[0]), @@ -1998,14 +2060,35 @@ fn lint_filter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, fn lint_filter_map<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, - _filter_args: &'tcx [hir::Expr], - _map_args: &'tcx [hir::Expr], + filter_args: &'tcx [hir::Expr], + map_args: &'tcx [hir::Expr], ) { - // lint if caller of `.filter().map()` is an Iterator - if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `filter(p).map(q)` on an `Iterator`. \ - This is more succinctly expressed by calling `.filter_map(..)` instead."; - span_lint(cx, FILTER_MAP, expr.span, msg); + let mut responses: FxHashMap = FxHashMap::default(); + responses.insert( + "is_some".to_string(), + "called `filter(p).map(q)` on an `Iterator>`. \ + Consider calling `.filter_map(..)` instead.", + ); + responses.insert( + "is_ok".to_string(), + "called `filter(p).map(q)` on an `Iterator>`. \ + Consider calling `.filter_map(..)` instead.", + ); + + if_chain! { + if match_trait_method(cx, expr, &paths::ITERATOR); + if let hir::ExprKind::Closure(_, _, ref map_body_id, _, _) = map_args[1].node; + if let hir::ExprKind::MethodCall(ref map_ps, _, _) = cx.tcx.hir().body(*map_body_id).value.node; + if map_ps.ident.name == sym!(unwrap); + if let hir::ExprKind::Closure(_, _, ref filter_body_id, _, _) = filter_args[1].node; + if let hir::ExprKind::MethodCall(ref filter_ps, _, _) = cx.tcx.hir().body(*filter_body_id).value.node; + if responses.contains_key(&filter_ps.ident.name.to_string()); + if let hir::ExprKind::MethodCall(_, _, ref expr_vec) = expr.node; + if let hir::ExprKind::MethodCall(_, ref filter_span, _) = expr_vec[0].node; + then { + let new_span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt()); + span_lint(cx, FILTER_MAP, new_span, responses[&filter_ps.ident.name.to_string()]); + } } } @@ -2052,43 +2135,18 @@ fn lint_filter_map_map<'a, 'tcx>( _filter_args: &'tcx [hir::Expr], _map_args: &'tcx [hir::Expr], ) { - // lint if caller of `.filter().map()` is an Iterator - if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `filter_map(p).map(q)` on an `Iterator`. \ - This is more succinctly expressed by only calling `.filter_map(..)` instead."; - span_lint(cx, FILTER_MAP, expr.span, msg); - } -} - -/// lint use of `filter().flat_map()` for `Iterators` -fn lint_filter_flat_map<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - expr: &'tcx hir::Expr, - _filter_args: &'tcx [hir::Expr], - _map_args: &'tcx [hir::Expr], -) { - // lint if caller of `.filter().flat_map()` is an Iterator - if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `filter(p).flat_map(q)` on an `Iterator`. \ - This is more succinctly expressed by calling `.flat_map(..)` \ - and filtering by returning an empty Iterator."; - span_lint(cx, FILTER_MAP, expr.span, msg); + let mut span = expr.span; + if let hir::ExprKind::MethodCall(_, _, ref expr_vec) = expr.node { + if let hir::ExprKind::MethodCall(_, ref filter_span, _) = expr_vec[0].node { + span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt()); + } } -} -/// lint use of `filter_map().flat_map()` for `Iterators` -fn lint_filter_map_flat_map<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - expr: &'tcx hir::Expr, - _filter_args: &'tcx [hir::Expr], - _map_args: &'tcx [hir::Expr], -) { - // lint if caller of `.filter_map().flat_map()` is an Iterator + // lint if caller of `.filter_map().map()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `filter_map(p).flat_map(q)` on an `Iterator`. \ - This is more succinctly expressed by calling `.flat_map(..)` \ - and filtering by returning an empty Iterator."; - span_lint(cx, FILTER_MAP, expr.span, msg); + let msg = "called `filter_map(p).map(q)` on an `Iterator`. \ + This is more succinctly expressed by only calling `.filter_map(..)` instead."; + span_lint(cx, FILTER_MAP_MAP, span, msg); } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 663b6a5e7092..d52f036d1e39 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 305] = [ +pub const ALL_LINTS: [Lint; 306] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -528,7 +528,14 @@ pub const ALL_LINTS: [Lint; 305] = [ Lint { name: "filter_map", group: "pedantic", - desc: "using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call", + desc: "using `filter` and `map` to destructure an iterator of Options or a Results instead of a single `filter_map` call", + deprecation: None, + module: "methods", + }, + Lint { + name: "filter_map_map", + group: "pedantic", + desc: "using `filter_map` followed by `map` can more concisely be written as a single method call", deprecation: None, module: "methods", }, diff --git a/tests/ui/filter_methods.rs b/tests/ui/filter_methods.rs index ef434245fd70..690cb4bd94e3 100644 --- a/tests/ui/filter_methods.rs +++ b/tests/ui/filter_methods.rs @@ -1,24 +1,55 @@ #![warn(clippy::all, clippy::pedantic)] #![allow(clippy::missing_docs_in_private_items)] +#![allow(clippy::redundant_closure)] +#![allow(clippy::unnecessary_filter_map)] fn main() { - let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect(); + let options: Vec> = vec![Some(5), None, Some(6)]; + let results = ["1", "lol", "3", "NaN", "5"]; - let _: Vec<_> = vec![5_i8; 6] - .into_iter() - .filter(|&x| x == 0) - .flat_map(|x| x.checked_mul(2)) + // validate iterator of values triggers filter_map + map lint + let _: Vec<_> = vec![5_i8; 6].into_iter() + .filter_map(|x| x.checked_mul(2)) + .map(|x| x.checked_mul(2)) .collect(); - let _: Vec<_> = vec![5_i8; 6] - .into_iter() - .filter_map(|x| x.checked_mul(2)) - .flat_map(|x| x.checked_mul(2)) + // validate iterator of options triggers filter + map lint + let _: Vec = options.clone().into_iter() + .filter(|x| x.is_some()) + .map(|x| x.unwrap()) .collect(); + // validate iterator of options triggers filter + flat_map lint + let _: Vec> = std::iter::repeat(options.clone()) + .take(5) + .map(|v| Some(v.into_iter())) + .filter(|x| x.is_some()) + .flat_map(|x| x.unwrap()) + .collect(); + + // validate iterator of results triggers filter + map lint + let _: Vec = results.iter() + .map(|s| s.parse()) + .filter(|s| s.is_ok()) + .map(|s| s.unwrap()) + .collect(); + + // validate iterator of values **does not** trigger filter + map lint + let _: Vec = results.iter() + .filter(|s| s.len() > 3) + .map(|s| format!("{}{}", s, s)) + .collect(); + + // validate iterator of values **does not** trigger filter + flat_map lint + let _: String = results.iter() + .filter(|i| i.len() > 1) + .flat_map(|s| s.chars()) + .collect(); + + // validate filter_map + flat_map **does not** trigger linter let _: Vec<_> = vec![5_i8; 6] .into_iter() .filter_map(|x| x.checked_mul(2)) - .map(|x| x.checked_mul(2)) + .flat_map(|x| x.checked_mul(2)) .collect(); } diff --git a/tests/ui/filter_methods.stderr b/tests/ui/filter_methods.stderr index 9dfd91f6d640..745541e386ee 100644 --- a/tests/ui/filter_methods.stderr +++ b/tests/ui/filter_methods.stderr @@ -1,40 +1,68 @@ -error: called `filter(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.filter_map(..)` instead. - --> $DIR/filter_methods.rs:5:21 +error: called `filter_map(p).map(q)` on an `Iterator`. This is more succinctly expressed by only calling `.filter_map(..)` instead. + --> $DIR/filter_methods.rs:12:10 + | +LL | .filter_map(|x| x.checked_mul(2)) + | __________^ +LL | | .map(|x| x.checked_mul(2)) + | |__________________________________^ + | + = note: `-D clippy::filter-map-map` implied by `-D warnings` + +error: redundant closure found + --> $DIR/filter_methods.rs:19:14 + | +LL | .map(|x| x.unwrap()) + | ^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::unwrap` + | + = note: `-D clippy::redundant-closure-for-method-calls` implied by `-D warnings` + +error: called `filter(p).map(q)` on an `Iterator>`. Consider calling `.filter_map(..)` instead. + --> $DIR/filter_methods.rs:18:10 | -LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | .filter(|x| x.is_some()) + | __________^ +LL | | .map(|x| x.unwrap()) + | |____________________________^ | = note: `-D clippy::filter-map` implied by `-D warnings` -error: called `filter(p).flat_map(q)` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` and filtering by returning an empty Iterator. - --> $DIR/filter_methods.rs:7:21 +error: redundant closure found + --> $DIR/filter_methods.rs:18:17 | -LL | let _: Vec<_> = vec![5_i8; 6] - | _____________________^ -LL | | .into_iter() -LL | | .filter(|&x| x == 0) -LL | | .flat_map(|x| x.checked_mul(2)) - | |_______________________________________^ +LL | .filter(|x| x.is_some()) + | ^^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::is_some` -error: called `filter_map(p).flat_map(q)` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` and filtering by returning an empty Iterator. - --> $DIR/filter_methods.rs:13:21 +error: redundant closure found + --> $DIR/filter_methods.rs:27:19 | -LL | let _: Vec<_> = vec![5_i8; 6] - | _____________________^ -LL | | .into_iter() -LL | | .filter_map(|x| x.checked_mul(2)) -LL | | .flat_map(|x| x.checked_mul(2)) - | |_______________________________________^ +LL | .flat_map(|x| x.unwrap()) + | ^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::unwrap` -error: called `filter_map(p).map(q)` on an `Iterator`. This is more succinctly expressed by only calling `.filter_map(..)` instead. - --> $DIR/filter_methods.rs:19:21 +error: redundant closure found + --> $DIR/filter_methods.rs:26:17 | -LL | let _: Vec<_> = vec![5_i8; 6] - | _____________________^ -LL | | .into_iter() -LL | | .filter_map(|x| x.checked_mul(2)) -LL | | .map(|x| x.checked_mul(2)) - | |__________________________________^ +LL | .filter(|x| x.is_some()) + | ^^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::is_some` + +error: redundant closure found + --> $DIR/filter_methods.rs:34:14 + | +LL | .map(|s| s.unwrap()) + | ^^^^^^^^^^^^^^ help: remove closure as shown: `std::result::Result::unwrap` + +error: called `filter(p).map(q)` on an `Iterator>`. Consider calling `.filter_map(..)` instead. + --> $DIR/filter_methods.rs:33:10 + | +LL | .filter(|s| s.is_ok()) + | __________^ +LL | | .map(|s| s.unwrap()) + | |____________________________^ + +error: redundant closure found + --> $DIR/filter_methods.rs:33:17 + | +LL | .filter(|s| s.is_ok()) + | ^^^^^^^^^^^^^ help: remove closure as shown: `std::result::Result::is_ok` -error: aborting due to 4 previous errors +error: aborting due to 9 previous errors