Add manual_option_zip lint (a.and_then(|x| b.map(|y| (x, y)))) (#16600)

*[View all
comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/16600)*

changelog: [`manual_option_zip`]: new lint that detects `a.and_then(|a|
b.map(|b| (a, b)))` and suggests using `a.zip(b)` instead

This PR introduces a new lint `manual_option_zip` that identifies cases
where `Option::and_then` is followed by `Option::map` in the provided
closure to construct a tuple that could be more concisely shaped using
`Option::zip`.

The lint detects the pattern:
```rust
a.and_then(|x| b.map(|y| (x, y)))
```

And suggests replacing it with:
```rust
a.zip(b)
```

Closes #16599

Author: llogiq

CHANGELOG.md

@@ -6801,6 +6801,7 @@ Released 2018-09-13
 [`manual_ok_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_err
 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
 [`manual_option_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_option_as_slice
+[`manual_option_zip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_option_zip
 [`manual_pattern_char_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_pattern_char_comparison
 [`manual_pop_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_pop_if
 [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

clippy_lints/src/declared_lints.rs

@@ -419,6 +419,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::methods::MANUAL_IS_VARIANT_AND_INFO,
     crate::methods::MANUAL_NEXT_BACK_INFO,
     crate::methods::MANUAL_OK_OR_INFO,
+    crate::methods::MANUAL_OPTION_ZIP_INFO,
     crate::methods::MANUAL_REPEAT_N_INFO,
     crate::methods::MANUAL_SATURATING_ARITHMETIC_INFO,
     crate::methods::MANUAL_SPLIT_ONCE_INFO,

clippy_lints/src/loops/manual_memcpy.rs

@@ -385,8 +385,8 @@ fn get_details_from_idx<'tcx>(
         ExprKind::Binary(op, lhs, rhs) => match op.node {
             BinOpKind::Add => {
                 let offset_opt = get_start(lhs, starts)
-                    .and_then(|s| get_offset(cx, rhs, starts).map(|o| (s, o)))
-                    .or_else(|| get_start(rhs, starts).and_then(|s| get_offset(cx, lhs, starts).map(|o| (s, o))));
+                    .zip(get_offset(cx, rhs, starts))
+                    .or_else(|| get_start(rhs, starts).zip(get_offset(cx, lhs, starts)));
 
                 offset_opt.map(|(s, o)| (s, Offset::positive(o)))
             },

clippy_lints/src/methods/manual_option_zip.rs

@@ -0,0 +1,89 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::msrvs::{self, Msrv};
+use clippy_utils::peel_blocks;
+use clippy_utils::res::{MaybeDef, MaybeResPath};
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::visitors::for_each_expr_without_closures;
+use rustc_errors::Applicability;
+use rustc_hir::{self as hir, Expr, ExprKind, HirId, PatKind};
+use rustc_lint::LateContext;
+use rustc_span::symbol::sym;
+use std::ops::ControlFlow;
+
+use super::MANUAL_OPTION_ZIP;
+
+/// Checks for `a.and_then(|a| b.map(|b| (a, b)))` and suggests `a.zip(b)`.
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'_>,
+    arg: &'tcx Expr<'_>,
+    msrv: Msrv,
+) {
+    // Looking for: `a.and_then(|a| b.map(|b| (a, b)))`.
+    // `and_then(|a| ...)`
+    if let ExprKind::Closure(&hir::Closure { body: outer_body_id, .. }) = arg.kind
+        && let hir::Body { params: [outer_param], value: outer_value, .. } = cx.tcx.hir_body(outer_body_id)
+        && let PatKind::Binding(_, outer_param_id, _, None) = outer_param.pat.kind
+        && cx.typeck_results().expr_ty(recv).is_diag_item(cx, sym::Option)
+        // `b.map(|b| ...)`
+        && let ExprKind::MethodCall(method_path, map_recv, [map_arg], _) = peel_blocks(outer_value).kind
+        && method_path.ident.name == sym::map
+        && cx.typeck_results().expr_ty(map_recv).is_diag_item(cx, sym::Option)
+        // `b` does not reference the outer closure parameter `a`.
+        && for_each_expr_without_closures(map_recv, |e| {
+            if e.res_local_id() == Some(outer_param_id) {
+                ControlFlow::Break(())
+            } else {
+                ControlFlow::Continue(())
+            }
+        }).is_none()
+        // `|b| (a, b)`
+        && let ExprKind::Closure(&hir::Closure { body: inner_body_id, .. }) = map_arg.kind
+        && let hir::Body { params: [inner_param], value: inner_value, .. } = cx.tcx.hir_body(inner_body_id)
+        && let PatKind::Binding(_, inner_param_id, _, None) = inner_param.pat.kind
+        // `(a, b)` or `(b, a)` — tuple of outer and inner param in either order.
+        && let ExprKind::Tup([first, second]) = peel_blocks(inner_value).kind
+        && let Some((zip_recv, zip_arg)) = zip_operands(first, second, outer_param_id, inner_param_id, recv, map_recv)
+        // `Option.zip()` is available.
+        && msrv.meets(cx, msrvs::OPTION_ZIP)
+    {
+        let mut applicability = Applicability::MachineApplicable;
+        let zip_recv_snip = snippet_with_applicability(cx, zip_recv.span, "_", &mut applicability);
+        let zip_arg_snip = snippet_with_applicability(cx, zip_arg.span, "_", &mut applicability);
+        let suggestion = format!("{zip_recv_snip}.zip({zip_arg_snip})");
+
+        span_lint_and_sugg(
+            cx,
+            MANUAL_OPTION_ZIP,
+            expr.span,
+            "manual implementation of `Option::zip`",
+            "use",
+            suggestion,
+            applicability,
+        );
+    }
+}
+
+/// Given the two tuple elements and the `and_then` receiver / `map` receiver, returns the
+/// `(zip_receiver, zip_argument)` expressions for the `.zip()` suggestion.
+///
+/// For `(outer, inner)` order the zip is `recv.zip(map_recv)`.
+/// For `(inner, outer)` (reversed) the zip is `map_recv.zip(recv)`.
+/// Returns `None` if the tuple elements don't match either order.
+fn zip_operands<'a>(
+    first: &Expr<'_>,
+    second: &Expr<'_>,
+    outer_param_id: HirId,
+    inner_param_id: HirId,
+    recv: &'a Expr<'a>,
+    map_recv: &'a Expr<'a>,
+) -> Option<(&'a Expr<'a>, &'a Expr<'a>)> {
+    if first.res_local_id() == Some(outer_param_id) && second.res_local_id() == Some(inner_param_id) {
+        Some((recv, map_recv))
+    } else if first.res_local_id() == Some(inner_param_id) && second.res_local_id() == Some(outer_param_id) {
+        Some((map_recv, recv))
+    } else {
+        None
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -63,6 +63,7 @@ mod manual_inspect;
 mod manual_is_variant_and;
 mod manual_next_back;
 mod manual_ok_or;
+mod manual_option_zip;
 mod manual_repeat_n;
 mod manual_saturating_arithmetic;
 mod manual_str_repeat;
@@ -1950,6 +1951,34 @@ declare_clippy_lint! {
     "finds patterns that can be encoded more concisely with `Option::ok_or`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `a.and_then(|a| b.map(|b| (a, b)))` which can be
+    /// more concisely expressed as `a.zip(b)`.
+    ///
+    /// ### Why is this bad?
+    /// `Option::zip` is more concise and directly expresses the intent of
+    /// combining two `Option` values into a tuple.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let a: Option<i32> = Some(1);
+    /// let b: Option<i32> = Some(2);
+    /// let _ = a.and_then(|x| b.map(|y| (x, y)));
+    /// ```
+    ///
+    /// Use instead:
+    /// ```no_run
+    /// let a: Option<i32> = Some(1);
+    /// let b: Option<i32> = Some(2);
+    /// let _ = a.zip(b);
+    /// ```
+    #[clippy::version = "1.95.0"]
+    pub MANUAL_OPTION_ZIP,
+    complexity,
+    "manual reimplementation of `Option::zip`"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     ///
@@ -4814,6 +4843,7 @@ impl_lint_pass!(Methods => [
     MANUAL_IS_VARIANT_AND,
     MANUAL_NEXT_BACK,
     MANUAL_OK_OR,
+    MANUAL_OPTION_ZIP,
     MANUAL_REPEAT_N,
     MANUAL_SATURATING_ARITHMETIC,
     MANUAL_SPLIT_ONCE,
@@ -5115,6 +5145,7 @@ impl Methods {
                     }
                 },
                 (sym::and_then, [arg]) => {
+                    manual_option_zip::check(cx, expr, recv, arg, self.msrv);
                     let biom_option_linted = bind_instead_of_map::check_and_then_some(cx, expr, recv, arg);
                     let biom_result_linted = bind_instead_of_map::check_and_then_ok(cx, expr, recv, arg);
                     if !biom_option_linted && !biom_result_linted {

clippy_utils/src/msrvs.rs

@@ -60,7 +60,7 @@ msrv_aliases! {
     1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS }
     1,50,0 { BOOL_THEN, CLAMP, SLICE_FILL }
     1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST }
-    1,46,0 { CONST_IF_MATCH }
+    1,46,0 { CONST_IF_MATCH, OPTION_ZIP }
     1,45,0 { STR_STRIP_PREFIX }
     1,43,0 { LOG2_10, LOG10_2, NUMERIC_ASSOCIATED_CONSTANTS }
     1,42,0 { MATCHES_MACRO, SLICE_PATTERNS, PTR_SLICE_RAW_PARTS }

tests/ui/manual_option_zip.fixed

@@ -0,0 +1,118 @@
+#![warn(clippy::manual_option_zip)]
+#![allow(clippy::bind_instead_of_map)]
+
+fn main() {}
+
+fn should_lint() {
+    // basic case
+    let a: Option<i32> = Some(1);
+    let b: Option<i32> = Some(2);
+    let _ = a.zip(b);
+    //~^ manual_option_zip
+
+    // different types
+    let a: Option<String> = Some(String::new());
+    let b: Option<i32> = Some(1);
+    let _ = a.zip(b);
+    //~^ manual_option_zip
+
+    // with None receiver
+    let b: Option<i32> = Some(2);
+    let _ = None::<i32>.zip(b);
+    //~^ manual_option_zip
+
+    // with function call as map receiver
+    let a: Option<i32> = Some(1);
+    let _ = a.zip(get_option());
+    //~^ manual_option_zip
+
+    // tuple order reversed: (inner, outer) instead of (outer, inner)
+    let a: Option<i32> = Some(1);
+    let b: Option<i32> = Some(2);
+    let _ = b.zip(a);
+    //~^ manual_option_zip
+
+    // closure bodies wrapped in blocks
+    let a: Option<i32> = Some(1);
+    let b: Option<i32> = Some(2);
+    #[rustfmt::skip]
+    let _ = a.zip(b);
+    //~^ manual_option_zip
+    #[rustfmt::skip]
+    let _ = a.zip(b);
+    //~^ manual_option_zip
+    #[rustfmt::skip]
+    let _ = a.zip(b);
+    //~^ manual_option_zip
+}
+
+fn should_not_lint() {
+    let a: Option<i32> = Some(1);
+    let b: Option<i32> = Some(2);
+
+    // tuple has more than 2 elements
+    let _ = a.and_then(|a| b.map(|b| (a, b, 1)));
+
+    // three-element tuple but with either `a` or `b` as the elements
+    let _ = a.and_then(|a| b.map(|b| (a, b, a)));
+
+    // inner closure body is not a simple tuple of the params
+    let _ = a.and_then(|a| b.map(|b| (a, b + 1)));
+
+    // map receiver uses the outer closure parameter
+    let _ = a.and_then(|a| a.checked_add(1).map(|b| (a, b)));
+
+    // .map receiver is not an Option type.
+    let _ = a.and_then(|a| NotOption(Some(1)).map(|b| (a, b)));
+
+    // .and_then receiver is not an Option type.
+    let _ = NotOption(Some(1)).and_then(|a| b.map(|b| (a, b)));
+
+    // closure body is not a map call
+    let a: Option<i32> = Some(1);
+    let _ = a.and_then(|a| Some((a, 1)));
+
+    // single-element tuple
+    let _ = a.and_then(|a| b.map(|_b| (a,)));
+
+    // the outer param used in the map receiver (cannot extract)
+    let opts: Vec<Option<i32>> = vec![Some(1), Some(2)];
+    let _ = a.and_then(|a| opts[a as usize].map(|b| (a, b)));
+
+    // extra statements in outer closure body
+    let _ = a.and_then(|a| {
+        let _x = 1;
+        b.map(|b| (a, b))
+    });
+
+    // extra statements in inner closure body
+    let _ = a.and_then(|a| {
+        b.map(|b| {
+            let _x = 1;
+            (a, b)
+        })
+    });
+
+    // n-ary zip where n > 2, which is out of scope for this lint (for now)
+    let c: Option<i32> = Some(3);
+    let _ = a.and_then(|a| b.and_then(|b| c.map(|c| (a, b, c))));
+
+    // not Option type (Result)
+    let a: Result<i32, &str> = Ok(1);
+    let b: Result<i32, &str> = Ok(2);
+    let _ = a.and_then(|a| b.map(|b| (a, b)));
+}
+
+fn get_option() -> Option<i32> {
+    Some(123)
+}
+
+struct NotOption(Option<i32>);
+impl NotOption {
+    fn map<U>(self, f: impl FnOnce(i32) -> U) -> Option<U> {
+        self.0.map(f)
+    }
+    fn and_then<U>(self, f: impl FnOnce(i32) -> Option<U>) -> Option<U> {
+        self.0.and_then(f)
+    }
+}