Fix manual_is_variant_and condition generation

When comparing `x.map(func) == Some(bool_lit)`, the value of `bool_lit` was
ignored, despite the fact that its value should determine the value of
the proposed expression.

`func` can be either a closure or a path. For the latter, η-expansion
will be used if needed to invert the result of the function call.

Author: samueltardieu

clippy_lints/src/methods/manual_is_variant_and.rs

@@ -1,14 +1,16 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::get_parent_expr;
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::source::{snippet, snippet_opt};
+use clippy_utils::source::{snippet, snippet_with_applicability};
+use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{get_parent_expr, sym};
+use rustc_ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
-use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
+use rustc_hir::{BinOpKind, Closure, Expr, ExprKind, QPath};
 use rustc_lint::LateContext;
 use rustc_middle::ty;
-use rustc_span::{BytePos, Span, sym};
+use rustc_span::Span;
 
 use super::MANUAL_IS_VARIANT_AND;
 
@@ -62,54 +64,147 @@ pub(super) fn check(
     );
 }
 
-fn emit_lint(cx: &LateContext<'_>, op: BinOpKind, parent: &Expr<'_>, method_span: Span, is_option: bool) {
-    if let Some(before_map_snippet) = snippet_opt(cx, parent.span.with_hi(method_span.lo()))
-        && let Some(after_map_snippet) = snippet_opt(cx, method_span.with_lo(method_span.lo() + BytePos(3)))
+#[derive(Clone, Copy, PartialEq)]
+enum Family {
+    Option,
+    Result,
+}
+
+#[derive(Clone, Copy, PartialEq)]
+enum Op {
+    Eq,
+    Ne,
+}
+
+impl TryFrom<BinOpKind> for Op {
+    type Error = ();
+    fn try_from(op: BinOpKind) -> Result<Self, Self::Error> {
+        match op {
+            BinOpKind::Eq => Ok(Self::Eq),
+            BinOpKind::Ne => Ok(Self::Ne),
+            _ => Err(()),
+        }
+    }
+}
+
+/// Represents the argument of the `.map()` function, as a closure or as a path
+/// in case η-reduction is used.
+enum MapFunc<'hir> {
+    Closure(&'hir Closure<'hir>),
+    Path(&'hir Expr<'hir>),
+}
+
+impl<'hir> TryFrom<&'hir Expr<'hir>> for MapFunc<'hir> {
+    type Error = ();
+
+    fn try_from(expr: &'hir Expr<'hir>) -> Result<Self, Self::Error> {
+        match expr.kind {
+            ExprKind::Closure(closure) => Ok(Self::Closure(closure)),
+            ExprKind::Path(_) => Ok(Self::Path(expr)),
+            _ => Err(()),
+        }
+    }
+}
+
+impl<'hir> MapFunc<'hir> {
+    /// Build a suggestion suitable for use in a `.map()`-like function. η-expansion will be applied
+    /// as needed.
+    fn sugg(self, cx: &LateContext<'hir>, invert: bool, app: &mut Applicability) -> String {
+        match self {
+            Self::Closure(closure) => {
+                let body = Sugg::hir_with_applicability(cx, cx.tcx.hir_body(closure.body).value, "..", app);
+                format!(
+                    "{} {}",
+                    snippet_with_applicability(cx, closure.fn_decl_span, "|..|", app),
+                    if invert { !body } else { body }
+                )
+            },
+            Self::Path(expr) => {
+                let path = snippet_with_applicability(cx, expr.span, "_", app);
+                if invert {
+                    format!("|x| !{path}(x)")
+                } else {
+                    path.to_string()
+                }
+            },
+        }
+    }
+}
+
+fn emit_lint<'tcx>(
+    cx: &LateContext<'tcx>,
+    span: Span,
+    op: Op,
+    family: Family,
+    in_some_or_ok: bool,
+    map_func: MapFunc<'tcx>,
+    recv: &Expr<'_>,
+) {
+    let mut app = Applicability::MachineApplicable;
+    let recv = snippet_with_applicability(cx, recv.span, "_", &mut app);
+
+    let (invert_expr, method, invert_body) = match (family, op, in_some_or_ok) {
+        (Family::Option, Op::Eq, bool_cst) => (false, "is_some_and", !bool_cst),
+        (Family::Option, Op::Ne, bool_cst) => (false, "is_none_or", bool_cst),
+        (Family::Result, Op::Eq, bool_cst) => (false, "is_ok_and", !bool_cst),
+        (Family::Result, Op::Ne, bool_cst) => (true, "is_ok_and", !bool_cst),
+    };
     {
         span_lint_and_sugg(
             cx,
             MANUAL_IS_VARIANT_AND,
-            parent.span,
+            span,
             format!(
                 "called `.map() {}= {}()`",
-                if op == BinOpKind::Eq { '=' } else { '!' },
-                if is_option { "Some" } else { "Ok" },
+                if op == Op::Eq { '=' } else { '!' },
+                if family == Family::Option { "Some" } else { "Ok" },
             ),
             "use",
-            if is_option && op == BinOpKind::Ne {
-                format!("{before_map_snippet}is_none_or{after_map_snippet}",)
-            } else {
-                format!(
-                    "{}{before_map_snippet}{}{after_map_snippet}",
-                    if op == BinOpKind::Eq { "" } else { "!" },
-                    if is_option { "is_some_and" } else { "is_ok_and" },
-                )
-            },
-            Applicability::MachineApplicable,
+            format!(
+                "{inversion}{recv}.{method}({body})",
+                inversion = if invert_expr { "!" } else { "" },
+                body = map_func.sugg(cx, invert_body, &mut app),
+            ),
+            app,
         );
-    }
+    };
 }
 
 pub(super) fn check_map(cx: &LateContext<'_>, expr: &Expr<'_>) {
     if let Some(parent_expr) = get_parent_expr(cx, expr)
         && let ExprKind::Binary(op, left, right) = parent_expr.kind
-        && matches!(op.node, BinOpKind::Eq | BinOpKind::Ne)
         && op.span.eq_ctxt(expr.span)
+        && let Ok(op) = Op::try_from(op.node)
     {
         // Check `left` and `right` expression in any order, and for `Option` and `Result`
         for (expr1, expr2) in [(left, right), (right, left)] {
             for item in [sym::Option, sym::Result] {
-                if let ExprKind::Call(call, ..) = expr1.kind
+                if let ExprKind::Call(call, [arg]) = expr1.kind
+                    && let ExprKind::Lit(lit) = arg.kind
+                    && let LitKind::Bool(bool_cst) = lit.node
                     && let ExprKind::Path(QPath::Resolved(_, path)) = call.kind
                     && let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), _) = path.res
                     && let ty = cx.typeck_results().expr_ty(expr1)
                     && let ty::Adt(adt, args) = ty.kind()
                     && cx.tcx.is_diagnostic_item(item, adt.did())
                     && args.type_at(0).is_bool()
-                    && let ExprKind::MethodCall(_, recv, _, span) = expr2.kind
+                    && let ExprKind::MethodCall(_, recv, [map_expr], _) = expr2.kind
                     && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), item)
+                    && let Ok(map_func) = MapFunc::try_from(map_expr)
                 {
-                    return emit_lint(cx, op.node, parent_expr, span, item == sym::Option);
+                    return emit_lint(
+                        cx,
+                        parent_expr.span,
+                        op,
+                        if item == sym::Option {
+                            Family::Option
+                        } else {
+                            Family::Result
+                        },
+                        bool_cst,
+                        map_func,
+                        recv,
+                    );
                 }
             }
         }

tests/ui/manual_is_variant_and.fixed

@@ -61,7 +61,7 @@ fn option_methods() {
 
     let _ = Some(2).is_some_and(|x| x % 2 == 0);
     //~^ manual_is_variant_and
-    let _ = Some(2).is_none_or(|x| x % 2 == 0);
+    let _ = Some(2).is_none_or(|x| x % 2 != 0);
     //~^ manual_is_variant_and
     let _ = Some(2).is_some_and(|x| x % 2 == 0);
     //~^ manual_is_variant_and
@@ -116,3 +116,113 @@ fn main() {
     option_methods();
     result_methods();
 }
+
+fn issue15202() {
+    let xs = [None, Some(b'_'), Some(b'1')];
+    for x in xs {
+        let a1 = x.is_none_or(|b| !b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = x.is_none_or(|b| !b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    for x in xs {
+        let a1 = x.is_none_or(|b| b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = x.is_none_or(|b| b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    for x in xs {
+        let a1 = x.is_some_and(|b| b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = x.is_some_and(|b| b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    for x in xs {
+        let a1 = x.is_some_and(|b| !b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = x.is_some_and(|b| !b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    let xs = [Err("foo"), Ok(b'_'), Ok(b'1')];
+    for x in xs {
+        let a1 = !x.is_ok_and(|b| b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = !x.is_ok_and(|b| b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    for x in xs {
+        let a1 = !x.is_ok_and(|b| !b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = !x.is_ok_and(|b| !b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    for x in xs {
+        let a1 = x.is_ok_and(|b| b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = x.is_ok_and(|b| b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+
+    for x in xs {
+        let a1 = x.is_ok_and(|b| !b.is_ascii_digit());
+        //~^ manual_is_variant_and
+        let a2 = x.is_ok_and(|b| !b.is_ascii_digit());
+        assert_eq!(a1, a2);
+    }
+}
+
+mod with_func {
+    fn iad(b: u8) -> bool {
+        b.is_ascii_digit()
+    }
+
+    fn check_option(b: Option<u8>) {
+        let a1 = b.is_some_and(iad);
+        //~^ manual_is_variant_and
+        let a2 = b.is_some_and(iad);
+        assert_eq!(a1, a2);
+
+        let a1 = b.is_some_and(|x| !iad(x));
+        //~^ manual_is_variant_and
+        let a2 = b.is_some_and(|x| !iad(x));
+        assert_eq!(a1, a2);
+
+        let a1 = b.is_none_or(|x| !iad(x));
+        //~^ manual_is_variant_and
+        let a2 = b.is_none_or(|x| !iad(x));
+        assert_eq!(a1, a2);
+
+        let a1 = b.is_none_or(iad);
+        //~^ manual_is_variant_and
+        let a2 = b.is_none_or(iad);
+        assert_eq!(a1, a2);
+    }
+
+    fn check_result(b: Result<u8, ()>) {
+        let a1 = b.is_ok_and(iad);
+        //~^ manual_is_variant_and
+        let a2 = b.is_ok_and(iad);
+        assert_eq!(a1, a2);
+
+        let a1 = b.is_ok_and(|x| !iad(x));
+        //~^ manual_is_variant_and
+        let a2 = b.is_ok_and(|x| !iad(x));
+        assert_eq!(a1, a2);
+
+        let a1 = !b.is_ok_and(iad);
+        //~^ manual_is_variant_and
+        let a2 = !b.is_ok_and(iad);
+        assert_eq!(a1, a2);
+
+        let a1 = !b.is_ok_and(|x| !iad(x));
+        //~^ manual_is_variant_and
+        let a2 = !b.is_ok_and(|x| !iad(x));
+        assert_eq!(a1, a2);
+    }
+}