From 25b9ca3f64bd3f7fda96e50f340c9c3459fe2e0a Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Wed, 20 Dec 2023 22:49:33 +0200 Subject: [PATCH 1/4] New lints `iter_filter_is_some` and `iter_filter_is_ok` Adds a pair of lints that check for cases of an iterator over `Result` and `Option` followed by `filter` without being followed by `map` as that is covered already by a different, specialized lint. changelog: New Lint: [`iter_filter_is_some`] changelog: New Lint: [`iter_filter_is_ok`] --- CHANGELOG.md | 2 + clippy_lints/src/declared_lints.rs | 2 + clippy_lints/src/methods/filter_map.rs | 1 + clippy_lints/src/methods/iter_filter.rs | 79 +++++++++++++++++++++++++ clippy_lints/src/methods/mod.rs | 76 +++++++++++++++++++++++- tests/ui/iter_filter_is_ok.fixed | 8 +++ tests/ui/iter_filter_is_ok.rs | 8 +++ tests/ui/iter_filter_is_ok.stderr | 17 ++++++ tests/ui/iter_filter_is_some.fixed | 12 ++++ tests/ui/iter_filter_is_some.rs | 12 ++++ tests/ui/iter_filter_is_some.stderr | 17 ++++++ 11 files changed, 231 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/methods/iter_filter.rs create mode 100644 tests/ui/iter_filter_is_ok.fixed create mode 100644 tests/ui/iter_filter_is_ok.rs create mode 100644 tests/ui/iter_filter_is_ok.stderr create mode 100644 tests/ui/iter_filter_is_some.fixed create mode 100644 tests/ui/iter_filter_is_some.rs create mode 100644 tests/ui/iter_filter_is_some.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb4eb17d46..9318c85ced97 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 38fda36c58ac..a00e2d3e0445 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -369,6 +369,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ 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 a8ca82d20e79..7cfd3d346b63 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 000000000000..04dc4820f6a5 --- /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 a94524b3ba6b..d67fa28b30b8 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -38,6 +38,7 @@ mod into_iter_on_ref; 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! { 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! { 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 @@ declare_clippy_lint! { "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 @@ impl_lint_pass!(Methods => [ 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 @@ impl Methods { 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 000000000000..9985877a02af --- /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 000000000000..176ce9026378 --- /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 000000000000..a5d7da977ff7 --- /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 000000000000..3732a8e72d19 --- /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 000000000000..5cc457efed53 --- /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 000000000000..2b1c1cdf7efc --- /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 + From 4b1ac31c18be2e7c8175c9a66bf45a038e76ee6e Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Sun, 24 Dec 2023 14:40:14 +0200 Subject: [PATCH 2/4] Added MSRV and more tests, improved wording --- clippy_config/src/msrvs.rs | 1 + clippy_lints/src/methods/iter_filter.rs | 26 +++++++++++++++++++------ clippy_lints/src/methods/mod.rs | 15 ++++++++------ tests/ui/iter_filter_is_ok.fixed | 14 +++++++++++++ tests/ui/iter_filter_is_ok.rs | 14 +++++++++++++ tests/ui/iter_filter_is_some.fixed | 18 +++++++++++++---- tests/ui/iter_filter_is_some.rs | 18 +++++++++++++---- tests/ui/iter_filter_is_some.stderr | 4 ++-- 8 files changed, 88 insertions(+), 22 deletions(-) diff --git a/clippy_config/src/msrvs.rs b/clippy_config/src/msrvs.rs index b3ef666e3063..8dec477cfdd6 100644 --- a/clippy_config/src/msrvs.rs +++ b/clippy_config/src/msrvs.rs @@ -41,6 +41,7 @@ msrv_aliases! { 1,35,0 { OPTION_COPIED, RANGE_CONTAINS } 1,34,0 { TRY_FROM } 1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES } + 1,29,0 { ITER_FLATTEN } 1,28,0 { FROM_BOOL } 1,27,0 { ITERATOR_TRY_FOLD } 1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN } diff --git a/clippy_lints/src/methods/iter_filter.rs b/clippy_lints/src/methods/iter_filter.rs index 04dc4820f6a5..f80bd2e2707b 100644 --- a/clippy_lints/src/methods/iter_filter.rs +++ b/clippy_lints/src/methods/iter_filter.rs @@ -1,10 +1,10 @@ -use rustc_lint::LateContext; +use rustc_lint::{LateContext, LintContext}; 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 clippy_utils::{is_trait_method, peel_blocks, span_contains_comment}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def::Res; @@ -54,7 +54,14 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir 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)) { + if is_iterator + && parent_is_not_map + && is_method(cx, filter_arg, sym!(is_some)) + && !span_contains_comment( + cx.sess().source_map(), + filter_span.with_hi(expr.span.hi()) + ) + { span_lint_and_sugg( cx, ITER_FILTER_IS_SOME, @@ -62,10 +69,17 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir "`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, + Applicability::HasPlaceholders, ); } - if is_iterator && parent_is_not_map && is_method(cx, filter_arg, sym!(is_ok)) { + if is_iterator + && parent_is_not_map + && is_method(cx, filter_arg, sym!(is_ok)) + && !span_contains_comment( + cx.sess().source_map(), + filter_span.with_hi(expr.span.hi()) + ) + { span_lint_and_sugg( cx, ITER_FILTER_IS_OK, @@ -73,7 +87,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir "`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, + Applicability::HasPlaceholders, ); } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index d67fa28b30b8..7a28f10ea87e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3779,8 +3779,8 @@ declare_clippy_lint! { 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. + /// Checks for usage of `.filter(Option::is_some)` that may be replaced with a `.flatten()` call. + /// This lint will require additional changes to the follow-up calls as it appects the type. /// /// ### Why is this bad? /// This pattern is often followed by manual unwrapping of the `Option`. The simplification @@ -3799,14 +3799,14 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.76.0"] pub ITER_FILTER_IS_SOME, - complexity, + pedantic, "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. + /// This lint will require additional changes to the follow-up calls as it appects the type. /// /// ### Why is this bad? /// This pattern is often followed by manual unwrapping of `Result`. The simplification @@ -3825,7 +3825,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.76.0"] pub ITER_FILTER_IS_OK, - complexity, + pedantic, "filtering an iterator over `Result`s for `Ok` can be achieved with `flatten`" } @@ -4324,8 +4324,11 @@ impl Methods { false, ); } + if self.msrv.meets(msrvs::ITER_FLATTEN) { - iter_filter::check(cx, expr, arg, span); + // use the sourcemap to get the span of the closure + iter_filter::check(cx, expr, arg, span); + } }, ("find", [arg]) => { if let Some(("cloned", recv2, [], _span2, _)) = method_call(recv) { diff --git a/tests/ui/iter_filter_is_ok.fixed b/tests/ui/iter_filter_is_ok.fixed index 9985877a02af..acfd097675c9 100644 --- a/tests/ui/iter_filter_is_ok.fixed +++ b/tests/ui/iter_filter_is_ok.fixed @@ -5,4 +5,18 @@ fn main() { //~^ HELP: consider using `flatten` instead let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten(); //~^ HELP: consider using `flatten` instead + + // Don't lint below + let mut counter = 0; + let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { + counter += 1; + o.is_ok() + }); + let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { + // Roses are red, + // Violets are blue, + // `Err` is not an `Option`, + // and this doesn't ryme + o.is_ok() + }); } diff --git a/tests/ui/iter_filter_is_ok.rs b/tests/ui/iter_filter_is_ok.rs index 176ce9026378..f13a28ad0e3c 100644 --- a/tests/ui/iter_filter_is_ok.rs +++ b/tests/ui/iter_filter_is_ok.rs @@ -5,4 +5,18 @@ fn main() { //~^ 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 + + // Don't lint below + let mut counter = 0; + let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { + counter += 1; + o.is_ok() + }); + let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { + // Roses are red, + // Violets are blue, + // `Err` is not an `Option`, + // and this doesn't ryme + o.is_ok() + }); } diff --git a/tests/ui/iter_filter_is_some.fixed b/tests/ui/iter_filter_is_some.fixed index 3732a8e72d19..f70f54b622e3 100644 --- a/tests/ui/iter_filter_is_some.fixed +++ b/tests/ui/iter_filter_is_some.fixed @@ -1,12 +1,22 @@ #![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 + + // Don't lint below + let mut counter = 0; + let _ = vec![Some(1)].into_iter().filter(|o| { + counter += 1; + o.is_some() + }); + let _ = vec![Some(1)].into_iter().filter(|o| { + // Roses are red, + // Violets are blue, + // `Err` is not an `Option`, + // and this doesn't ryme + o.is_some() + }); } diff --git a/tests/ui/iter_filter_is_some.rs b/tests/ui/iter_filter_is_some.rs index 5cc457efed53..18d450e8a986 100644 --- a/tests/ui/iter_filter_is_some.rs +++ b/tests/ui/iter_filter_is_some.rs @@ -1,12 +1,22 @@ #![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 + + // Don't lint below + let mut counter = 0; + let _ = vec![Some(1)].into_iter().filter(|o| { + counter += 1; + o.is_some() + }); + let _ = vec![Some(1)].into_iter().filter(|o| { + // Roses are red, + // Violets are blue, + // `Err` is not an `Option`, + // and this doesn't ryme + o.is_some() + }); } diff --git a/tests/ui/iter_filter_is_some.stderr b/tests/ui/iter_filter_is_some.stderr index 2b1c1cdf7efc..a9ef1d547a95 100644 --- a/tests/ui/iter_filter_is_some.stderr +++ b/tests/ui/iter_filter_is_some.stderr @@ -1,5 +1,5 @@ error: `filter` for `is_some` on iterator over `Option` - --> $DIR/iter_filter_is_some.rs:8:39 + --> $DIR/iter_filter_is_some.rs:4:39 | LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some); | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` @@ -8,7 +8,7 @@ LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some); = 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 + --> $DIR/iter_filter_is_some.rs:6:39 | LL | let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()); | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` From 2177f2cb9699a410302d474d7baf3e6d546530e9 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Sun, 24 Dec 2023 14:42:18 +0200 Subject: [PATCH 3/4] formatting --- clippy_lints/src/methods/iter_filter.rs | 10 ++-------- clippy_lints/src/methods/mod.rs | 1 - 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/iter_filter.rs b/clippy_lints/src/methods/iter_filter.rs index f80bd2e2707b..ade8e3155fae 100644 --- a/clippy_lints/src/methods/iter_filter.rs +++ b/clippy_lints/src/methods/iter_filter.rs @@ -57,10 +57,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir if is_iterator && parent_is_not_map && is_method(cx, filter_arg, sym!(is_some)) - && !span_contains_comment( - cx.sess().source_map(), - filter_span.with_hi(expr.span.hi()) - ) + && !span_contains_comment(cx.sess().source_map(), filter_span.with_hi(expr.span.hi())) { span_lint_and_sugg( cx, @@ -75,10 +72,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_arg: &hir if is_iterator && parent_is_not_map && is_method(cx, filter_arg, sym!(is_ok)) - && !span_contains_comment( - cx.sess().source_map(), - filter_span.with_hi(expr.span.hi()) - ) + && !span_contains_comment(cx.sess().source_map(), filter_span.with_hi(expr.span.hi())) { span_lint_and_sugg( cx, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7a28f10ea87e..25b1ea526e2e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -4325,7 +4325,6 @@ impl Methods { ); } if self.msrv.meets(msrvs::ITER_FLATTEN) { - // use the sourcemap to get the span of the closure iter_filter::check(cx, expr, arg, span); } From 85e1646afcf4010b8bb74b64762f247e73912884 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Sun, 24 Dec 2023 17:32:00 +0200 Subject: [PATCH 4/4] more tests --- tests/ui/iter_filter_is_ok.fixed | 4 ++++ tests/ui/iter_filter_is_ok.rs | 4 ++++ tests/ui/iter_filter_is_ok.stderr | 8 +++++++- tests/ui/iter_filter_is_some.fixed | 5 +++++ tests/ui/iter_filter_is_some.rs | 5 +++++ tests/ui/iter_filter_is_some.stderr | 8 +++++++- 6 files changed, 32 insertions(+), 2 deletions(-) diff --git a/tests/ui/iter_filter_is_ok.fixed b/tests/ui/iter_filter_is_ok.fixed index acfd097675c9..a5ca41528afd 100644 --- a/tests/ui/iter_filter_is_ok.fixed +++ b/tests/ui/iter_filter_is_ok.fixed @@ -6,6 +6,10 @@ fn main() { let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().flatten(); //~^ HELP: consider using `flatten` instead + #[rustfmt::skip] + let _ = vec![Ok(1), Err(2)].into_iter().flatten(); + //~^ HELP: consider using `flatten` instead + // Don't lint below let mut counter = 0; let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { diff --git a/tests/ui/iter_filter_is_ok.rs b/tests/ui/iter_filter_is_ok.rs index f13a28ad0e3c..e4e73f5ada1c 100644 --- a/tests/ui/iter_filter_is_ok.rs +++ b/tests/ui/iter_filter_is_ok.rs @@ -6,6 +6,10 @@ fn main() { let _ = vec![Ok(1), Err(2), Ok(3)].into_iter().filter(|a| a.is_ok()); //~^ HELP: consider using `flatten` instead + #[rustfmt::skip] + let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { o.is_ok() }); + //~^ HELP: consider using `flatten` instead + // Don't lint below let mut counter = 0; let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { diff --git a/tests/ui/iter_filter_is_ok.stderr b/tests/ui/iter_filter_is_ok.stderr index a5d7da977ff7..f3acbe38d8a8 100644 --- a/tests/ui/iter_filter_is_ok.stderr +++ b/tests/ui/iter_filter_is_ok.stderr @@ -13,5 +13,11 @@ error: `filter` for `is_ok` on iterator over `Result`s 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 +error: `filter` for `is_ok` on iterator over `Result`s + --> $DIR/iter_filter_is_ok.rs:10:45 + | +LL | let _ = vec![Ok(1), Err(2)].into_iter().filter(|o| { o.is_ok() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` + +error: aborting due to 3 previous errors diff --git a/tests/ui/iter_filter_is_some.fixed b/tests/ui/iter_filter_is_some.fixed index f70f54b622e3..c3fa93f0ab21 100644 --- a/tests/ui/iter_filter_is_some.fixed +++ b/tests/ui/iter_filter_is_some.fixed @@ -6,12 +6,17 @@ fn main() { let _ = vec![Some(1)].into_iter().flatten(); //~^ HELP: consider using `flatten` instead + #[rustfmt::skip] + let _ = vec![Some(1)].into_iter().flatten(); + //~^ HELP: consider using `flatten` instead + // Don't lint below let mut counter = 0; let _ = vec![Some(1)].into_iter().filter(|o| { counter += 1; o.is_some() }); + let _ = vec![Some(1)].into_iter().filter(|o| { // Roses are red, // Violets are blue, diff --git a/tests/ui/iter_filter_is_some.rs b/tests/ui/iter_filter_is_some.rs index 18d450e8a986..b023776abe46 100644 --- a/tests/ui/iter_filter_is_some.rs +++ b/tests/ui/iter_filter_is_some.rs @@ -6,12 +6,17 @@ fn main() { let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()); //~^ HELP: consider using `flatten` instead + #[rustfmt::skip] + let _ = vec![Some(1)].into_iter().filter(|o| { o.is_some() }); + //~^ HELP: consider using `flatten` instead + // Don't lint below let mut counter = 0; let _ = vec![Some(1)].into_iter().filter(|o| { counter += 1; o.is_some() }); + let _ = vec![Some(1)].into_iter().filter(|o| { // Roses are red, // Violets are blue, diff --git a/tests/ui/iter_filter_is_some.stderr b/tests/ui/iter_filter_is_some.stderr index a9ef1d547a95..1f2b10036fef 100644 --- a/tests/ui/iter_filter_is_some.stderr +++ b/tests/ui/iter_filter_is_some.stderr @@ -13,5 +13,11 @@ error: `filter` for `is_some` on iterator over `Option` 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 +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 3 previous errors