about summary refs log tree commit diff
path: root/clippy_lints/src/methods
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2023-12-26 15:41:40 +0000
committerbors <bors@rust-lang.org>2023-12-26 15:41:40 +0000
commit9dd2252b2c78013425816e6b288977bfafb1b659 (patch)
tree561ceedd44281b69c8060cbc2f3f142e0181c8d1 /clippy_lints/src/methods
parent99c783843dbbdc0ff1af984666341544c962845a (diff)
parent85e1646afcf4010b8bb74b64762f247e73912884 (diff)
downloadrust-9dd2252b2c78013425816e6b288977bfafb1b659.tar.gz
rust-9dd2252b2c78013425816e6b288977bfafb1b659.zip
Auto merge of #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 #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`]
[#12004](https://github.com/rust-lang/rust/pull/12004)
changelog: New Lint: [`iter_filter_is_ok`]
[#12004](https://github.com/rust-lang/rust/pull/12004)
Diffstat (limited to 'clippy_lints/src/methods')
-rw-r--r--clippy_lints/src/methods/filter_map.rs1
-rw-r--r--clippy_lints/src/methods/iter_filter.rs87
-rw-r--r--clippy_lints/src/methods/mod.rs78
3 files changed, 163 insertions, 3 deletions
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..ade8e3155fa
--- /dev/null
+++ b/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,
+        );
+    }
+}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index a94524b3ba6..25b1ea526e2 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(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(