Skip to content

Commit

Permalink
refactor, test on proc/external macros
Browse files Browse the repository at this point in the history
  • Loading branch information
Jacherr committed Jun 28, 2024
1 parent d65c0c4 commit d305884
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 110 deletions.
163 changes: 88 additions & 75 deletions clippy_lints/src/methods/unnecessary_map_or.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use clippy_utils::eager_or_lazy::switch_to_eager_eval;
use clippy_utils::source::{snippet, snippet_opt};
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::visitors::is_local_used;
use clippy_utils::{get_parent_expr, path_to_local_id};
use clippy_utils::{get_parent_expr, is_from_proc_macro, path_to_local_id};
use rustc_ast::util::parser::AssocOp;
use rustc_ast::LitKind::Bool;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -34,86 +34,99 @@ impl Variant {
}
}

// Only checking map_or for now
pub(super) fn check(
cx: &LateContext<'_>,
expr: &Expr<'_>,
pub(super) fn check<'a>(
cx: &LateContext<'a>,
expr: &Expr<'a>,
recv: &Expr<'_>,
def: &Expr<'_>,
map: &Expr<'_>,
msrv: &Msrv,
) {
if let ExprKind::Lit(def_kind) = def.kind
&& let typeck_results = cx.typeck_results()
&& let recv_ty = typeck_results.expr_ty(recv)
&& (is_type_diagnostic_item(cx, recv_ty, sym::Option) || is_type_diagnostic_item(cx, recv_ty, sym::Result))
&& let Bool(def_bool) = def_kind.node
{
let variant = if is_type_diagnostic_item(cx, recv_ty, sym::Option) {
Variant::Some
} else {
Variant::Ok
};

let (sugg, method) = if let ExprKind::Closure(map_closure) = map.kind
&& let closure_body = cx.tcx.hir().body(map_closure.body)
&& let closure_body_value = closure_body.value.peel_blocks()
&& let ExprKind::Binary(op, l, r) = closure_body_value.kind
&& let Some(param) = closure_body.params.first()
&& let PatKind::Binding(_, hir_id, _, _) = param.pat.kind
// checking that map_or is one of the following:
// .map_or(false, |x| x == y)
// .map_or(false, |x| y == x) - swapped comparison
// .map_or(true, |x| x != y)
// .map_or(true, |x| y != x) - swapped comparison
&& ((BinOpKind::Eq == op.node && !def_bool) || (BinOpKind::Ne == op.node && def_bool))
&& let non_binding_location = if path_to_local_id(l, hir_id) { r } else { l }
&& switch_to_eager_eval(cx, non_binding_location)
// xor, because if its both then thats a strange edge case and
// we can just ignore it, since by default clippy will error on this
&& (path_to_local_id(l, hir_id) ^ path_to_local_id(r, hir_id))
&& !is_local_used(cx, non_binding_location, hir_id)
&& typeck_results.expr_ty(l) == typeck_results.expr_ty(r)
{
let wrap = variant.variant_name();
let comparator = op.node.as_str();
if is_from_proc_macro(cx, expr) {
return;
}

// we may need to add parens around the suggestion
// in case the parent expression has additional method calls,
// since for example `Some(5).map_or(false, |x| x == 5).then(|| 1)`
// being converted to `Some(5) == Some(5).then(|| 1)` isnt
// the same thing
let should_add_parens = get_parent_expr(cx, expr)
.is_some_and(|expr| expr.precedence().order() > i8::try_from(AssocOp::Equal.precedence()).unwrap_or(0));
(
format!(
"{}{} {comparator} {wrap}({}){}",
if should_add_parens { "(" } else { "" },
snippet(cx, recv.span, ".."),
snippet(cx, non_binding_location.span.source_callsite(), ".."),
if should_add_parens { ")" } else { "" }
),
"standard comparison",
)
} else if !def_bool
&& msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND)
&& let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite())
&& let Some(span_callsite) = snippet_opt(cx, map.span.source_callsite())
{
let sugg = variant.method_name();
(format!("{recv_callsite}.{sugg}({span_callsite})",), sugg)
} else {
return;
};
let ExprKind::Lit(def_kind) = def.kind else {
return;
};

span_lint_and_sugg(
cx,
UNNECESSARY_MAP_OR,
expr.span,
format!("`map_or` is redundant, use {method} instead"),
"try",
sugg,
Applicability::MachineApplicable,
);
let recv_ty = cx.typeck_results().expr_ty(recv);
if !(is_type_diagnostic_item(cx, recv_ty, sym::Option) || is_type_diagnostic_item(cx, recv_ty, sym::Result)) {
return;
}

let Bool(def_bool) = def_kind.node else {
return;
};

let variant = if is_type_diagnostic_item(cx, recv_ty, sym::Option) {
Variant::Some
} else {
Variant::Ok
};

let (sugg, method) = if let ExprKind::Closure(map_closure) = map.kind
&& let closure_body = cx.tcx.hir().body(map_closure.body)
&& let closure_body_value = closure_body.value.peel_blocks()
&& let ExprKind::Binary(op, l, r) = closure_body_value.kind
&& let Some(param) = closure_body.params.first()
&& let PatKind::Binding(_, hir_id, _, _) = param.pat.kind
// checking that map_or is one of the following:
// .map_or(false, |x| x == y)
// .map_or(false, |x| y == x) - swapped comparison
// .map_or(true, |x| x != y)
// .map_or(true, |x| y != x) - swapped comparison
&& ((BinOpKind::Eq == op.node && !def_bool) || (BinOpKind::Ne == op.node && def_bool))
&& let non_binding_location = if path_to_local_id(l, hir_id) { r } else { l }
&& switch_to_eager_eval(cx, non_binding_location)
// xor, because if its both then thats a strange edge case and
// we can just ignore it, since by default clippy will error on this
&& (path_to_local_id(l, hir_id) ^ path_to_local_id(r, hir_id))
&& !is_local_used(cx, non_binding_location, hir_id)
&& let typeck_results = cx.typeck_results()
&& typeck_results.expr_ty(l) == typeck_results.expr_ty(r)
{
let wrap = variant.variant_name();
let comparator = op.node.as_str();

// we may need to add parens around the suggestion
// in case the parent expression has additional method calls,
// since for example `Some(5).map_or(false, |x| x == 5).then(|| 1)`
// being converted to `Some(5) == Some(5).then(|| 1)` isnt
// the same thing
let should_add_parens = get_parent_expr(cx, expr)
.is_some_and(|expr| expr.precedence().order() > i8::try_from(AssocOp::Equal.precedence()).unwrap_or(0));
(
format!(
"{}{} {comparator} {wrap}({}){}",
if should_add_parens { "(" } else { "" },
snippet(cx, recv.span, ".."),
snippet(cx, non_binding_location.span.source_callsite(), ".."),
if should_add_parens { ")" } else { "" }
),
"standard comparison",
)
} else if !def_bool
&& msrv.meets(msrvs::OPTION_RESULT_IS_VARIANT_AND)
&& let Some(recv_callsite) = snippet_opt(cx, recv.span.source_callsite())
&& let Some(span_callsite) = snippet_opt(cx, map.span.source_callsite())
{
let suggested_name = variant.method_name();
(
format!("{recv_callsite}.{suggested_name}({span_callsite})",),
suggested_name,
)
} else {
return;
};

span_lint_and_sugg(
cx,
UNNECESSARY_MAP_OR,
expr.span,
format!("this `map_or` is redundant"),
format!("use {method} instead"),
sugg,
Applicability::MachineApplicable,
);
}
14 changes: 13 additions & 1 deletion tests/ui/unnecessary_map_or.fixed
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
//@aux-build:proc_macros.rs
#![warn(clippy::unnecessary_map_or)]
#![allow(clippy::no_effect)]
#![allow(clippy::eq_op)]
#[allow(clippy::unnecessary_lazy_evaluations)]
#![allow(clippy::unnecessary_lazy_evaluations)]
#[clippy::msrv = "1.70.0"]
#[macro_use]
extern crate proc_macros;

fn main() {
// should trigger
let _ = Some(5) == Some(5);
Expand Down Expand Up @@ -33,6 +37,14 @@ fn main() {
let _ = x!().map_or(false, |n| n == vec![1][0]);

msrv_1_69();

external! {
let _ = Some(5).map_or(false, |n| n == 5);
}

with_span! {
let _ = Some(5).map_or(false, |n| n == 5);
}
}

#[clippy::msrv = "1.69.0"]
Expand Down
14 changes: 13 additions & 1 deletion tests/ui/unnecessary_map_or.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
//@aux-build:proc_macros.rs
#![warn(clippy::unnecessary_map_or)]
#![allow(clippy::no_effect)]
#![allow(clippy::eq_op)]
#[allow(clippy::unnecessary_lazy_evaluations)]
#![allow(clippy::unnecessary_lazy_evaluations)]
#[clippy::msrv = "1.70.0"]
#[macro_use]
extern crate proc_macros;

fn main() {
// should trigger
let _ = Some(5).map_or(false, |n| n == 5);
Expand Down Expand Up @@ -36,6 +40,14 @@ fn main() {
let _ = x!().map_or(false, |n| n == vec![1][0]);

msrv_1_69();

external! {
let _ = Some(5).map_or(false, |n| n == 5);
}

with_span! {
let _ = Some(5).map_or(false, |n| n == 5);
}
}

#[clippy::msrv = "1.69.0"]
Expand Down
66 changes: 33 additions & 33 deletions tests/ui/unnecessary_map_or.stderr
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
error: `map_or` is redundant, use standard comparison instead
--> tests/ui/unnecessary_map_or.rs:8:13
error: this `map_or` is redundant
--> tests/ui/unnecessary_map_or.rs:12:13
|
LL | let _ = Some(5).map_or(false, |n| n == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(5) == Some(5)`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use standard comparison instead: `Some(5) == Some(5)`
|
= note: `-D clippy::unnecessary-map-or` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_map_or)]`

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

error: `map_or` is redundant, use standard comparison instead
--> tests/ui/unnecessary_map_or.rs:10:13
error: this `map_or` is redundant
--> tests/ui/unnecessary_map_or.rs:14:13
|
LL | let _ = Some(5).map_or(false, |n| {
| _____________^
LL | | let _ = 1;
LL | | n == 5
LL | | });
| |______^ help: try: `Some(5) == Some(5)`
| |______^ help: use standard comparison instead: `Some(5) == Some(5)`

error: `map_or` is redundant, use is_some_and instead
--> tests/ui/unnecessary_map_or.rs:14:13
error: this `map_or` is redundant
--> tests/ui/unnecessary_map_or.rs:18:13
|
LL | let _ = Some(5).map_or(false, |n| {
| _____________^
Expand All @@ -33,55 +33,55 @@ LL | | 6 >= 5
LL | | });
| |______^
|
help: try
help: use is_some_and instead
|
LL ~ let _ = Some(5).is_some_and(|n| {
LL + let _ = n;
LL + 6 >= 5
LL ~ });
|

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

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

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

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

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

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

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

error: aborting due to 11 previous errors

0 comments on commit d305884

Please sign in to comment.