new lint: and_then_then_some

CHANGELOG.md

@@ -5199,6 +5199,7 @@ Released 2018-09-13
 [`almost_complete_letter_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_letter_range
 [`almost_complete_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_range
 [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped
+[`and_then_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#and_then_then_some
 [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant
 [`arc_with_non_send_sync`]: https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
 [`arithmetic_side_effects`]: https://rust-lang.github.io/rust-clippy/master/index.html#arithmetic_side_effects

clippy_lints/src/and_then_then_some.rs

@@ -0,0 +1,242 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::is_copy;
+use clippy_utils::visitors::for_each_expr;
+use clippy_utils::{fn_def_id, is_from_proc_macro, match_def_path};
+use rustc_errors::Applicability;
+use rustc_hir::def::Res;
+use rustc_hir::{Block, Body, Closure, Expr, ExprKind, HirId, Node, Param, Pat, Path, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::lint::in_external_macro;
+use rustc_session::declare_lint_pass;
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detects usage of `Option::and_then` and `bool::then_some` that could
+    /// be replaced with `Option::filter`.
+    ///
+    /// ### Why is this bad?
+    /// Needless complexity, uses recent and uncommon stdlib functions instead of
+    /// one older function.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let x = Some("foo".to_string());
+    /// let _y = x.clone().and_then(|v| v.starts_with('f').then_some(v));
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let x = Some("foo".to_string());
+    /// let _y = x.clone().filter(|v| v.starts_with('f'));
+    /// ```
+    #[clippy::version = "1.81.0"]
+    pub AND_THEN_THEN_SOME,
+    nursery,
+    "detects usage of `and_then` and `then_some` that can be replaced by `filter`"
+}
+
+// note: `Option::filter` is older than `bool::then_some`,
+// so no msrv check is required.
+declare_lint_pass!(AndThenThenSome => [AND_THEN_THEN_SOME]);
+
+impl<'tcx> LateLintPass<'tcx> for AndThenThenSome {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if in_external_macro(cx.tcx.sess, expr.span) {
+            return;
+        }
+        match expr.kind {
+            ExprKind::MethodCall(_, recv_or_self, [arg], _) | ExprKind::Call(_, [recv_or_self, arg]) => {
+                // we could check if type of reciever is diagnostic item Option,
+                // but we don't technically need to, since we're checking the path.
+                if is_and_then(cx, expr) {
+                    if let Some((closure_arg_span, closure_arg_hid, predicate)) = then_some_closure_arg(cx, arg) {
+                        let add_ref_pattern;
+                        // check if `closure_arg_hid` implements Copy,
+                        // if so, show suggestion but with `&` added to the
+                        // argument pattern.
+                        if is_copy(cx, cx.typeck_results().node_type(closure_arg_hid)) {
+                            add_ref_pattern = true;
+                        } else if contains_only_autoref_of(cx, closure_arg_hid, predicate) {
+                            add_ref_pattern = false;
+                        } else {
+                            return;
+                        }
+                        // this check is expensive, so we do it last.
+                        if is_from_proc_macro(cx, expr) {
+                            return;
+                        }
+                        show_sugg(
+                            cx,
+                            expr.span,
+                            recv_or_self,
+                            closure_arg_span,
+                            predicate,
+                            add_ref_pattern,
+                        );
+                    }
+                }
+            },
+            _ => {},
+        }
+    }
+}
+
+// This function returns the span and hir id of the closure arguments and the receiver of
+// `then_some` (usually `bool`) if the expression passed is a closure whose single expression is a
+// call to `then_some`.
+fn then_some_closure_arg<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option<(Span, HirId, &'tcx Expr<'tcx>)> {
+    match expr.kind {
+        ExprKind::Closure(Closure { body, .. }) => {
+            if let Node::Expr(expr) = cx.tcx.hir_node(body.hir_id)
+                && let Body {
+                    params:
+                        [
+                            Param {
+                                hir_id: arg_id,
+                                pat: Pat { span, .. },
+                                ..
+                            },
+                        ],
+                    ..
+                } = cx.tcx.hir().body(*body)
+            {
+                (peel_closure_body(cx, expr, *arg_id)).map(|body| (*span, *arg_id, body))
+            } else {
+                None
+            }
+        },
+        _ => None,
+    }
+}
+
+fn peel_closure_body<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    closure_arg_id: HirId,
+) -> Option<&'tcx Expr<'tcx>> {
+    match expr.kind {
+        ExprKind::Ret(Some(wrapped_expr)) =>
+        // duplicated blocks because 2023 reference statements are awkward.
+        // "&" peels multiple layers of indirection instead of just one like we want.
+        {
+            peel_closure_body(cx, wrapped_expr, closure_arg_id)
+        },
+        // it would be nice if we could lift { x; y.a() } into { x; y }.a()
+        ExprKind::Block(
+            Block {
+                stmts: [],
+                expr: Some(wrapped_expr),
+                ..
+            },
+            _,
+        ) => peel_closure_body(cx, wrapped_expr, closure_arg_id),
+        ExprKind::MethodCall(_, pred, [arg], _) | ExprKind::Call(_, [pred, arg]) => {
+            if is_then_some(cx, expr) && is_local_defined_at(cx, arg, closure_arg_id) {
+                Some(pred)
+            } else {
+                None
+            }
+        },
+        _ => None,
+    }
+}
+
+fn get_pat_hid(node: Node<'_>) -> Option<HirId> {
+    match node {
+        Node::Param(Param {
+            pat: Pat { hir_id, .. },
+            ..
+        })
+        | Node::Pat(Pat { hir_id, .. }) => Some(*hir_id),
+        _ => None,
+    }
+}
+
+fn is_local_defined_at(cx: &LateContext<'_>, local: &Expr<'_>, arg_hid: HirId) -> bool {
+    match local.kind {
+        ExprKind::Path(QPath::Resolved(
+            _,
+            Path {
+                res: Res::Local(local_hid),
+                ..
+            },
+        )) => {
+            let local_pat_id = get_pat_hid(cx.tcx.hir_node(*local_hid));
+            local_pat_id.is_some() && local_pat_id == get_pat_hid(cx.tcx.hir_node(arg_hid))
+        },
+        // is not local at all, so definitly isn't a local defined at the given position
+        _ => false,
+    }
+}
+
+fn show_sugg(
+    cx: &LateContext<'_>,
+    span: Span,
+    selfarg: &Expr<'_>,
+    closure_args: Span,
+    predicate: &Expr<'_>,
+    add_ref_pattern: bool,
+) {
+    let mut appl = Applicability::MachineApplicable;
+    let prefix = if add_ref_pattern { "&" } else { "" };
+    let sugg = format!(
+        "{}.filter(|{prefix}{}| {})",
+        snippet_with_applicability(cx, selfarg.span, "<OPTION>", &mut appl),
+        snippet_with_applicability(cx, closure_args, "<ARGS>", &mut appl),
+        snippet_with_applicability(cx, predicate.span, "<PREDICATE>", &mut appl)
+    );
+    span_lint_and_sugg(
+        cx,
+        AND_THEN_THEN_SOME,
+        span,
+        "use of `Option::and_then` and `bool::then_some` is equivelent to `filter`",
+        "use `Option::filter` instead",
+        sugg,
+        appl,
+    );
+}
+
+fn is_then_some(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let Some(def_id) = fn_def_id(cx, expr) {
+        match_def_path(cx, def_id, &["core", "bool", "<impl bool>", "then_some"])
+    } else {
+        false
+    }
+}
+
+fn is_and_then(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let Some(def_id) = fn_def_id(cx, expr) {
+        match_def_path(cx, def_id, &["core", "option", "Option", "and_then"])
+    } else {
+        false
+    }
+}
+
+/// checks that `expr` contains no references to `var` outside of autoref method calls.
+fn contains_only_autoref_of<'tcx>(cx: &LateContext<'tcx>, var: HirId, expr: &'tcx Expr<'tcx>) -> bool {
+    use std::ops::ControlFlow::{Break, Continue};
+    for_each_expr(cx, expr, |subexpr| {
+        if is_local_defined_at(cx, subexpr, var) {
+            match cx.tcx.hir().parent_iter(subexpr.hir_id).next() {
+                Some((
+                    _hir_id,
+                    Node::Expr(Expr {
+                        kind: ExprKind::MethodCall(_, _, _, _),
+                        ..
+                    }),
+                )) => {
+                    // TODO: check if type of method reciver has one more level
+                    // of indirection than `var`
+                    return Continue(());
+                },
+                non_method_call => {
+                    dbg!(non_method_call);
+                },
+            }
+            return Break(());
+        }
+        Continue(())
+    })
+    .is_none()
+}

clippy_lints/src/declared_lints.rs

@@ -39,6 +39,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::utils::internal_lints::unsorted_clippy_utils_paths::UNSORTED_CLIPPY_UTILS_PATHS_INFO,
     crate::absolute_paths::ABSOLUTE_PATHS_INFO,
     crate::almost_complete_range::ALMOST_COMPLETE_RANGE_INFO,
+    crate::and_then_then_some::AND_THEN_THEN_SOME_INFO,
     crate::approx_const::APPROX_CONSTANT_INFO,
     crate::arc_with_non_send_sync::ARC_WITH_NON_SEND_SYNC_INFO,
     crate::as_conversions::AS_CONVERSIONS_INFO,

clippy_lints/src/lib.rs

@@ -73,6 +73,7 @@ mod renamed_lints;
 // begin lints modules, do not remove this comment, it’s used in `update_lints`
 mod absolute_paths;
 mod almost_complete_range;
+mod and_then_then_some;
 mod approx_const;
 mod arc_with_non_send_sync;
 mod as_conversions;
@@ -1178,6 +1179,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(set_contains_or_insert::HashsetInsertAfterContains));
     store.register_early_pass(|| Box::new(byte_char_slices::ByteCharSlice));
     store.register_early_pass(|| Box::new(cfg_not_test::CfgNotTest));
+    store.register_late_pass(|_| Box::new(and_then_then_some::AndThenThenSome));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

tests/ui/and_then_then_some.fixed

@@ -0,0 +1,18 @@
+#![warn(clippy::and_then_then_some)]
+
+fn main() {
+    let x = Some("foo".to_string());
+
+    let _y = x.clone().filter(|v| v.starts_with('f'));
+
+    let _z = x.clone().filter(|v| v.starts_with('f'));
+    let _w = x.clone().filter(|v| v.starts_with('f'));
+    #[allow(clippy::needless_return)]
+    let _v = x.clone().filter(|v| v.starts_with('g'));
+    #[derive(Clone)]
+    struct NonCopy;
+    let a = Some(NonCopy);
+    // non-copy value, but it doesn't appear in the predicate.
+    let _a1 = a.clone().filter(|v| true);
+    let _ = Some(true).filter(|&v| v);
+}

tests/ui/and_then_then_some.rs

@@ -0,0 +1,18 @@
+#![warn(clippy::and_then_then_some)]
+
+fn main() {
+    let x = Some("foo".to_string());
+
+    let _y = x.clone().and_then(|v| v.starts_with('f').then_some(v));
+
+    let _z = x.clone().and_then(|v| bool::then_some(v.starts_with('f'), v));
+    let _w = Option::and_then(x.clone(), |v: String| bool::then_some(v.starts_with('f'), v));
+    #[allow(clippy::needless_return)]
+    let _v = x.clone().and_then(|v| return v.starts_with('g').then_some(v));
+    #[derive(Clone)]
+    struct NonCopy;
+    let a = Some(NonCopy);
+    // non-copy value, but it doesn't appear in the predicate.
+    let _a1 = a.clone().and_then(|v| true.then_some(v));
+    let _ = Some(true).and_then(|v| v.then_some(v));
+}

tests/ui/and_then_then_some.stderr

@@ -0,0 +1,41 @@
+error: use of `Option::and_then` and `bool::then_some` is equivelent to `filter`
+  --> tests/ui/and_then_then_some.rs:6:14
+   |
+LL |     let _y = x.clone().and_then(|v| v.starts_with('f').then_some(v));
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `Option::filter` instead: `x.clone().filter(|v| v.starts_with('f'))`
+   |
+   = note: `-D clippy::and-then-then-some` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::and_then_then_some)]`
+
+error: use of `Option::and_then` and `bool::then_some` is equivelent to `filter`
+  --> tests/ui/and_then_then_some.rs:8:14
+   |
+LL |     let _z = x.clone().and_then(|v| bool::then_some(v.starts_with('f'), v));
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `Option::filter` instead: `x.clone().filter(|v| v.starts_with('f'))`
+
+error: use of `Option::and_then` and `bool::then_some` is equivelent to `filter`
+  --> tests/ui/and_then_then_some.rs:9:14
+   |
+LL |     let _w = Option::and_then(x.clone(), |v: String| bool::then_some(v.starts_with('f'), v));
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `Option::filter` instead: `x.clone().filter(|v| v.starts_with('f'))`
+
+error: use of `Option::and_then` and `bool::then_some` is equivelent to `filter`
+  --> tests/ui/and_then_then_some.rs:11:14
+   |
+LL |     let _v = x.clone().and_then(|v| return v.starts_with('g').then_some(v));
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `Option::filter` instead: `x.clone().filter(|v| v.starts_with('g'))`
+
+error: use of `Option::and_then` and `bool::then_some` is equivelent to `filter`
+  --> tests/ui/and_then_then_some.rs:16:15
+   |
+LL |     let _a1 = a.clone().and_then(|v| true.then_some(v));
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `Option::filter` instead: `a.clone().filter(|v| true)`
+
+error: use of `Option::and_then` and `bool::then_some` is equivelent to `filter`
+  --> tests/ui/and_then_then_some.rs:17:13
+   |
+LL |     let _ = Some(true).and_then(|v| v.then_some(v));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `Option::filter` instead: `Some(true).filter(|&v| v)`
+
+error: aborting due to 6 previous errors
+