From 69c796e118b1c3b742f30c31fac6bac20ac3f772 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 8 Jun 2016 19:54:22 +0200 Subject: [PATCH 01/11] lint on `filter(x).map(y)`, `filter(x).flat_map(y)`, `filter_map(x).flat_map(y)` --- clippy_lints/src/methods.rs | 39 +++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 795e4e18d87..abb8078c286 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -379,6 +379,12 @@ impl LateLintPass for Pass { lint_map_unwrap_or_else(cx, expr, arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["filter", "next"]) { lint_filter_next(cx, expr, arglists[0]); + } else if let Some(arglists) = method_chain_args(expr, &["filter", "map"]) { + lint_filter_map(cx, expr, arglists[0], arglists[1]); + } else if let Some(arglists) = method_chain_args(expr, &["filter", "flat_map"]) { + lint_filter_flat_map(cx, expr, arglists[0], arglists[1]); + } else if let Some(arglists) = method_chain_args(expr, &["filter_map", "flat_map"]) { + lint_filter_map_flat_map(cx, expr, arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["find", "is_some"]) { lint_search_is_some(cx, expr, "find", arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["position", "is_some"]) { @@ -834,6 +840,39 @@ fn lint_filter_next(cx: &LateContext, expr: &hir::Expr, filter_args: &MethodArgs } } +// Type of MethodArgs is potentially a Vec +/// lint use of `filter().map() for Iterators` +fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { + // 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_NEXT, expr.span, msg); + } +} + +// Type of MethodArgs is potentially a Vec +/// lint use of `filter().flat_map() for Iterators` +fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { + // 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_NEXT, expr.span, msg); + } +} + +// Type of MethodArgs is potentially a Vec +/// lint use of `filter_map().flat_map() for Iterators` +fn lint_filter_map_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { + // lint if caller of `.filter_map().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_NEXT, expr.span, msg); + } +} + #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint searching an Iterator followed by `is_some()` From 4e528521467d014cc554976a9ec4e25659d8ead6 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 13:04:50 +0200 Subject: [PATCH 02/11] create a lint for each of the messages --- clippy_lints/src/methods.rs | 47 +++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index abb8078c286..3ecbe6251c4 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -159,6 +159,42 @@ declare_lint! { "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`" } +/// **What it does:** This lint `Warn`s on `_.filter(_).map(_)`. +/// +/// **Why is this bad?** Readability, this can be written more concisely as `_.filter_map(_)`. +/// +/// **Known problems:** Often requires a condition + Option creation in `filter_map` +/// +/// **Example:** `iter.filter(|x| x == 0).map(|x| x * 2)` +declare_lint! { + pub FILTER_MAP, Allow, + "using `filter(_).map(_)`, which is more succinctly expressed as `.filter_map(_)`" +} + +/// **What it does:** This lint `Warn`s on `_.filter(_).flat_map(_)`. +/// +/// **Why is this bad?** Readability, this just needs the `flat_map` to return an empty iterator, if the value should be filtered. +/// +/// **Known problems:** Often requires a condition + Iterator creation in `flat_map` +/// +/// **Example:** `iter.filter(|x| x == 0).flat_map(|x| x.bits())` +declare_lint! { + pub FILTER_FLAT_MAP, Allow, + "using `filter(_).flat_map(_)`, which can be rewritten using just the flat_map" +} + +/// **What it does:** This lint `Warn`s on `_.filter_map(_).flat_map(_)`. +/// +/// **Why is this bad?** Readability, this just needs the `flat_map` to return an empty iterator, if the value should be filtered. +/// +/// **Known problems:** Often requires a condition + Iterator creation in `flat_map` +/// +/// **Example:** `iter.filter_map(|x| x.process()).flat_map(|x| x.bits())` +declare_lint! { + pub FILTER_MAP_FLAT_MAP, Allow, + "using `filter_map(_).flat_map(_)`, which can be rewritten using just the flat_map" +} + /// **What it does:** This lint `Warn`s on an iterator search (such as `find()`, `position()`, or /// `rposition()`) followed by a call to `is_some()`. /// @@ -356,6 +392,9 @@ impl LintPass for Pass { SINGLE_CHAR_PATTERN, SEARCH_IS_SOME, TEMPORARY_CSTRING_AS_PTR, + FILTER_MAP, + FILTER_FLAT_MAP, + FILTER_MAP_FLAT_MAP, ITER_NTH) } } @@ -847,7 +886,7 @@ fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs 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_NEXT, expr.span, msg); + span_lint(cx, FILTER_MAP, expr.span, msg); } } @@ -858,7 +897,7 @@ fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &Metho 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_NEXT, expr.span, msg); + span_lint(cx, FILTER_FLAT_MAP, expr.span, msg); } } @@ -867,9 +906,9 @@ fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &Metho fn lint_filter_map_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // lint if caller of `.filter_map().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(..)` \ + 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_NEXT, expr.span, msg); + span_lint(cx, FILTER_MAP_FLAT_MAP, expr.span, msg); } } From 77e2155778e9700e4797acb8629ab52d19cd6b29 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 13:32:34 +0200 Subject: [PATCH 03/11] update lints --- CHANGELOG.md | 3 +++ README.md | 5 ++++- clippy_lints/src/lib.rs | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c2f9e1a622..cbbd5497c79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,9 @@ All notable changes to this project will be documented in this file. [`explicit_counter_loop`]: https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop [`explicit_iter_loop`]: https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop [`extend_from_slice`]: https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice +[`filter_flat_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_flat_map +[`filter_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_map +[`filter_map_flat_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_map_flat_map [`filter_next`]: https://github.com/Manishearth/rust-clippy/wiki#filter_next [`float_arithmetic`]: https://github.com/Manishearth/rust-clippy/wiki#float_arithmetic [`float_cmp`]: https://github.com/Manishearth/rust-clippy/wiki#float_cmp diff --git a/README.md b/README.md index f410ff990d9..8f9d47f6ee9 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 154 lints included in this crate: +There are 157 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -60,6 +60,9 @@ name [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do [extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice) | warn | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice +[filter_flat_map](https://github.com/Manishearth/rust-clippy/wiki#filter_flat_map) | allow | using `filter(_).flat_map(_)`, which can be rewritten using just the flat_map +[filter_map](https://github.com/Manishearth/rust-clippy/wiki#filter_map) | allow | using `filter(_).map(_)`, which is more succinctly expressed as `.filter_map(_)` +[filter_map_flat_map](https://github.com/Manishearth/rust-clippy/wiki#filter_map_flat_map) | allow | using `filter_map(_).flat_map(_)`, which can be rewritten using just the flat_map [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_arithmetic](https://github.com/Manishearth/rust-clippy/wiki#float_arithmetic) | allow | Any floating-point arithmetic statement [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b68596f2535..a10ab8add0c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -268,6 +268,9 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { items_after_statements::ITEMS_AFTER_STATEMENTS, matches::SINGLE_MATCH_ELSE, mem_forget::MEM_FORGET, + methods::FILTER_FLAT_MAP, + methods::FILTER_MAP, + methods::FILTER_MAP_FLAT_MAP, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, From eef439cb78d7efbf24856c81a78e2b38ebfbc189 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 13:48:56 +0200 Subject: [PATCH 04/11] add tests --- tests/compile-fail/filter_methods.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 tests/compile-fail/filter_methods.rs diff --git a/tests/compile-fail/filter_methods.rs b/tests/compile-fail/filter_methods.rs new file mode 100644 index 00000000000..2a0e4156ceb --- /dev/null +++ b/tests/compile-fail/filter_methods.rs @@ -0,0 +1,15 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(clippy, clippy_pedantic)] +fn main() { + let _: Vec<_> = vec![5; 6].into_iter() //~ERROR called `filter(p).map(q)` on an Iterator + .filter(|&x| x == 0) + .map(|x| x * 2) + .collect(); + + let _: Vec<_> = vec![5i8; 6].into_iter() //~ERROR called `filter(p).flat_map(q)` on an Iterator + .filter(|&x| x == 0) + .flat_map(|x| x.checked_mul(2)) + .collect(); +} From 48a5f8446d0e23c37a36c808fb68b2796e044ca7 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 13:49:08 +0200 Subject: [PATCH 05/11] fallout --- clippy_lints/src/lifetimes.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 797e9708b60..dc5df874020 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -71,19 +71,17 @@ enum RefLt { Named(Name), } -fn bound_lifetimes(bound: &TyParamBound) -> Option> { +fn bound_lifetimes(bound: &TyParamBound) -> HirVec<&Lifetime> { if let TraitTyParamBound(ref trait_ref, _) = *bound { - let lt = trait_ref.trait_ref - .path - .segments - .last() - .expect("a path must have at least one segment") - .parameters - .lifetimes(); - - Some(lt) + trait_ref.trait_ref + .path + .segments + .last() + .expect("a path must have at least one segment") + .parameters + .lifetimes() } else { - None + HirVec::new() } } @@ -94,7 +92,7 @@ fn check_fn_inner(cx: &LateContext, decl: &FnDecl, generics: &Generics, span: Sp let bounds_lts = generics.ty_params .iter() - .flat_map(|ref typ| typ.bounds.iter().filter_map(bound_lifetimes).flat_map(|lts| lts)); + .flat_map(|typ| typ.bounds.iter().flat_map(bound_lifetimes)); if could_use_elision(cx, decl, &generics.lifetimes, bounds_lts) { span_lint(cx, From f5dfcd694bfd9922ee94f59465e31200310a6a39 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 13:51:44 +0200 Subject: [PATCH 06/11] fallout2 --- clippy_lints/src/matches.rs | 41 ++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index fab15ac3238..e578fbf6d68 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -334,31 +334,30 @@ fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: Match /// Get all arms that are unbounded `PatRange`s. fn all_ranges(cx: &LateContext, arms: &[Arm]) -> Vec> { arms.iter() - .filter_map(|arm| { + .flat_map(|arm| { if let Arm { ref pats, guard: None, .. } = *arm { - Some(pats.iter().filter_map(|pat| { - if_let_chain! {[ - let PatKind::Range(ref lhs, ref rhs) = pat.node, - let Ok(lhs) = eval_const_expr_partial(cx.tcx, lhs, ExprTypeChecked, None), - let Ok(rhs) = eval_const_expr_partial(cx.tcx, rhs, ExprTypeChecked, None) - ], { - return Some(SpannedRange { span: pat.span, node: (lhs, rhs) }); - }} - - if_let_chain! {[ - let PatKind::Lit(ref value) = pat.node, - let Ok(value) = eval_const_expr_partial(cx.tcx, value, ExprTypeChecked, None) - ], { - return Some(SpannedRange { span: pat.span, node: (value.clone(), value) }); - }} - - None - })) + pats.iter() } else { + [].iter() + }.filter_map(|pat| { + if_let_chain! {[ + let PatKind::Range(ref lhs, ref rhs) = pat.node, + let Ok(lhs) = eval_const_expr_partial(cx.tcx, lhs, ExprTypeChecked, None), + let Ok(rhs) = eval_const_expr_partial(cx.tcx, rhs, ExprTypeChecked, None) + ], { + return Some(SpannedRange { span: pat.span, node: (lhs, rhs) }); + }} + + if_let_chain! {[ + let PatKind::Lit(ref value) = pat.node, + let Ok(value) = eval_const_expr_partial(cx.tcx, value, ExprTypeChecked, None) + ], { + return Some(SpannedRange { span: pat.span, node: (value.clone(), value) }); + }} + None - } + }) }) - .flat_map(IntoIterator::into_iter) .collect() } From 8bfb31ee9725c58f5dd2f2b3e3ce9592db49974f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 14:39:56 +0200 Subject: [PATCH 07/11] doc nits --- clippy_lints/src/methods.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 3ecbe6251c4..f3a073521be 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -858,7 +858,7 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &hir::Expr, map_args: &Method #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec -/// lint use of `filter().next() for Iterators` +/// lint use of `filter().next()` for `Iterators` fn lint_filter_next(cx: &LateContext, expr: &hir::Expr, filter_args: &MethodArgs) { // lint if caller of `.filter().next()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { @@ -880,7 +880,7 @@ fn lint_filter_next(cx: &LateContext, expr: &hir::Expr, filter_args: &MethodArgs } // Type of MethodArgs is potentially a Vec -/// lint use of `filter().map() for Iterators` +/// lint use of `filter().map()` for `Iterators` fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // lint if caller of `.filter().map()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { @@ -891,7 +891,7 @@ fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs } // Type of MethodArgs is potentially a Vec -/// lint use of `filter().flat_map() for Iterators` +/// lint use of `filter().flat_map()` for `Iterators` fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // lint if caller of `.filter().flat_map()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { @@ -902,7 +902,7 @@ fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &Metho } // Type of MethodArgs is potentially a Vec -/// lint use of `filter_map().flat_map() for Iterators` +/// lint use of `filter_map().flat_map()` for `Iterators` fn lint_filter_map_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // lint if caller of `.filter_map().flat_map()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { From 415ddfb6302039e15180f005fb1775f562844ded Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 14:41:16 +0200 Subject: [PATCH 08/11] lint message nits --- clippy_lints/src/methods.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index f3a073521be..3b7122cb410 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -862,7 +862,7 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &hir::Expr, map_args: &Method fn lint_filter_next(cx: &LateContext, expr: &hir::Expr, filter_args: &MethodArgs) { // lint if caller of `.filter().next()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `filter(p).next()` on an Iterator. This is more succinctly expressed by calling `.find(p)` \ + let msg = "called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` \ instead."; let filter_snippet = snippet(cx, filter_args[1].span, ".."); if filter_snippet.lines().count() <= 1 { @@ -884,7 +884,7 @@ fn lint_filter_next(cx: &LateContext, expr: &hir::Expr, filter_args: &MethodArgs fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // 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(..)` \ + 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); } @@ -895,7 +895,7 @@ fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // 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(..)` \ + 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_FLAT_MAP, expr.span, msg); } @@ -906,7 +906,7 @@ fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &Metho fn lint_filter_map_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // lint if caller of `.filter_map().flat_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(..)` \ + 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_FLAT_MAP, expr.span, msg); } @@ -919,7 +919,7 @@ fn lint_search_is_some(cx: &LateContext, expr: &hir::Expr, search_method: &str, is_some_args: &MethodArgs) { // lint if caller of search is an Iterator if match_trait_method(cx, &*is_some_args[0], &paths::ITERATOR) { - let msg = format!("called `is_some()` after searching an iterator with {}. This is more succinctly expressed \ + let msg = format!("called `is_some()` after searching an `Iterator` with {}. This is more succinctly expressed \ by calling `any()`.", search_method); let search_snippet = snippet(cx, search_args[1].span, ".."); From ac6e7b29577285a4476c9cc34395e4d1d51d3aac Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 21 Jun 2016 14:46:02 +0200 Subject: [PATCH 09/11] fix tests --- tests/compile-fail/filter_methods.rs | 9 +++++++-- tests/compile-fail/methods.rs | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/compile-fail/filter_methods.rs b/tests/compile-fail/filter_methods.rs index 2a0e4156ceb..8f37a8553b4 100644 --- a/tests/compile-fail/filter_methods.rs +++ b/tests/compile-fail/filter_methods.rs @@ -3,13 +3,18 @@ #![deny(clippy, clippy_pedantic)] fn main() { - let _: Vec<_> = vec![5; 6].into_iter() //~ERROR called `filter(p).map(q)` on an Iterator + let _: Vec<_> = vec![5; 6].into_iter() //~ERROR called `filter(p).map(q)` on an `Iterator` .filter(|&x| x == 0) .map(|x| x * 2) .collect(); - let _: Vec<_> = vec![5i8; 6].into_iter() //~ERROR called `filter(p).flat_map(q)` on an Iterator + let _: Vec<_> = vec![5i8; 6].into_iter() //~ERROR called `filter(p).flat_map(q)` on an `Iterator` .filter(|&x| x == 0) .flat_map(|x| x.checked_mul(2)) .collect(); + + let _: Vec<_> = vec![5i8; 6].into_iter() //~ERROR called `filter_map(p).flat_map(q)` on an `Iterator` + .filter_map(|x| x.checked_mul(2)) + .flat_map(|x| x.checked_mul(2)) + .collect(); } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 647cca5a39c..c3f18bef462 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -181,11 +181,11 @@ fn filter_next() { // check single-line case let _ = v.iter().filter(|&x| *x < 0).next(); - //~^ ERROR called `filter(p).next()` on an Iterator. + //~^ ERROR called `filter(p).next()` on an `Iterator`. //~| NOTE replace `filter(|&x| *x < 0).next()` // check multi-line case - let _ = v.iter().filter(|&x| { //~ERROR called `filter(p).next()` on an Iterator. + let _ = v.iter().filter(|&x| { //~ERROR called `filter(p).next()` on an `Iterator`. *x < 0 } ).next(); From 5ccbf3d43740a714f7c7a61bf71690fedc65046d Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 22 Jun 2016 10:44:46 +0200 Subject: [PATCH 10/11] unify the lints --- CHANGELOG.md | 2 - README.md | 4 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/methods.rs | 59 +++++++++++----------------- tests/compile-fail/filter_methods.rs | 5 +++ 5 files changed, 30 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbbd5497c79..9bd8affcded 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,9 +154,7 @@ All notable changes to this project will be documented in this file. [`explicit_counter_loop`]: https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop [`explicit_iter_loop`]: https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop [`extend_from_slice`]: https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice -[`filter_flat_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_flat_map [`filter_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_map -[`filter_map_flat_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_map_flat_map [`filter_next`]: https://github.com/Manishearth/rust-clippy/wiki#filter_next [`float_arithmetic`]: https://github.com/Manishearth/rust-clippy/wiki#float_arithmetic [`float_cmp`]: https://github.com/Manishearth/rust-clippy/wiki#float_cmp diff --git a/README.md b/README.md index 8f9d47f6ee9..4345c504822 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 157 lints included in this crate: +There are 155 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -60,9 +60,7 @@ name [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do [extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice) | warn | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice -[filter_flat_map](https://github.com/Manishearth/rust-clippy/wiki#filter_flat_map) | allow | using `filter(_).flat_map(_)`, which can be rewritten using just the flat_map [filter_map](https://github.com/Manishearth/rust-clippy/wiki#filter_map) | allow | using `filter(_).map(_)`, which is more succinctly expressed as `.filter_map(_)` -[filter_map_flat_map](https://github.com/Manishearth/rust-clippy/wiki#filter_map_flat_map) | allow | using `filter_map(_).flat_map(_)`, which can be rewritten using just the flat_map [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_arithmetic](https://github.com/Manishearth/rust-clippy/wiki#float_arithmetic) | allow | Any floating-point arithmetic statement [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a10ab8add0c..835823b8cd3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -268,9 +268,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { items_after_statements::ITEMS_AFTER_STATEMENTS, matches::SINGLE_MATCH_ELSE, mem_forget::MEM_FORGET, - methods::FILTER_FLAT_MAP, methods::FILTER_MAP, - methods::FILTER_MAP_FLAT_MAP, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 3b7122cb410..97130ee751f 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -159,11 +159,11 @@ declare_lint! { "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`" } -/// **What it does:** This lint `Warn`s on `_.filter(_).map(_)`. +/// **What it does:** This lint `Warn`s on `_.filter(_).map(_)`, `_.filter(_).flat_map(_)`, `_.filter_map(_).flat_map(_)` and similar. /// -/// **Why is this bad?** Readability, this can be written more concisely as `_.filter_map(_)`. +/// **Why is this bad?** Readability, this can be written more concisely as a single method call /// -/// **Known problems:** Often requires a condition + Option creation in `filter_map` +/// **Known problems:** Often requires a condition + Option/Iterator creation inside the closure /// /// **Example:** `iter.filter(|x| x == 0).map(|x| x * 2)` declare_lint! { @@ -171,30 +171,6 @@ declare_lint! { "using `filter(_).map(_)`, which is more succinctly expressed as `.filter_map(_)`" } -/// **What it does:** This lint `Warn`s on `_.filter(_).flat_map(_)`. -/// -/// **Why is this bad?** Readability, this just needs the `flat_map` to return an empty iterator, if the value should be filtered. -/// -/// **Known problems:** Often requires a condition + Iterator creation in `flat_map` -/// -/// **Example:** `iter.filter(|x| x == 0).flat_map(|x| x.bits())` -declare_lint! { - pub FILTER_FLAT_MAP, Allow, - "using `filter(_).flat_map(_)`, which can be rewritten using just the flat_map" -} - -/// **What it does:** This lint `Warn`s on `_.filter_map(_).flat_map(_)`. -/// -/// **Why is this bad?** Readability, this just needs the `flat_map` to return an empty iterator, if the value should be filtered. -/// -/// **Known problems:** Often requires a condition + Iterator creation in `flat_map` -/// -/// **Example:** `iter.filter_map(|x| x.process()).flat_map(|x| x.bits())` -declare_lint! { - pub FILTER_MAP_FLAT_MAP, Allow, - "using `filter_map(_).flat_map(_)`, which can be rewritten using just the flat_map" -} - /// **What it does:** This lint `Warn`s on an iterator search (such as `find()`, `position()`, or /// `rposition()`) followed by a call to `is_some()`. /// @@ -393,8 +369,6 @@ impl LintPass for Pass { SEARCH_IS_SOME, TEMPORARY_CSTRING_AS_PTR, FILTER_MAP, - FILTER_FLAT_MAP, - FILTER_MAP_FLAT_MAP, ITER_NTH) } } @@ -420,6 +394,8 @@ impl LateLintPass for Pass { lint_filter_next(cx, expr, arglists[0]); } else if let Some(arglists) = method_chain_args(expr, &["filter", "map"]) { lint_filter_map(cx, expr, arglists[0], arglists[1]); + } else if let Some(arglists) = method_chain_args(expr, &["filter_map", "map"]) { + lint_filter_map_map(cx, expr, arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["filter", "flat_map"]) { lint_filter_flat_map(cx, expr, arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["filter_map", "flat_map"]) { @@ -884,8 +860,19 @@ fn lint_filter_next(cx: &LateContext, expr: &hir::Expr, filter_args: &MethodArgs fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // 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."; + 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); + } +} + +// Type of MethodArgs is potentially a Vec +/// lint use of `filter().map()` for `Iterators` +fn lint_filter_map_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { + // 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); } } @@ -895,9 +882,10 @@ fn lint_filter_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // 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(..)` \ + 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_FLAT_MAP, expr.span, msg); + span_lint(cx, FILTER_MAP, expr.span, msg); } } @@ -906,9 +894,10 @@ fn lint_filter_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &Metho fn lint_filter_map_flat_map(cx: &LateContext, expr: &hir::Expr, _filter_args: &MethodArgs, _map_args: &MethodArgs) { // lint if caller of `.filter_map().flat_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(..)` \ + 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_FLAT_MAP, expr.span, msg); + span_lint(cx, FILTER_MAP, expr.span, msg); } } diff --git a/tests/compile-fail/filter_methods.rs b/tests/compile-fail/filter_methods.rs index 8f37a8553b4..743c3c15aeb 100644 --- a/tests/compile-fail/filter_methods.rs +++ b/tests/compile-fail/filter_methods.rs @@ -17,4 +17,9 @@ fn main() { .filter_map(|x| x.checked_mul(2)) .flat_map(|x| x.checked_mul(2)) .collect(); + + let _: Vec<_> = vec![5i8; 6].into_iter() //~ERROR called `filter_map(p).map(q)` on an `Iterator` + .filter_map(|x| x.checked_mul(2)) + .map(|x| x.checked_mul(2)) + .collect(); } From 262148c946187dd69e2d875360c45fc4cbd9becf Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 22 Jun 2016 13:03:59 +0200 Subject: [PATCH 11/11] update lint doc text --- README.md | 2 +- clippy_lints/src/methods.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4345c504822..d210a1b218f 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ name [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do [extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice) | warn | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice -[filter_map](https://github.com/Manishearth/rust-clippy/wiki#filter_map) | allow | using `filter(_).map(_)`, which is more succinctly expressed as `.filter_map(_)` +[filter_map](https://github.com/Manishearth/rust-clippy/wiki#filter_map) | allow | using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_arithmetic](https://github.com/Manishearth/rust-clippy/wiki#float_arithmetic) | allow | Any floating-point arithmetic statement [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 97130ee751f..950a03159b2 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -168,7 +168,7 @@ declare_lint! { /// **Example:** `iter.filter(|x| x == 0).map(|x| x * 2)` declare_lint! { pub FILTER_MAP, Allow, - "using `filter(_).map(_)`, which is more succinctly expressed as `.filter_map(_)`" + "using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call" } /// **What it does:** This lint `Warn`s on an iterator search (such as `find()`, `position()`, or