Auto merge of rust-lang#12004 - PartiallyTyped:11843, r=xFrednet

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.

Fixes rust-lang#11843

PS, I also made some minor documentations fixes in a case where a double tick (`) was included.

---

changelog: New Lint: [`iter_filter_is_some`]
[rust-lang#12004](rust-lang#12004)
changelog: New Lint: [`iter_filter_is_ok`]
[rust-lang#12004](rust-lang#12004)

Author: bors

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

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 }

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,

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);

clippy_lints/src/methods/iter_filter.rs

@@ -0,0 +1,87 @@
+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, span_contains_comment};
+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_contains_comment(cx.sess().source_map(), filter_span.with_hi(expr.span.hi()))
+    {
+        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::HasPlaceholders,
+        );
+    }
+    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,
+            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::HasPlaceholders,
+        );
+    }
+}

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(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
+    /// 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,
+    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 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
+    /// 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::<i32, String>(1)].into_iter().filter(Result::is_ok);
+    ///
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// // example code which does not raise clippy warning
+    /// vec![Ok::<i32, String>(1)].into_iter().flatten();
+    /// ```
+    #[clippy::version = "1.76.0"]
+    pub ITER_FILTER_IS_OK,
+    pedantic,
+    "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,24 @@ 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,
+                        );
+                    }
+                    if self.msrv.meets(msrvs::ITER_FLATTEN) {
+                        // 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) {
                         // if `arg` has side-effect, the semantic will change
                         iter_overeager_cloned::check(

tests/ui/iter_filter_is_ok.fixed

@@ -0,0 +1,26 @@
+#![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
+
+    #[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| {
+        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()
+    });
+}

tests/ui/iter_filter_is_ok.rs

@@ -0,0 +1,26 @@
+#![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
+
+    #[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| {
+        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()
+    });
+}

tests/ui/iter_filter_is_ok.stderr

@@ -0,0 +1,23 @@
+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: `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
+

tests/ui/iter_filter_is_some.fixed

@@ -0,0 +1,27 @@
+#![warn(clippy::iter_filter_is_some)]
+
+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
+
+    #[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,
+        // `Err` is not an `Option`,
+        // and this doesn't ryme
+        o.is_some()
+    });
+}