Skip to content

Commit 7edc1f0

Browse files
JacherrJacherr
Jacherr
authored and
Jacherr
committed
refactor, test on proc/external macros
1 parent ef1750e commit 7edc1f0

File tree

4 files changed

+147
-110
lines changed

4 files changed

+147
-110
lines changed

‎clippy_lints/src/methods/unnecessary_map_or.rs

+88-75
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use clippy_utils::eager_or_lazy::switch_to_eager_eval;
44
use clippy_utils::source::{snippet, snippet_opt};
55
use clippy_utils::ty::is_type_diagnostic_item;
66
use clippy_utils::visitors::is_local_used;
7-
use clippy_utils::{get_parent_expr, path_to_local_id};
7+
use clippy_utils::{get_parent_expr, is_from_proc_macro, path_to_local_id};
88
use rustc_ast::util::parser::AssocOp;
99
use rustc_ast::LitKind::Bool;
1010
use rustc_errors::Applicability;
@@ -34,86 +34,99 @@ impl Variant {
3434
}
3535
}
3636

37-
// Only checking map_or for now
38-
pub(super) fn check(
39-
cx: &LateContext<'_>,
40-
expr: &Expr<'_>,
37+
pub(super) fn check<'a>(
38+
cx: &LateContext<'a>,
39+
expr: &Expr<'a>,
4140
recv: &Expr<'_>,
4241
def: &Expr<'_>,
4342
map: &Expr<'_>,
4443
msrv: &Msrv,
4544
) {
46-
if let ExprKind::Lit(def_kind) = def.kind
47-
&& let typeck_results = cx.typeck_results()
48-
&& let recv_ty = typeck_results.expr_ty(recv)
49-
&& (is_type_diagnostic_item(cx, recv_ty, sym::Option) || is_type_diagnostic_item(cx, recv_ty, sym::Result))
50-
&& let Bool(def_bool) = def_kind.node
51-
{
52-
let variant = if is_type_diagnostic_item(cx, recv_ty, sym::Option) {
53-
Variant::Some
54-
} else {
55-
Variant::Ok
56-
};
57-
58-
let (sugg, method) = if let ExprKind::Closure(map_closure) = map.kind
59-
&& let closure_body = cx.tcx.hir().body(map_closure.body)
60-
&& let closure_body_value = closure_body.value.peel_blocks()
61-
&& let ExprKind::Binary(op, l, r) = closure_body_value.kind
62-
&& let Some(param) = closure_body.params.first()
63-
&& let PatKind::Binding(_, hir_id, _, _) = param.pat.kind
64-
// checking that map_or is one of the following:
65-
// .map_or(false, |x| x == y)
66-
// .map_or(false, |x| y == x) - swapped comparison
67-
// .map_or(true, |x| x != y)
68-
// .map_or(true, |x| y != x) - swapped comparison
69-
&& ((BinOpKind::Eq == op.node && !def_bool) || (BinOpKind::Ne == op.node && def_bool))
70-
&& let non_binding_location = if path_to_local_id(l, hir_id) { r } else { l }
71-
&& switch_to_eager_eval(cx, non_binding_location)
72-
// xor, because if its both then thats a strange edge case and
73-
// we can just ignore it, since by default clippy will error on this
74-
&& (path_to_local_id(l, hir_id) ^ path_to_local_id(r, hir_id))
75-
&& !is_local_used(cx, non_binding_location, hir_id)
76-
&& typeck_results.expr_ty(l) == typeck_results.expr_ty(r)
77-
{
78-
let wrap = variant.variant_name();
79-
let comparator = op.node.as_str();
45+
if is_from_proc_macro(cx, expr) {
46+
return;
47+
}
8048

81-
// we may need to add parens around the suggestion
82-
// in case the parent expression has additional method calls,
83-
// since for example `Some(5).map_or(false, |x| x == 5).then(|| 1)`
84-
// being converted to `Some(5) == Some(5).then(|| 1)` isnt
85-
// the same thing
86-
let should_add_parens = get_parent_expr(cx, expr)
87-
.is_some_and(|expr| expr.precedence().order() > i8::try_from(AssocOp::Equal.precedence()).unwrap_or(0));
88-
(
89-
format!(
90-
"{}{} {comparator} {wrap}({}){}",
91-
if should_add_parens { "(" } else { "" },
92-
snippet(cx, recv.span, ".."),
93-
snippet(cx, non_binding_location.span.source_callsite(), ".."),
94-
if should_add_parens { ")" } else { "" }
95-
),
96-
"standard comparison",
97-
)
98-
} else if !def_bool
99-
&& msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND)
100-
&& let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite())
101-
&& let Some(span_callsite) = snippet_opt(cx, map.span.source_callsite())
102-
{
103-
let sugg = variant.method_name();
104-
(format!("{recv_callsite}.{sugg}({span_callsite})",), sugg)
105-
} else {
106-
return;
107-
};
49+
let ExprKind::Lit(def_kind) = def.kind else {
50+
return;
51+
};
10852

109-
span_lint_and_sugg(
110-
cx,
111-
UNNECESSARY_MAP_OR,
112-
expr.span,
113-
format!("`map_or` is redundant, use {method} instead"),
114-
"try",
115-
sugg,
116-
Applicability::MachineApplicable,
117-
);
53+
let recv_ty = cx.typeck_results().expr_ty(recv);
54+
if !(is_type_diagnostic_item(cx, recv_ty, sym::Option) || is_type_diagnostic_item(cx, recv_ty, sym::Result)) {
55+
return;
11856
}
57+
58+
let Bool(def_bool) = def_kind.node else {
59+
return;
60+
};
61+
62+
let variant = if is_type_diagnostic_item(cx, recv_ty, sym::Option) {
63+
Variant::Some
64+
} else {
65+
Variant::Ok
66+
};
67+
68+
let (sugg, method) = if let ExprKind::Closure(map_closure) = map.kind
69+
&& let closure_body = cx.tcx.hir().body(map_closure.body)
70+
&& let closure_body_value = closure_body.value.peel_blocks()
71+
&& let ExprKind::Binary(op, l, r) = closure_body_value.kind
72+
&& let Some(param) = closure_body.params.first()
73+
&& let PatKind::Binding(_, hir_id, _, _) = param.pat.kind
74+
// checking that map_or is one of the following:
75+
// .map_or(false, |x| x == y)
76+
// .map_or(false, |x| y == x) - swapped comparison
77+
// .map_or(true, |x| x != y)
78+
// .map_or(true, |x| y != x) - swapped comparison
79+
&& ((BinOpKind::Eq == op.node && !def_bool) || (BinOpKind::Ne == op.node && def_bool))
80+
&& let non_binding_location = if path_to_local_id(l, hir_id) { r } else { l }
81+
&& switch_to_eager_eval(cx, non_binding_location)
82+
// xor, because if its both then thats a strange edge case and
83+
// we can just ignore it, since by default clippy will error on this
84+
&& (path_to_local_id(l, hir_id) ^ path_to_local_id(r, hir_id))
85+
&& !is_local_used(cx, non_binding_location, hir_id)
86+
&& let typeck_results = cx.typeck_results()
87+
&& typeck_results.expr_ty(l) == typeck_results.expr_ty(r)
88+
{
89+
let wrap = variant.variant_name();
90+
let comparator = op.node.as_str();
91+
92+
// we may need to add parens around the suggestion
93+
// in case the parent expression has additional method calls,
94+
// since for example `Some(5).map_or(false, |x| x == 5).then(|| 1)`
95+
// being converted to `Some(5) == Some(5).then(|| 1)` isnt
96+
// the same thing
97+
let should_add_parens = get_parent_expr(cx, expr)
98+
.is_some_and(|expr| expr.precedence().order() > i8::try_from(AssocOp::Equal.precedence()).unwrap_or(0));
99+
(
100+
format!(
101+
"{}{} {comparator} {wrap}({}){}",
102+
if should_add_parens { "(" } else { "" },
103+
snippet(cx, recv.span, ".."),
104+
snippet(cx, non_binding_location.span.source_callsite(), ".."),
105+
if should_add_parens { ")" } else { "" }
106+
),
107+
"standard comparison",
108+
)
109+
} else if !def_bool
110+
&& msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND)
111+
&& let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite())
112+
&& let Some(span_callsite) = snippet_opt(cx, map.span.source_callsite())
113+
{
114+
let suggested_name = variant.method_name();
115+
(
116+
format!("{recv_callsite}.{suggested_name}({span_callsite})",),
117+
suggested_name,
118+
)
119+
} else {
120+
return;
121+
};
122+
123+
span_lint_and_sugg(
124+
cx,
125+
UNNECESSARY_MAP_OR,
126+
expr.span,
127+
format!("this `map_or` is redundant"),
128+
format!("use {method} instead"),
129+
sugg,
130+
Applicability::MachineApplicable,
131+
);
119132
}

‎tests/ui/unnecessary_map_or.fixed

+13-1
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1+
//@aux-build:proc_macros.rs
12
#![warn(clippy::unnecessary_map_or)]
23
#![allow(clippy::no_effect)]
34
#![allow(clippy::eq_op)]
4-
#[allow(clippy::unnecessary_lazy_evaluations)]
5+
#![allow(clippy::unnecessary_lazy_evaluations)]
56
#[clippy::msrv = "1.70.0"]
7+
#[macro_use]
8+
extern crate proc_macros;
9+
610
fn main() {
711
// should trigger
812
let _ = Some(5) == Some(5);
@@ -33,6 +37,14 @@ fn main() {
3337
let _ = x!().map_or(false, |n| n == vec![1][0]);
3438

3539
msrv_1_69();
40+
41+
external! {
42+
let _ = Some(5).map_or(false, |n| n == 5);
43+
}
44+
45+
with_span! {
46+
let _ = Some(5).map_or(false, |n| n == 5);
47+
}
3648
}
3749

3850
#[clippy::msrv = "1.69.0"]

‎tests/ui/unnecessary_map_or.rs

+13-1
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1+
//@aux-build:proc_macros.rs
12
#![warn(clippy::unnecessary_map_or)]
23
#![allow(clippy::no_effect)]
34
#![allow(clippy::eq_op)]
4-
#[allow(clippy::unnecessary_lazy_evaluations)]
5+
#![allow(clippy::unnecessary_lazy_evaluations)]
56
#[clippy::msrv = "1.70.0"]
7+
#[macro_use]
8+
extern crate proc_macros;
9+
610
fn main() {
711
// should trigger
812
let _ = Some(5).map_or(false, |n| n == 5);
@@ -36,6 +40,14 @@ fn main() {
3640
let _ = x!().map_or(false, |n| n == vec![1][0]);
3741

3842
msrv_1_69();
43+
44+
external! {
45+
let _ = Some(5).map_or(false, |n| n == 5);
46+
}
47+
48+
with_span! {
49+
let _ = Some(5).map_or(false, |n| n == 5);
50+
}
3951
}
4052

4153
#[clippy::msrv = "1.69.0"]

‎tests/ui/unnecessary_map_or.stderr

+33-33
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,30 @@
1-
error: `map_or` is redundant, use standard comparison instead
2-
--> tests/ui/unnecessary_map_or.rs:8:13
1+
error: this `map_or` is redundant
2+
--> tests/ui/unnecessary_map_or.rs:12:13
33
|
44
LL | let _ = Some(5).map_or(false, |n| n == 5);
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) == Some(5)`
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use standard comparison instead: `Some(5) == Some(5)`
66
|
77
= note: `-D clippy::unnecessary-map-or` implied by `-D warnings`
88
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_map_or)]`
99

10-
error: `map_or` is redundant, use standard comparison instead
11-
--> tests/ui/unnecessary_map_or.rs:9:13
10+
error: this `map_or` is redundant
11+
--> tests/ui/unnecessary_map_or.rs:13:13
1212
|
1313
LL | let _ = Some(5).map_or(true, |n| n != 5);
14-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) != Some(5)`
14+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use standard comparison instead: `Some(5) != Some(5)`
1515

16-
error: `map_or` is redundant, use standard comparison instead
17-
--> tests/ui/unnecessary_map_or.rs:10:13
16+
error: this `map_or` is redundant
17+
--> tests/ui/unnecessary_map_or.rs:14:13
1818
|
1919
LL | let _ = Some(5).map_or(false, |n| {
2020
| _____________^
2121
LL | | let _ = 1;
2222
LL | | n == 5
2323
LL | | });
24-
| |______^ help: try: `Some(5) == Some(5)`
24+
| |______^ help: use standard comparison instead: `Some(5) == Some(5)`
2525

26-
error: `map_or` is redundant, use is_some_and instead
27-
--> tests/ui/unnecessary_map_or.rs:14:13
26+
error: this `map_or` is redundant
27+
--> tests/ui/unnecessary_map_or.rs:18:13
2828
|
2929
LL | let _ = Some(5).map_or(false, |n| {
3030
| _____________^
@@ -33,55 +33,55 @@ LL | | 6 >= 5
3333
LL | | });
3434
| |______^
3535
|
36-
help: try
36+
help: use is_some_and instead
3737
|
3838
LL ~ let _ = Some(5).is_some_and(|n| {
3939
LL + let _ = n;
4040
LL + 6 >= 5
4141
LL ~ });
4242
|
4343

44-
error: `map_or` is redundant, use is_some_and instead
45-
--> tests/ui/unnecessary_map_or.rs:18:13
44+
error: this `map_or` is redundant
45+
--> tests/ui/unnecessary_map_or.rs:22:13
4646
|
4747
LL | let _ = Some(vec![5]).map_or(false, |n| n == [5]);
48-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(vec![5]).is_some_and(|n| n == [5])`
48+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(vec![5]).is_some_and(|n| n == [5])`
4949

50-
error: `map_or` is redundant, use is_some_and instead
51-
--> tests/ui/unnecessary_map_or.rs:19:13
50+
error: this `map_or` is redundant
51+
--> tests/ui/unnecessary_map_or.rs:23:13
5252
|
5353
LL | let _ = Some(vec![1]).map_or(false, |n| vec![2] == n);
54-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(vec![1]).is_some_and(|n| vec![2] == n)`
54+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(vec![1]).is_some_and(|n| vec![2] == n)`
5555

56-
error: `map_or` is redundant, use is_some_and instead
57-
--> tests/ui/unnecessary_map_or.rs:20:13
56+
error: this `map_or` is redundant
57+
--> tests/ui/unnecessary_map_or.rs:24:13
5858
|
5959
LL | let _ = Some(5).map_or(false, |n| n == n);
60-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5).is_some_and(|n| n == n)`
60+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(5).is_some_and(|n| n == n)`
6161

62-
error: `map_or` is redundant, use is_some_and instead
63-
--> tests/ui/unnecessary_map_or.rs:21:13
62+
error: this `map_or` is redundant
63+
--> tests/ui/unnecessary_map_or.rs:25:13
6464
|
6565
LL | let _ = Some(5).map_or(false, |n| n == if 2 > 1 { n } else { 0 });
66-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5).is_some_and(|n| n == if 2 > 1 { n } else { 0 })`
66+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_some_and instead: `Some(5).is_some_and(|n| n == if 2 > 1 { n } else { 0 })`
6767

68-
error: `map_or` is redundant, use is_ok_and instead
69-
--> tests/ui/unnecessary_map_or.rs:22:13
68+
error: this `map_or` is redundant
69+
--> tests/ui/unnecessary_map_or.rs:26:13
7070
|
7171
LL | let _ = Ok::<Vec<i32>, i32>(vec![5]).map_or(false, |n| n == [5]);
72-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Ok::<Vec<i32>, i32>(vec![5]).is_ok_and(|n| n == [5])`
72+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use is_ok_and instead: `Ok::<Vec<i32>, i32>(vec![5]).is_ok_and(|n| n == [5])`
7373

74-
error: `map_or` is redundant, use standard comparison instead
75-
--> tests/ui/unnecessary_map_or.rs:23:13
74+
error: this `map_or` is redundant
75+
--> tests/ui/unnecessary_map_or.rs:27:13
7676
|
7777
LL | let _ = Ok::<i32, i32>(5).map_or(false, |n| n == 5);
78-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Ok::<i32, i32>(5) == Ok(5)`
78+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use standard comparison instead: `Ok::<i32, i32>(5) == Ok(5)`
7979

80-
error: `map_or` is redundant, use standard comparison instead
81-
--> tests/ui/unnecessary_map_or.rs:24:13
80+
error: this `map_or` is redundant
81+
--> tests/ui/unnecessary_map_or.rs:28:13
8282
|
8383
LL | let _ = Some(5).map_or(false, |n| n == 5).then(|| 1);
84-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(Some(5) == Some(5))`
84+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use standard comparison instead: `(Some(5) == Some(5))`
8585

8686
error: aborting due to 11 previous errors
8787

0 commit comments

Comments
 (0)