user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Extend manual_is_variant_and lint to check for boolean map comparisons (#14646)

Browse source 
Fixes rust-lang/rust-clippy#14542.

changelog: [`manual_is_variant_and`]: Extend to check for boolean map
comparisons

r? @samueltardieu
This commit is contained in:
Samuel Tardieu 2025-05-28 10:25:11 +00:00 • committed by GitHub
commit 551870df96
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 240 additions and 15 deletions
Download patch file Download diff file Expand all files Collapse all files

70
clippy_lints/src/methods/manual_is_variant_and.rs
View file

@ -1,18 +1,22 @@
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;
use clippy_utils::source::{snippet, snippet_opt};
use clippy_utils::ty::is_type_diagnostic_item;
use rustc_errors::Applicability;
use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
use rustc_lint::LateContext;
use rustc_span::{Span, sym};
use rustc_middle::ty;
use rustc_span::{BytePos, Span, sym};
use super::MANUAL_IS_VARIANT_AND;
pub(super) fn check<'tcx>(
pub(super) fn check(
cx: &LateContext<'_>,
expr: &'tcx rustc_hir::Expr<'_>,
map_recv: &'tcx rustc_hir::Expr<'_>,
map_arg: &'tcx rustc_hir::Expr<'_>,
expr: &Expr<'_>,
map_recv: &Expr<'_>,
map_arg: &Expr<'_>,
map_span: Span,
msrv: Msrv,
) {
@ -57,3 +61,57 @@ pub(super) fn check<'tcx>(
Applicability::MachineApplicable,
);
}
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 {
format!(
"{}{before_map_snippet}{}{after_map_snippet}",
if op == BinOpKind::Eq { "" } else { "!" },
if is_option { "is_some_and" } else { "is_ok_and" },
)
},
Applicability::MachineApplicable,
);
}
}
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)
{
// 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
&& 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
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), item)
{
return emit_lint(cx, op.node, parent_expr, span, item == sym::Option);
}
}
}
}
}

1
clippy_lints/src/methods/mod.rs
View file

@ -5203,6 +5203,7 @@ impl Methods {
unused_enumerate_index::check(cx, expr, recv, m_arg);
map_clone::check(cx, expr, recv, m_arg, self.msrv);
map_with_unused_argument_over_ranges::check(cx, expr, recv, m_arg, self.msrv, span);
manual_is_variant_and::check_map(cx, expr);
match method_call(recv) {
Some((map_name @ (sym::iter | sym::into_iter), recv2, _, _, _)) => {
iter_kv_map::check(cx, map_name, expr, recv2, m_arg, self.msrv);

62
tests/ui/manual_is_variant_and.fixed
View file

@ -4,6 +4,44 @@
#[macro_use]
extern crate option_helpers;
struct Foo<T>(T);
impl<T> Foo<T> {
fn map<F: FnMut(T) -> bool>(self, mut f: F) -> Option<bool> {
Some(f(self.0))
}
}
fn foo() -> Option<bool> {
Some(true)
}
macro_rules! some_true {
() => {
Some(true)
};
}
macro_rules! some_false {
() => {
Some(false)
};
}
macro_rules! mac {
(some $e:expr) => {
Some($e)
};
(some_map $e:expr) => {
Some($e).map(|x| x % 2 == 0)
};
(map $e:expr) => {
$e.map(|x| x % 2 == 0)
};
(eq $a:expr, $b:expr) => {
$a == $b
};
}
#[rustfmt::skip]
fn option_methods() {
let opt = Some(1);
@ -21,6 +59,15 @@ fn option_methods() {
let _ = opt
.is_some_and(|x| x > 1);
let _ = Some(2).is_some_and(|x| x % 2 == 0);
//~^ manual_is_variant_and
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
let _ = Some(2).is_none_or(|x| x % 2 == 0);
//~^ manual_is_variant_and
// won't fix because the return type of the closure is not `bool`
let _ = opt.map(|x| x + 1).unwrap_or_default();
@ -28,6 +75,14 @@ fn option_methods() {
let _ = opt2.is_some_and(char::is_alphanumeric); // should lint
//~^ manual_is_variant_and
let _ = opt_map!(opt2, |x| x == 'a').unwrap_or_default(); // should not lint
// Should not lint.
let _ = Foo::<u32>(0).map(|x| x % 2 == 0) == Some(true);
let _ = Some(2).map(|x| x % 2 == 0) != foo();
let _ = mac!(eq Some(2).map(|x| x % 2 == 0), Some(true));
let _ = mac!(some 2).map(|x| x % 2 == 0) == Some(true);
let _ = mac!(some_map 2) == Some(true);
let _ = mac!(map Some(2)) == Some(true);
}
#[rustfmt::skip]
@ -41,6 +96,13 @@ fn result_methods() {
});
let _ = res.is_ok_and(|x| x > 1);
let _ = Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0);
//~^ manual_is_variant_and
let _ = !Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0);
//~^ manual_is_variant_and
let _ = !Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0);
//~^ manual_is_variant_and
// won't fix because the return type of the closure is not `bool`
let _ = res.map(|x| x + 1).unwrap_or_default();

62
tests/ui/manual_is_variant_and.rs
View file

@ -4,6 +4,44 @@
#[macro_use]
extern crate option_helpers;
struct Foo<T>(T);
impl<T> Foo<T> {
fn map<F: FnMut(T) -> bool>(self, mut f: F) -> Option<bool> {
Some(f(self.0))
}
}
fn foo() -> Option<bool> {
Some(true)
}
macro_rules! some_true {
() => {
Some(true)
};
}
macro_rules! some_false {
() => {
Some(false)
};
}
macro_rules! mac {
(some $e:expr) => {
Some($e)
};
(some_map $e:expr) => {
Some($e).map(|x| x % 2 == 0)
};
(map $e:expr) => {
$e.map(|x| x % 2 == 0)
};
(eq $a:expr, $b:expr) => {
$a == $b
};
}
#[rustfmt::skip]
fn option_methods() {
let opt = Some(1);
@ -27,6 +65,15 @@ fn option_methods() {
//~^ manual_is_variant_and
.unwrap_or_default();
let _ = Some(2).map(|x| x % 2 == 0) == Some(true);
//~^ manual_is_variant_and
let _ = Some(2).map(|x| x % 2 == 0) != Some(true);
//~^ manual_is_variant_and
let _ = Some(2).map(|x| x % 2 == 0) == some_true!();
//~^ manual_is_variant_and
let _ = Some(2).map(|x| x % 2 == 0) != some_false!();
//~^ manual_is_variant_and
// won't fix because the return type of the closure is not `bool`
let _ = opt.map(|x| x + 1).unwrap_or_default();
@ -34,6 +81,14 @@ fn option_methods() {
let _ = opt2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
//~^ manual_is_variant_and
let _ = opt_map!(opt2, |x| x == 'a').unwrap_or_default(); // should not lint
// Should not lint.
let _ = Foo::<u32>(0).map(|x| x % 2 == 0) == Some(true);
let _ = Some(2).map(|x| x % 2 == 0) != foo();
let _ = mac!(eq Some(2).map(|x| x % 2 == 0), Some(true));
let _ = mac!(some 2).map(|x| x % 2 == 0) == Some(true);
let _ = mac!(some_map 2) == Some(true);
let _ = mac!(map Some(2)) == Some(true);
}
#[rustfmt::skip]
@ -50,6 +105,13 @@ fn result_methods() {
//~^ manual_is_variant_and
.unwrap_or_default();
let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) == Ok(true);
//~^ manual_is_variant_and
let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
//~^ manual_is_variant_and
let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
//~^ manual_is_variant_and
// won't fix because the return type of the closure is not `bool`
let _ = res.map(|x| x + 1).unwrap_or_default();

60
tests/ui/manual_is_variant_and.stderr
View file

@ -1,5 +1,5 @@
error: called `map(<f>).unwrap_or_default()` on an `Option` value
--> tests/ui/manual_is_variant_and.rs:13:17
--> tests/ui/manual_is_variant_and.rs:51:17
|
LL | let _ = opt.map(|x| x > 1)
| _________________^
@ -11,7 +11,7 @@ LL | | .unwrap_or_default();
= help: to override `-D warnings` add `#[allow(clippy::manual_is_variant_and)]`
error: called `map(<f>).unwrap_or_default()` on an `Option` value
--> tests/ui/manual_is_variant_and.rs:18:17
--> tests/ui/manual_is_variant_and.rs:56:17
|
LL | let _ = opt.map(|x| {
| _________________^
@ -30,13 +30,13 @@ LL ~ });
|
error: called `map(<f>).unwrap_or_default()` on an `Option` value
--> tests/ui/manual_is_variant_and.rs:23:17
--> tests/ui/manual_is_variant_and.rs:61:17
|
LL | let _ = opt.map(|x| x > 1).unwrap_or_default();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_some_and(|x| x > 1)`
error: called `map(<f>).unwrap_or_default()` on an `Option` value
--> tests/ui/manual_is_variant_and.rs:26:10
--> tests/ui/manual_is_variant_and.rs:64:10
|
LL | .map(|x| x > 1)
| __________^
@ -44,14 +44,38 @@ LL | |
LL | | .unwrap_or_default();
| |____________________________^ help: use: `is_some_and(|x| x > 1)`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:68:13
|
LL | let _ = Some(2).map(|x| x % 2 == 0) == Some(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_some_and(|x| x % 2 == 0)`
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)`
error: called `.map() == Some()`
--> tests/ui/manual_is_variant_and.rs:72:13
|
LL | let _ = Some(2).map(|x| x % 2 == 0) == some_true!();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_some_and(|x| x % 2 == 0)`
error: called `.map() != Some()`
--> tests/ui/manual_is_variant_and.rs:74:13
|
LL | let _ = Some(2).map(|x| x % 2 == 0) != some_false!();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_none_or(|x| x % 2 == 0)`
error: called `map(<f>).unwrap_or_default()` on an `Option` value
--> tests/ui/manual_is_variant_and.rs:34:18
--> tests/ui/manual_is_variant_and.rs:81:18
|
LL | let _ = opt2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_some_and(char::is_alphanumeric)`
error: called `map(<f>).unwrap_or_default()` on a `Result` value
--> tests/ui/manual_is_variant_and.rs:44:17
--> tests/ui/manual_is_variant_and.rs:99:17
|
LL | let _ = res.map(|x| {
| _________________^
@ -70,7 +94,7 @@ LL ~ });
|
error: called `map(<f>).unwrap_or_default()` on a `Result` value
--> tests/ui/manual_is_variant_and.rs:49:17
--> tests/ui/manual_is_variant_and.rs:104:17
|
LL | let _ = res.map(|x| x > 1)
| _________________^
@ -78,11 +102,29 @@ LL | |
LL | | .unwrap_or_default();
| |____________________________^ help: use: `is_ok_and(|x| x > 1)`
error: called `.map() == Ok()`
--> tests/ui/manual_is_variant_and.rs:108:13
|
LL | let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) == Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0)`
error: called `.map() != Ok()`
--> tests/ui/manual_is_variant_and.rs:110:13
|
LL | let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0)`
error: called `.map() != Ok()`
--> tests/ui/manual_is_variant_and.rs:112:13
|
LL | let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0)`
error: called `map(<f>).unwrap_or_default()` on a `Result` value
--> tests/ui/manual_is_variant_and.rs:57:18
--> tests/ui/manual_is_variant_and.rs:119:18
|
LL | let _ = res2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_ok_and(char::is_alphanumeric)`
error: aborting due to 8 previous errors
error: aborting due to 15 previous errors