diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb4eb17d4..9318c85ced9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5177,6 +5177,8 @@ Released 2018-09-13 [`items_after_test_module`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_test_module [`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect [`iter_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_count +[`iter_filter_is_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_filter_is_ok +[`iter_filter_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_filter_is_some [`iter_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map [`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop [`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 38fda36c58a..a00e2d3e044 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -369,6 +369,8 @@ crate::methods::ITERATOR_STEP_BY_ZERO_INFO, crate::methods::ITER_CLONED_COLLECT_INFO, crate::methods::ITER_COUNT_INFO, + crate::methods::ITER_FILTER_IS_OK_INFO, + crate::methods::ITER_FILTER_IS_SOME_INFO, crate::methods::ITER_KV_MAP_INFO, crate::methods::ITER_NEXT_SLICE_INFO, crate::methods::ITER_NTH_INFO, diff --git a/clippy_lints/src/methods/filter_map.rs b/clippy_lints/src/methods/filter_map.rs index a8ca82d20e7..7cfd3d346b6 100644 --- a/clippy_lints/src/methods/filter_map.rs +++ b/clippy_lints/src/methods/filter_map.rs @@ -22,6 +22,7 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> hir::ExprKind::Path(QPath::Resolved(_, segments)) => { segments.segments.last().unwrap().ident.name == method_name }, + hir::ExprKind::MethodCall(segment, _, _, _) => segment.ident.name == method_name, hir::ExprKind::Closure(&hir::Closure { body, .. }) => { let body = cx.tcx.hir().body(body); let closure_expr = peel_blocks(body.value); diff --git a/clippy_lints/src/methods/iter_filter.rs b/clippy_lints/src/methods/iter_filter.rs new file mode 100644 index 00000000000..04dc4820f6a --- /dev/null +++ b/clippy_lints/src/methods/iter_filter.rs @@ -0,0 +1,79 @@ +use rustc_lint::LateContext; + +use super::{ITER_FILTER_IS_OK, ITER_FILTER_IS_SOME}; + +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::{indent_of, reindent_multiline}; +use clippy_utils::{is_trait_method, peel_blocks}; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::def::Res; +use rustc_hir::QPath; +use rustc_span::symbol::{sym, Symbol}; +use rustc_span::Span; +use std::borrow::Cow; + +fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool { + match &expr.kind { + hir::ExprKind::Path(QPath::TypeRelative(_, mname)) => mname.ident.name == method_name, + hir::ExprKind::Path(QPath::Resolved(_, segments)) => { + segments.segments.last().unwrap().ident.name == method_name + }, + hir::ExprKind::MethodCall(segment, _, _, _) => segment.ident.name == method_name, + hir::ExprKind::Closure(&hir::Closure { body, .. }) => { + let body = cx.tcx.hir().body(body); + let closure_expr = peel_blocks(body.value); + let arg_id = body.params[0].pat.hir_id; + match closure_expr.kind { + hir::ExprKind::MethodCall(hir::PathSegment { ident, .. }, receiver, ..) => { + if ident.name == method_name + && let hir::ExprKind::Path(path) = &receiver.kind + && let Res::Local(ref local) = cx.qpath_res(path, receiver.hir_id) + { + return arg_id == *local; + } + false + }, + _ => false, + } + }, + _ => false, + } +} + +fn parent_is_map(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + if let hir::Node::Expr(parent_expr) = cx.tcx.hir().get_parent(expr.hir_id) { + is_method(cx, parent_expr, rustc_span::sym::map) + } else { + false + } +} + +#[allow(clippy::too_many_arguments)] +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir::Expr<'_>, filter_span: Span) { + let is_iterator = is_trait_method(cx, expr, sym::Iterator); + let parent_is_not_map = !parent_is_map(cx, expr); + + if is_iterator && parent_is_not_map && is_method(cx, filter_arg, sym!(is_some)) { + span_lint_and_sugg( + cx, + ITER_FILTER_IS_SOME, + filter_span.with_hi(expr.span.hi()), + "`filter` for `is_some` on iterator over `Option`", + "consider using `flatten` instead", + reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, filter_span)).into_owned(), + Applicability::MaybeIncorrect, + ); + } + if is_iterator && parent_is_not_map && is_method(cx, filter_arg, sym!(is_ok)) { + span_lint_and_sugg( + cx, + ITER_FILTER_IS_OK, + filter_span.with_hi(expr.span.hi()), + "`filter` for `is_ok` on iterator over `Result`s", + "consider using `flatten` instead", + reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, filter_span)).into_owned(), + Applicability::MaybeIncorrect, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a94524b3ba6..d67fa28b30b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -38,6 +38,7 @@ mod is_digit_ascii_radix; mod iter_cloned_collect; mod iter_count; +mod iter_filter; mod iter_kv_map; mod iter_next_slice; mod iter_nth; @@ -1175,7 +1176,7 @@ declare_clippy_lint! { /// ### What it does - /// Checks for iterators of `Option`s using ``.filter(Option::is_some).map(Option::unwrap)` that may + /// Checks for iterators of `Option`s using `.filter(Option::is_some).map(Option::unwrap)` that may /// be replaced with a `.flatten()` call. /// /// ### Why is this bad? @@ -3755,7 +3756,7 @@ declare_clippy_lint! { /// ### What it does - /// Checks for iterators of `Result`s using ``.filter(Result::is_ok).map(Result::unwrap)` that may + /// Checks for iterators of `Result`s using `.filter(Result::is_ok).map(Result::unwrap)` that may /// be replaced with a `.flatten()` call. /// /// ### Why is this bad? @@ -3776,6 +3777,58 @@ "filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `.filter(Optional::is_some)` that may be replaced with a `.flatten()` call. + /// This lint is potentially incorrect as it affects the type of follow-up calls. + /// + /// ### Why is this bad? + /// This pattern is often followed by manual unwrapping of the `Option`. The simplification + /// results in more readable and succint code without the need for manual unwrapping. + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// vec![Some(1)].into_iter().filter(Option::is_some); + /// + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// vec![Some(1)].into_iter().flatten(); + /// ``` + #[clippy::version = "1.76.0"] + pub ITER_FILTER_IS_SOME, + complexity, + "filtering an iterator over `Option`s for `Some` can be achieved with `flatten`" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `.filter(Result::is_ok)` that may be replaced with a `.flatten()` call. + /// This lint is potentially incorrect as it affects the type of follow-up calls. + /// + /// ### Why is this bad? + /// This pattern is often followed by manual unwrapping of `Result`. The simplification + /// results in more readable and succint code without the need for manual unwrapping. + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// vec![Ok::(1)].into_iter().filter(Result::is_ok); + /// + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// vec![Ok::(1)].into_iter().flatten(); + /// ``` + #[clippy::version = "1.76.0"] + pub ITER_FILTER_IS_OK, + complexity, + "filtering an iterator over `Result`s for `Ok` can be achieved with `flatten`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3928,6 +3981,8 @@ pub fn new( JOIN_ABSOLUTE_PATHS, OPTION_MAP_OR_ERR_OK, RESULT_FILTER_MAP, + ITER_FILTER_IS_SOME, + ITER_FILTER_IS_OK, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4257,7 +4312,22 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, - (name @ ("filter" | "find"), [arg]) => { + ("filter", [arg]) => { + if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { + // if `arg` has side-effect, the semantic will change + iter_overeager_cloned::check( + cx, + expr, + recv, + recv2, + iter_overeager_cloned::Op::FixClosure(name, arg), + false, + ); + } + + iter_filter::check(cx, expr, arg, span); + }, + ("find", [arg]) => { if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { // if `arg` has side-effect, the semantic will change iter_overeager_cloned::check( diff --git a/tests/ui/iter_filter_is_ok.fixed b/tests/ui/iter_filter_is_ok.fixed new file mode 100644 index 00000000000..9985877a02a --- /dev/null +++ b/tests/ui/iter_filter_is_ok.fixed @@ -0,0 +1,8 @@ +#![warn(clippy::iter_filter_is_ok)] + +fn main() { + let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten(); + //~^ HELP: consider using `flatten` instead + let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten(); + //~^ HELP: consider using `flatten` instead +} diff --git a/tests/ui/iter_filter_is_ok.rs b/tests/ui/iter_filter_is_ok.rs new file mode 100644 index 00000000000..176ce902637 --- /dev/null +++ b/tests/ui/iter_filter_is_ok.rs @@ -0,0 +1,8 @@ +#![warn(clippy::iter_filter_is_ok)] + +fn main() { + let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Result::is_ok); + //~^ HELP: consider using `flatten` instead + let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| a.is_ok()); + //~^ HELP: consider using `flatten` instead +} diff --git a/tests/ui/iter_filter_is_ok.stderr b/tests/ui/iter_filter_is_ok.stderr new file mode 100644 index 00000000000..a5d7da977ff --- /dev/null +++ b/tests/ui/iter_filter_is_ok.stderr @@ -0,0 +1,17 @@ +error: `filter` for `is_ok` on iterator over `Result`s + --> $DIR/iter_filter_is_ok.rs:4:52 + | +LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(Result::is_ok); + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` + | + = note: `-D clippy::iter-filter-is-ok` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::iter_filter_is_ok)]` + +error: `filter` for `is_ok` on iterator over `Result`s + --> $DIR/iter_filter_is_ok.rs:6:52 + | +LL | let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| a.is_ok()); + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/iter_filter_is_some.fixed b/tests/ui/iter_filter_is_some.fixed new file mode 100644 index 00000000000..3732a8e72d1 --- /dev/null +++ b/tests/ui/iter_filter_is_some.fixed @@ -0,0 +1,12 @@ +#![warn(clippy::iter_filter_is_some)] + +fn odds_out(x: i32) -> Option { + if x % 2 == 0 { Some(x) } else { None } +} + +fn main() { + let _ = vec![Some(1)].into_iter().flatten(); + //~^ HELP: consider using `flatten` instead + let _ = vec![Some(1)].into_iter().flatten(); + //~^ HELP: consider using `flatten` instead +} diff --git a/tests/ui/iter_filter_is_some.rs b/tests/ui/iter_filter_is_some.rs new file mode 100644 index 00000000000..5cc457efed5 --- /dev/null +++ b/tests/ui/iter_filter_is_some.rs @@ -0,0 +1,12 @@ +#![warn(clippy::iter_filter_is_some)] + +fn odds_out(x: i32) -> Option { + if x % 2 == 0 { Some(x) } else { None } +} + +fn main() { + let _ = vec![Some(1)].into_iter().filter(Option::is_some); + //~^ HELP: consider using `flatten` instead + let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()); + //~^ HELP: consider using `flatten` instead +} diff --git a/tests/ui/iter_filter_is_some.stderr b/tests/ui/iter_filter_is_some.stderr new file mode 100644 index 00000000000..2b1c1cdf7ef --- /dev/null +++ b/tests/ui/iter_filter_is_some.stderr @@ -0,0 +1,17 @@ +error: `filter` for `is_some` on iterator over `Option` + --> $DIR/iter_filter_is_some.rs:8:39 + | +LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` + | + = note: `-D clippy::iter-filter-is-some` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::iter_filter_is_some)]` + +error: `filter` for `is_some` on iterator over `Option` + --> $DIR/iter_filter_is_some.rs:10:39 + | +LL | let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` + +error: aborting due to 2 previous errors +