Skip to content

Commit 551870d

Browse files
samueltardieusamueltardieu
authored
Extend manual_is_variant_and lint to check for boolean map comparisons (rust-lang#14646)
Fixes rust-lang/rust-clippy#14542. changelog: [`manual_is_variant_and`]: Extend to check for boolean map comparisons r? @samueltardieu
2 parents 3927a61 + 634f875 commit 551870d

File tree

5 files changed

+240
-15
lines changed

5 files changed

+240
-15
lines changed

‎clippy_lints/src/methods/manual_is_variant_and.rs‎

Lines changed: 64 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,22 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::get_parent_expr;
23
use clippy_utils::msrvs::{self, Msrv};
3-
use clippy_utils::source::snippet;
4+
use clippy_utils::source::{snippet, snippet_opt};
45
use clippy_utils::ty::is_type_diagnostic_item;
56
use rustc_errors::Applicability;
7+
use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
8+
use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
69
use rustc_lint::LateContext;
7-
use rustc_span::{Span, sym};
10+
use rustc_middle::ty;
11+
use rustc_span::{BytePos, Span, sym};
812

913
use super::MANUAL_IS_VARIANT_AND;
1014

11-
pub(super) fn check<'tcx>(
15+
pub(super) fn check(
1216
cx: &LateContext<'_>,
13-
expr: &'tcx rustc_hir::Expr<'_>,
14-
map_recv: &'tcx rustc_hir::Expr<'_>,
15-
map_arg: &'tcx rustc_hir::Expr<'_>,
17+
expr: &Expr<'_>,
18+
map_recv: &Expr<'_>,
19+
map_arg: &Expr<'_>,
1620
map_span: Span,
1721
msrv: Msrv,
1822
) {
@@ -57,3 +61,57 @@ pub(super) fn check<'tcx>(
5761
Applicability::MachineApplicable,
5862
);
5963
}
64+
65+
fn emit_lint(cx: &LateContext<'_>, op: BinOpKind, parent: &Expr<'_>, method_span: Span, is_option: bool) {
66+
if let Some(before_map_snippet) = snippet_opt(cx, parent.span.with_hi(method_span.lo()))
67+
&& let Some(after_map_snippet) = snippet_opt(cx, method_span.with_lo(method_span.lo() + BytePos(3)))
68+
{
69+
span_lint_and_sugg(
70+
cx,
71+
MANUAL_IS_VARIANT_AND,
72+
parent.span,
73+
format!(
74+
"called `.map() {}= {}()`",
75+
if op == BinOpKind::Eq { '=' } else { '!' },
76+
if is_option { "Some" } else { "Ok" },
77+
),
78+
"use",
79+
if is_option && op == BinOpKind::Ne {
80+
format!("{before_map_snippet}is_none_or{after_map_snippet}",)
81+
} else {
82+
format!(
83+
"{}{before_map_snippet}{}{after_map_snippet}",
84+
if op == BinOpKind::Eq { "" } else { "!" },
85+
if is_option { "is_some_and" } else { "is_ok_and" },
86+
)
87+
},
88+
Applicability::MachineApplicable,
89+
);
90+
}
91+
}
92+
93+
pub(super) fn check_map(cx: &LateContext<'_>, expr: &Expr<'_>) {
94+
if let Some(parent_expr) = get_parent_expr(cx, expr)
95+
&& let ExprKind::Binary(op, left, right) = parent_expr.kind
96+
&& matches!(op.node, BinOpKind::Eq | BinOpKind::Ne)
97+
&& op.span.eq_ctxt(expr.span)
98+
{
99+
// Check `left` and `right` expression in any order, and for `Option` and `Result`
100+
for (expr1, expr2) in [(left, right), (right, left)] {
101+
for item in [sym::Option, sym::Result] {
102+
if let ExprKind::Call(call, ..) = expr1.kind
103+
&& let ExprKind::Path(QPath::Resolved(_, path)) = call.kind
104+
&& let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), _) = path.res
105+
&& let ty = cx.typeck_results().expr_ty(expr1)
106+
&& let ty::Adt(adt, args) = ty.kind()
107+
&& cx.tcx.is_diagnostic_item(item, adt.did())
108+
&& args.type_at(0).is_bool()
109+
&& let ExprKind::MethodCall(_, recv, _, span) = expr2.kind
110+
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), item)
111+
{
112+
return emit_lint(cx, op.node, parent_expr, span, item == sym::Option);
113+
}
114+
}
115+
}
116+
}
117+
}

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5203,6 +5203,7 @@ impl Methods {
52035203
unused_enumerate_index::check(cx, expr, recv, m_arg);
52045204
map_clone::check(cx, expr, recv, m_arg, self.msrv);
52055205
map_with_unused_argument_over_ranges::check(cx, expr, recv, m_arg, self.msrv, span);
5206+
manual_is_variant_and::check_map(cx, expr);
52065207
match method_call(recv) {
52075208
Some((map_name @ (sym::iter | sym::into_iter), recv2, _, _, _)) => {
52085209
iter_kv_map::check(cx, map_name, expr, recv2, m_arg, self.msrv);

‎tests/ui/manual_is_variant_and.fixed‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,44 @@
44
#[macro_use]
55
extern crate option_helpers;
66

7+
struct Foo<T>(T);
8+
9+
impl<T> Foo<T> {
10+
fn map<F: FnMut(T) -> bool>(self, mut f: F) -> Option<bool> {
11+
Some(f(self.0))
12+
}
13+
}
14+
15+
fn foo() -> Option<bool> {
16+
Some(true)
17+
}
18+
19+
macro_rules! some_true {
20+
() => {
21+
Some(true)
22+
};
23+
}
24+
macro_rules! some_false {
25+
() => {
26+
Some(false)
27+
};
28+
}
29+
30+
macro_rules! mac {
31+
(some $e:expr) => {
32+
Some($e)
33+
};
34+
(some_map $e:expr) => {
35+
Some($e).map(|x| x % 2 == 0)
36+
};
37+
(map $e:expr) => {
38+
$e.map(|x| x % 2 == 0)
39+
};
40+
(eq $a:expr, $b:expr) => {
41+
$a == $b
42+
};
43+
}
44+
745
#[rustfmt::skip]
846
fn option_methods() {
947
let opt = Some(1);
@@ -21,13 +59,30 @@ fn option_methods() {
2159
let _ = opt
2260
.is_some_and(|x| x > 1);
2361

62+
let _ = Some(2).is_some_and(|x| x % 2 == 0);
63+
//~^ manual_is_variant_and
64+
let _ = Some(2).is_none_or(|x| x % 2 == 0);
65+
//~^ manual_is_variant_and
66+
let _ = Some(2).is_some_and(|x| x % 2 == 0);
67+
//~^ manual_is_variant_and
68+
let _ = Some(2).is_none_or(|x| x % 2 == 0);
69+
//~^ manual_is_variant_and
70+
2471
// won't fix because the return type of the closure is not `bool`
2572
let _ = opt.map(|x| x + 1).unwrap_or_default();
2673

2774
let opt2 = Some('a');
2875
let _ = opt2.is_some_and(char::is_alphanumeric); // should lint
2976
//~^ manual_is_variant_and
3077
let _ = opt_map!(opt2, |x| x == 'a').unwrap_or_default(); // should not lint
78+
79+
// Should not lint.
80+
let _ = Foo::<u32>(0).map(|x| x % 2 == 0) == Some(true);
81+
let _ = Some(2).map(|x| x % 2 == 0) != foo();
82+
let _ = mac!(eq Some(2).map(|x| x % 2 == 0), Some(true));
83+
let _ = mac!(some 2).map(|x| x % 2 == 0) == Some(true);
84+
let _ = mac!(some_map 2) == Some(true);
85+
let _ = mac!(map Some(2)) == Some(true);
3186
}
3287

3388
#[rustfmt::skip]
@@ -41,6 +96,13 @@ fn result_methods() {
4196
});
4297
let _ = res.is_ok_and(|x| x > 1);
4398

99+
let _ = Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0);
100+
//~^ manual_is_variant_and
101+
let _ = !Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0);
102+
//~^ manual_is_variant_and
103+
let _ = !Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0);
104+
//~^ manual_is_variant_and
105+
44106
// won't fix because the return type of the closure is not `bool`
45107
let _ = res.map(|x| x + 1).unwrap_or_default();
46108

‎tests/ui/manual_is_variant_and.rs‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,44 @@
44
#[macro_use]
55
extern crate option_helpers;
66

7+
struct Foo<T>(T);
8+
9+
impl<T> Foo<T> {
10+
fn map<F: FnMut(T) -> bool>(self, mut f: F) -> Option<bool> {
11+
Some(f(self.0))
12+
}
13+
}
14+
15+
fn foo() -> Option<bool> {
16+
Some(true)
17+
}
18+
19+
macro_rules! some_true {
20+
() => {
21+
Some(true)
22+
};
23+
}
24+
macro_rules! some_false {
25+
() => {
26+
Some(false)
27+
};
28+
}
29+
30+
macro_rules! mac {
31+
(some $e:expr) => {
32+
Some($e)
33+
};
34+
(some_map $e:expr) => {
35+
Some($e).map(|x| x % 2 == 0)
36+
};
37+
(map $e:expr) => {
38+
$e.map(|x| x % 2 == 0)
39+
};
40+
(eq $a:expr, $b:expr) => {
41+
$a == $b
42+
};
43+
}
44+
745
#[rustfmt::skip]
846
fn option_methods() {
947
let opt = Some(1);
@@ -27,13 +65,30 @@ fn option_methods() {
2765
//~^ manual_is_variant_and
2866
.unwrap_or_default();
2967

68+
let _ = Some(2).map(|x| x % 2 == 0) == Some(true);
69+
//~^ manual_is_variant_and
70+
let _ = Some(2).map(|x| x % 2 == 0) != Some(true);
71+
//~^ manual_is_variant_and
72+
let _ = Some(2).map(|x| x % 2 == 0) == some_true!();
73+
//~^ manual_is_variant_and
74+
let _ = Some(2).map(|x| x % 2 == 0) != some_false!();
75+
//~^ manual_is_variant_and
76+
3077
// won't fix because the return type of the closure is not `bool`
3178
let _ = opt.map(|x| x + 1).unwrap_or_default();
3279

3380
let opt2 = Some('a');
3481
let _ = opt2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
3582
//~^ manual_is_variant_and
3683
let _ = opt_map!(opt2, |x| x == 'a').unwrap_or_default(); // should not lint
84+
85+
// Should not lint.
86+
let _ = Foo::<u32>(0).map(|x| x % 2 == 0) == Some(true);
87+
let _ = Some(2).map(|x| x % 2 == 0) != foo();
88+
let _ = mac!(eq Some(2).map(|x| x % 2 == 0), Some(true));
89+
let _ = mac!(some 2).map(|x| x % 2 == 0) == Some(true);
90+
let _ = mac!(some_map 2) == Some(true);
91+
let _ = mac!(map Some(2)) == Some(true);
3792
}
3893

3994
#[rustfmt::skip]
@@ -50,6 +105,13 @@ fn result_methods() {
50105
//~^ manual_is_variant_and
51106
.unwrap_or_default();
52107

108+
let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) == Ok(true);
109+
//~^ manual_is_variant_and
110+
let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
111+
//~^ manual_is_variant_and
112+
let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
113+
//~^ manual_is_variant_and
114+
53115
// won't fix because the return type of the closure is not `bool`
54116
let _ = res.map(|x| x + 1).unwrap_or_default();
55117

‎tests/ui/manual_is_variant_and.stderr‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: called `map(<f>).unwrap_or_default()` on an `Option` value
2-
--> tests/ui/manual_is_variant_and.rs:13:17
2+
--> tests/ui/manual_is_variant_and.rs:51:17
33
|
44
LL | let _ = opt.map(|x| x > 1)
55
| _________________^
@@ -11,7 +11,7 @@ LL | | .unwrap_or_default();
1111
= help: to override `-D warnings` add `#[allow(clippy::manual_is_variant_and)]`
1212

1313
error: called `map(<f>).unwrap_or_default()` on an `Option` value
14-
--> tests/ui/manual_is_variant_and.rs:18:17
14+
--> tests/ui/manual_is_variant_and.rs:56:17
1515
|
1616
LL | let _ = opt.map(|x| {
1717
| _________________^
@@ -30,28 +30,52 @@ LL ~ });
3030
|
3131

3232
error: called `map(<f>).unwrap_or_default()` on an `Option` value
33-
--> tests/ui/manual_is_variant_and.rs:23:17
33+
--> tests/ui/manual_is_variant_and.rs:61:17
3434
|
3535
LL | let _ = opt.map(|x| x > 1).unwrap_or_default();
3636
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_some_and(|x| x > 1)`
3737

3838
error: called `map(<f>).unwrap_or_default()` on an `Option` value
39-
--> tests/ui/manual_is_variant_and.rs:26:10
39+
--> tests/ui/manual_is_variant_and.rs:64:10
4040
|
4141
LL | .map(|x| x > 1)
4242
| __________^
4343
LL | |
4444
LL | | .unwrap_or_default();
4545
| |____________________________^ help: use: `is_some_and(|x| x > 1)`
4646

47+
error: called `.map() == Some()`
48+
--> tests/ui/manual_is_variant_and.rs:68:13
49+
|
50+
LL | let _ = Some(2).map(|x| x % 2 == 0) == Some(true);
51+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_some_and(|x| x % 2 == 0)`
52+
53+
error: called `.map() != Some()`
54+
--> tests/ui/manual_is_variant_and.rs:70:13
55+
|
56+
LL | let _ = Some(2).map(|x| x % 2 == 0) != Some(true);
57+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_none_or(|x| x % 2 == 0)`
58+
59+
error: called `.map() == Some()`
60+
--> tests/ui/manual_is_variant_and.rs:72:13
61+
|
62+
LL | let _ = Some(2).map(|x| x % 2 == 0) == some_true!();
63+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_some_and(|x| x % 2 == 0)`
64+
65+
error: called `.map() != Some()`
66+
--> tests/ui/manual_is_variant_and.rs:74:13
67+
|
68+
LL | let _ = Some(2).map(|x| x % 2 == 0) != some_false!();
69+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Some(2).is_none_or(|x| x % 2 == 0)`
70+
4771
error: called `map(<f>).unwrap_or_default()` on an `Option` value
48-
--> tests/ui/manual_is_variant_and.rs:34:18
72+
--> tests/ui/manual_is_variant_and.rs:81:18
4973
|
5074
LL | let _ = opt2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
5175
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_some_and(char::is_alphanumeric)`
5276

5377
error: called `map(<f>).unwrap_or_default()` on a `Result` value
54-
--> tests/ui/manual_is_variant_and.rs:44:17
78+
--> tests/ui/manual_is_variant_and.rs:99:17
5579
|
5680
LL | let _ = res.map(|x| {
5781
| _________________^
@@ -70,19 +94,37 @@ LL ~ });
7094
|
7195

7296
error: called `map(<f>).unwrap_or_default()` on a `Result` value
73-
--> tests/ui/manual_is_variant_and.rs:49:17
97+
--> tests/ui/manual_is_variant_and.rs:104:17
7498
|
7599
LL | let _ = res.map(|x| x > 1)
76100
| _________________^
77101
LL | |
78102
LL | | .unwrap_or_default();
79103
| |____________________________^ help: use: `is_ok_and(|x| x > 1)`
80104

105+
error: called `.map() == Ok()`
106+
--> tests/ui/manual_is_variant_and.rs:108:13
107+
|
108+
LL | let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) == Ok(true);
109+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0)`
110+
111+
error: called `.map() != Ok()`
112+
--> tests/ui/manual_is_variant_and.rs:110:13
113+
|
114+
LL | let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
115+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0)`
116+
117+
error: called `.map() != Ok()`
118+
--> tests/ui/manual_is_variant_and.rs:112:13
119+
|
120+
LL | let _ = Ok::<usize, ()>(2).map(|x| x % 2 == 0) != Ok(true);
121+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!Ok::<usize, ()>(2).is_ok_and(|x| x % 2 == 0)`
122+
81123
error: called `map(<f>).unwrap_or_default()` on a `Result` value
82-
--> tests/ui/manual_is_variant_and.rs:57:18
124+
--> tests/ui/manual_is_variant_and.rs:119:18
83125
|
84126
LL | let _ = res2.map(char::is_alphanumeric).unwrap_or_default(); // should lint
85127
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `is_ok_and(char::is_alphanumeric)`
86128

87-
error: aborting due to 8 previous errors
129+
error: aborting due to 15 previous errors
88130

0 commit comments

Comments
 (0)