diff --git a/CHANGELOG.md b/CHANGELOG.md index 630c8bf80922..728ed347da27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -158,6 +158,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_map`]: https://github.com/Manishearth/rust-clippy/wiki#filter_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 f410ff990d9c..d210a1b218ff 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 155 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -60,6 +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 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/lib.rs b/clippy_lints/src/lib.rs index b68596f25355..835823b8cd31 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -268,6 +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_MAP, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 797e9708b609..dc5df8740201 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, diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index fab15ac3238f..e578fbf6d68c 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() } diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 795e4e18d878..950a03159b22 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -159,6 +159,18 @@ declare_lint! { "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`" } +/// **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 a single method call +/// +/// **Known problems:** Often requires a condition + Option/Iterator creation inside the closure +/// +/// **Example:** `iter.filter(|x| x == 0).map(|x| x * 2)` +declare_lint! { + pub FILTER_MAP, Allow, + "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 /// `rposition()`) followed by a call to `is_some()`. /// @@ -356,6 +368,7 @@ impl LintPass for Pass { SINGLE_CHAR_PATTERN, SEARCH_IS_SOME, TEMPORARY_CSTRING_AS_PTR, + FILTER_MAP, ITER_NTH) } } @@ -379,6 +392,14 @@ 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_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"]) { + 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"]) { @@ -813,11 +834,11 @@ 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) { - 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 { @@ -834,6 +855,52 @@ 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_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); + } +} + +// 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_MAP, 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_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); + } +} + #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint searching an Iterator followed by `is_some()` @@ -841,7 +908,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, ".."); diff --git a/tests/compile-fail/filter_methods.rs b/tests/compile-fail/filter_methods.rs new file mode 100644 index 000000000000..743c3c15aeb8 --- /dev/null +++ b/tests/compile-fail/filter_methods.rs @@ -0,0 +1,25 @@ +#![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(); + + 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(); + + 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(); +} diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 647cca5a39cc..c3f18bef4625 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();