Fix manual_is_variant_and condition generation (#15206)

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.

changelog: [`manual_is_variant_and`]: fix inverted suggestions that
could lead to code with different semantics

Fixes rust-lang/rust-clippy#15202

<!-- TRIAGEBOT_START -->

<!-- TRIAGEBOT_SUMMARY_START -->

### Summary Notes

-
[Beta-nomination](https://github.com/rust-lang/rust-clippy/pull/15206#issuecomment-3034006613)
by [samueltardieu](https://github.com/samueltardieu)

*Managed by `@rustbot`—see
[help](https://forge.rust-lang.org/triagebot/note.html) for details*

<!-- TRIAGEBOT_SUMMARY_END -->
<!-- TRIAGEBOT_END -->
This commit is contained in:
Timo 2025-07-09 21:04:18 +00:00 committed by GitHub
commit 012e5f538b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 454 additions and 36 deletions

View file

@ -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, Symbol};
use super::MANUAL_IS_VARIANT_AND;
@ -62,54 +64,154 @@ 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)))
{
span_lint_and_sugg(
cx,
MANUAL_IS_VARIANT_AND,
parent.span,
format!(
"called `.map() {}= {}()`",
if op == BinOpKind::Eq { '=' } else { '!' },
if is_option { "Some" } else { "Ok" },
),
"use",
if is_option && op == BinOpKind::Ne {
format!("{before_map_snippet}is_none_or{after_map_snippet}",)
} else {
#[derive(Clone, Copy, PartialEq)]
enum Flavor {
Option,
Result,
}
impl Flavor {
const fn symbol(self) -> Symbol {
match self {
Self::Option => sym::Option,
Self::Result => sym::Result,
}
}
const fn positive(self) -> Symbol {
match self {
Self::Option => sym::Some,
Self::Result => sym::Ok,
}
}
}
#[derive(Clone, Copy, PartialEq)]
enum Op {
Eq,
Ne,
}
impl std::fmt::Display for Op {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Eq => write!(f, "=="),
Self::Ne => write!(f, "!="),
}
}
}
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!(
"{}{before_map_snippet}{}{after_map_snippet}",
if op == BinOpKind::Eq { "" } else { "!" },
if is_option { "is_some_and" } else { "is_ok_and" },
"{} {}",
snippet_with_applicability(cx, closure.fn_decl_span, "|..|", app),
if invert { !body } else { body }
)
},
Applicability::MachineApplicable,
);
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,
flavor: Flavor,
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 (flavor, op) {
(Flavor::Option, Op::Eq) => (false, "is_some_and", !in_some_or_ok),
(Flavor::Option, Op::Ne) => (false, "is_none_or", in_some_or_ok),
(Flavor::Result, Op::Eq) => (false, "is_ok_and", !in_some_or_ok),
(Flavor::Result, Op::Ne) => (true, "is_ok_and", !in_some_or_ok),
};
span_lint_and_sugg(
cx,
MANUAL_IS_VARIANT_AND,
span,
format!("called `.map() {op} {pos}()`", pos = flavor.positive(),),
"use",
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
for flavor in [Flavor::Option, Flavor::Result] {
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())
&& cx.tcx.is_diagnostic_item(flavor.symbol(), adt.did())
&& args.type_at(0).is_bool()
&& let ExprKind::MethodCall(_, recv, _, span) = expr2.kind
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), item)
&& let ExprKind::MethodCall(_, recv, [map_expr], _) = expr2.kind
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), flavor.symbol())
&& 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, flavor, bool_cst, map_func, recv);
}
}
}

View file

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

View file

@ -125,3 +125,113 @@ fn main() {
option_methods();
result_methods();
}
fn issue15202() {
let xs = [None, Some(b'_'), Some(b'1')];
for x in xs {
let a1 = x.map(|b| b.is_ascii_digit()) != Some(true);
//~^ 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.map(|b| b.is_ascii_digit()) != Some(false);
//~^ 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.map(|b| b.is_ascii_digit()) == Some(true);
//~^ 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.map(|b| b.is_ascii_digit()) == Some(false);
//~^ 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.map(|b| b.is_ascii_digit()) != Ok(true);
//~^ 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.map(|b| b.is_ascii_digit()) != Ok(false);
//~^ 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.map(|b| b.is_ascii_digit()) == Ok(true);
//~^ 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.map(|b| b.is_ascii_digit()) == Ok(false);
//~^ 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.map(iad) == Some(true);
//~^ manual_is_variant_and
let a2 = b.is_some_and(iad);
assert_eq!(a1, a2);
let a1 = b.map(iad) == Some(false);
//~^ manual_is_variant_and
let a2 = b.is_some_and(|x| !iad(x));
assert_eq!(a1, a2);
let a1 = b.map(iad) != Some(true);
//~^ manual_is_variant_and
let a2 = b.is_none_or(|x| !iad(x));
assert_eq!(a1, a2);
let a1 = b.map(iad) != Some(false);
//~^ manual_is_variant_and
let a2 = b.is_none_or(iad);
assert_eq!(a1, a2);
}
fn check_result(b: Result<u8, ()>) {
let a1 = b.map(iad) == Ok(true);
//~^ manual_is_variant_and
let a2 = b.is_ok_and(iad);
assert_eq!(a1, a2);
let a1 = b.map(iad) == Ok(false);
//~^ manual_is_variant_and
let a2 = b.is_ok_and(|x| !iad(x));
assert_eq!(a1, a2);
let a1 = b.map(iad) != Ok(true);
//~^ manual_is_variant_and
let a2 = !b.is_ok_and(iad);
assert_eq!(a1, a2);
let a1 = b.map(iad) != Ok(false);
//~^ manual_is_variant_and
let a2 = !b.is_ok_and(|x| !iad(x));
assert_eq!(a1, a2);
}
}

View file

@ -54,7 +54,7 @@ error: called `.map() != Some()`
--> tests/ui/manual_is_variant_and.rs:70:13
|
LL | let _ = Some(2).map(|x| x % 2 == 0) != Some(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_none_or(|x| x % 2 == 0)`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_none_or(|x| x % 2 != 0)`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:72:13
@ -126,5 +126,101 @@ error: called `map(<f>).unwrap_or_default()` on a `Result` value
LL | let _ = res2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_ok_and(char::is_alphanumeric)`
error: aborting due to 15 previous errors
error: called `.map() != Some()`
--> tests/ui/manual_is_variant_and.rs:132:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) != Some(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `x.is_none_or(|b| !b.is_ascii_digit())`
error: called `.map() != Some()`
--> tests/ui/manual_is_variant_and.rs:139:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) != Some(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `x.is_none_or(|b| b.is_ascii_digit())`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:146:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) == Some(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `x.is_some_and(|b| b.is_ascii_digit())`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:153:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) == Some(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `x.is_some_and(|b| !b.is_ascii_digit())`
error: called `.map() != Ok()`
--> tests/ui/manual_is_variant_and.rs:161:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) != Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!x.is_ok_and(|b| b.is_ascii_digit())`
error: called `.map() != Ok()`
--> tests/ui/manual_is_variant_and.rs:168:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) != Ok(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!x.is_ok_and(|b| !b.is_ascii_digit())`
error: called `.map() == Ok()`
--> tests/ui/manual_is_variant_and.rs:175:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) == Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `x.is_ok_and(|b| b.is_ascii_digit())`
error: called `.map() == Ok()`
--> tests/ui/manual_is_variant_and.rs:182:18
|
LL | let a1 = x.map(|b| b.is_ascii_digit()) == Ok(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `x.is_ok_and(|b| !b.is_ascii_digit())`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:195:18
|
LL | let a1 = b.map(iad) == Some(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `b.is_some_and(iad)`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:200:18
|
LL | let a1 = b.map(iad) == Some(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `b.is_some_and(|x| !iad(x))`
error: called `.map() != Some()`
--> tests/ui/manual_is_variant_and.rs:205:18
|
LL | let a1 = b.map(iad) != Some(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `b.is_none_or(|x| !iad(x))`
error: called `.map() != Some()`
--> tests/ui/manual_is_variant_and.rs:210:18
|
LL | let a1 = b.map(iad) != Some(false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `b.is_none_or(iad)`
error: called `.map() == Ok()`
--> tests/ui/manual_is_variant_and.rs:217:18
|
LL | let a1 = b.map(iad) == Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^ help: use: `b.is_ok_and(iad)`
error: called `.map() == Ok()`
--> tests/ui/manual_is_variant_and.rs:222:18
|
LL | let a1 = b.map(iad) == Ok(false);
| ^^^^^^^^^^^^^^^^^^^^^^^ help: use: `b.is_ok_and(|x| !iad(x))`
error: called `.map() != Ok()`
--> tests/ui/manual_is_variant_and.rs:227:18
|
LL | let a1 = b.map(iad) != Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^ help: use: `!b.is_ok_and(iad)`
error: called `.map() != Ok()`
--> tests/ui/manual_is_variant_and.rs:232:18
|
LL | let a1 = b.map(iad) != Ok(false);
| ^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!b.is_ok_and(|x| !iad(x))`
error: aborting due to 31 previous errors