Skip to content

Commit

Permalink
Replace assert!(a==b) with assert_eq!
Browse files Browse the repository at this point in the history
Expand lint `bool_assert_comparison` to catch `assert!(a == b)` and `assert!(a != b)` (or their debug versions), and suggest to replace them with the corresponding `assert_eq!(a, b)`.
  • Loading branch information
nyurik committed Sep 20, 2024
1 parent 2e5b680 commit a20cb02
Show file tree
Hide file tree
Showing 18 changed files with 180 additions and 81 deletions.
172 changes: 108 additions & 64 deletions clippy_lints/src/bool_assert_comparison.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::macros::{find_assert_eq_args, root_macro_call_first_node};
use clippy_utils::macros::{find_assert_args, find_assert_eq_args, root_macro_call_first_node, MacroCall};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{implements_trait, is_copy};
use rustc_ast::ast::LitKind;
use rustc_ast::BinOpKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, Lit};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::ty::{self, Ty};
use rustc_session::declare_lint_pass;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::Ident;
use rustc_span::Span;

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -74,74 +77,115 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
return;
};
let macro_name = cx.tcx.item_name(macro_call.def_id);
let eq_macro = match macro_name.as_str() {
"assert_eq" | "debug_assert_eq" => true,
"assert_ne" | "debug_assert_ne" => false,
_ => return,
};
let Some((a, b, _)) = find_assert_eq_args(cx, expr, macro_call.expn) else {
return;
let macro_name = macro_name.as_str();
match macro_name {
"assert_eq" | "debug_assert_eq" => check_eq(cx, expr, &macro_call, macro_name, true),
"assert_ne" | "debug_assert_ne" => check_eq(cx, expr, &macro_call, macro_name, false),
"assert" | "debug_assert" => check(cx, expr, &macro_call, macro_name),
_ => {},
};
}
}

let a_span = a.span.source_callsite();
let b_span = b.span.source_callsite();

let (lit_span, bool_value, non_lit_expr) = match (extract_bool_lit(a), extract_bool_lit(b)) {
// assert_eq!(true/false, b)
// ^^^^^^^^^^^^
(Some(bool_value), None) => (a_span.until(b_span), bool_value, b),
// assert_eq!(a, true/false)
// ^^^^^^^^^^^^
(None, Some(bool_value)) => (b_span.with_lo(a_span.hi()), bool_value, a),
// If there are two boolean arguments, we definitely don't understand
// what's going on, so better leave things as is...
//
// Or there is simply no boolean and then we can leave things as is!
_ => return,
};
fn check_eq<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
macro_call: &MacroCall,
macro_name: &str,
eq_macro: bool,
) {
let Some((a, b, _)) = find_assert_eq_args(cx, expr, macro_call.expn) else {
return;
};

let non_lit_ty = cx.typeck_results().expr_ty(non_lit_expr);
let a_span = a.span.source_callsite();
let b_span = b.span.source_callsite();

if !is_impl_not_trait_with_bool_out(cx, non_lit_ty) {
// At this point the expression which is not a boolean
// literal does not implement Not trait with a bool output,
// so we cannot suggest to rewrite our code
return;
}
let (lit_span, bool_value, non_lit_expr) = match (extract_bool_lit(a), extract_bool_lit(b)) {
// assert_eq!(true/false, b)
// ^^^^^^^^^^^^
(Some(bool_value), None) => (a_span.until(b_span), bool_value, b),
// assert_eq!(a, true/false)
// ^^^^^^^^^^^^
(None, Some(bool_value)) => (b_span.with_lo(a_span.hi()), bool_value, a),
// If there are two boolean arguments, we definitely don't understand
// what's going on, so better leave things as is...
//
// Or there is simply no boolean and then we can leave things as is!
_ => return,
};

if !is_copy(cx, non_lit_ty) {
// Only lint with types that are `Copy` because `assert!(x)` takes
// ownership of `x` whereas `assert_eq(x, true)` does not
return;
}
let non_lit_ty = cx.typeck_results().expr_ty(non_lit_expr);

let macro_name = macro_name.as_str();
let non_eq_mac = &macro_name[..macro_name.len() - 3];
span_lint_and_then(
cx,
BOOL_ASSERT_COMPARISON,
macro_call.span,
format!("used `{macro_name}!` with a literal bool"),
|diag| {
// assert_eq!(...)
// ^^^^^^^^^
let name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');

let mut suggestions = vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())];

if bool_value ^ eq_macro {
let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else {
return;
};
suggestions.push((non_lit_expr.span, (!sugg).to_string()));
}

diag.multipart_suggestion(
format!("replace it with `{non_eq_mac}!(..)`"),
suggestions,
Applicability::MachineApplicable,
);
},
);
if !is_impl_not_trait_with_bool_out(cx, non_lit_ty) {
// At this point the expression which is not a boolean
// literal does not implement Not trait with a bool output,
// so we cannot suggest to rewrite our code
return;
}

if !is_copy(cx, non_lit_ty) {
// Only lint with types that are `Copy` because `assert!(x)` takes
// ownership of `x` whereas `assert_eq(x, true)` does not
return;
}

let non_eq_mac = &macro_name[..macro_name.len() - 3];
span_lint_and_then(
cx,
BOOL_ASSERT_COMPARISON,
macro_call.span,
format!("used `{macro_name}!` with a literal bool"),
|diag| {
// assert_eq!(...)
// ^^^^^^^^^
let name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');

let mut suggestions = vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())];

if bool_value ^ eq_macro {
let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else {
return;
};
suggestions.push((non_lit_expr.span, (!sugg).to_string()));
}

diag.multipart_suggestion(
format!("replace it with `{non_eq_mac}!(..)`"),
suggestions,
Applicability::MachineApplicable,
);
},
);
}

/// Checks for `assert!(a == b)` and `assert!(a != b)`
fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, macro_call: &MacroCall, macro_name: &str) {
let Some((cond, _)) = find_assert_args(cx, expr, macro_call.expn) else {
return;
};
let ExprKind::Binary(Spanned { node, span }, lhs, rhs) = cond.kind else {
return;
};
// TODO: is `cond.span.from_expansion()` correct / needed?
if (node != BinOpKind::Eq && node != BinOpKind::Ne) || cond.span.from_expansion() {
return;
}

let new_name = format!("{macro_name}_{}", if node == BinOpKind::Eq { "eq" } else { "ne" });
let msg = format!("replace it with `{new_name}!(..)`");
span_lint_and_then(
cx,
BOOL_ASSERT_COMPARISON,
macro_call.span,
format!("used `{macro_name}!` with an equality comparison"),
|diag| {
let macro_name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');
// TODO: should this be `cond.span`, expanded to include preceding whitespace? If so, how?
let equality_span = Span::new(lhs.span.hi(), rhs.span.lo(), span.ctxt(), span.parent());
let suggestions = vec![(macro_name_span, new_name), (equality_span, ", ".to_string())];

diag.multipart_suggestion(msg, suggestions, Applicability::MachineApplicable);
},
);
}
2 changes: 1 addition & 1 deletion clippy_lints/src/eta_reduction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'
}
}

assert!(from_sig.inputs_and_output.len() == to_sig.inputs_and_output.len());
assert_eq!(from_sig.inputs_and_output.len(), to_sig.inputs_and_output.len());
from_sig
.inputs_and_output
.iter()
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/functions/renamed_function_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl RenamedFnArgs {
{
let mut renamed: Vec<(Span, String)> = vec![];

debug_assert!(default_names.size_hint() == current_names.size_hint());
debug_assert_eq!(default_names.size_hint(), current_names.size_hint());
while let (Some(def_name), Some(cur_name)) = (default_names.next(), current_names.next()) {
let current_name = cur_name.name;
let default_name = def_name.name;
Expand Down
5 changes: 3 additions & 2 deletions clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1040,8 +1040,9 @@ fn assert_generic_args_match<'tcx>(tcx: TyCtxt<'tcx>, did: DefId, args: &[Generi
.chain(&g.own_params)
.map(|x| &x.kind);

assert!(
count == args.len(),
assert_eq!(
count,
args.len(),
"wrong number of arguments for `{did:?}`: expected `{count}`, found {}\n\
note: the expected arguments are: `[{}]`\n\
the given arguments are: `{args:#?}`",
Expand Down
4 changes: 2 additions & 2 deletions lintcheck/src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ fn print_stats(old_stats: HashMap<String, usize>, new_stats: HashMap<&String, us

// remove duplicates from both hashmaps
for (k, v) in &same_in_both_hashmaps {
assert!(old_stats_deduped.remove(k) == Some(*v));
assert!(new_stats_deduped.remove(k) == Some(*v));
assert_eq!(old_stats_deduped.remove(k), Some(*v));
assert_eq!(new_stats_deduped.remove(k), Some(*v));
}

println!("\nStats:");
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/assertions_on_constants.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op)]
#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op, clippy::bool_assert_comparison)]

macro_rules! assert_const {
($len:expr) => {
Expand Down
5 changes: 5 additions & 0 deletions tests/ui/bool_assert_comparison.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,9 @@ fn main() {
debug_assert!("".is_empty());
debug_assert!(!"requires negation".is_empty());
debug_assert!(!"requires negation".is_empty());

assert_eq!("a", "a".to_ascii_lowercase());
assert_eq!("a", "a".to_ascii_lowercase(), "a==a");
assert_ne!("A", "A".to_ascii_lowercase());
assert_ne!("A", "A".to_ascii_lowercase(), "A!=a");
}
5 changes: 5 additions & 0 deletions tests/ui/bool_assert_comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,9 @@ fn main() {
debug_assert_ne!("".is_empty(), false);
debug_assert_ne!("requires negation".is_empty(), true);
debug_assert_eq!("requires negation".is_empty(), false);

assert!("a" == "a".to_ascii_lowercase());
assert!("a" == "a".to_ascii_lowercase(), "a==a");
assert!("A" != "A".to_ascii_lowercase());
assert!("A" != "A".to_ascii_lowercase(), "A!=a");
}
46 changes: 45 additions & 1 deletion tests/ui/bool_assert_comparison.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -396,5 +396,49 @@ LL - debug_assert_eq!("requires negation".is_empty(), false);
LL + debug_assert!(!"requires negation".is_empty());
|

error: aborting due to 33 previous errors
error: used `assert!` with an equality comparison
--> tests/ui/bool_assert_comparison.rs:170:5
|
LL | assert!("a" == "a".to_ascii_lowercase());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: replace it with `assert_eq!(..)`
|
LL | assert_eq!("a", "a".to_ascii_lowercase());
| ~~~~~~~~~ ~

error: used `assert!` with an equality comparison
--> tests/ui/bool_assert_comparison.rs:171:5
|
LL | assert!("a" == "a".to_ascii_lowercase(), "a==a");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: replace it with `assert_eq!(..)`
|
LL | assert_eq!("a", "a".to_ascii_lowercase(), "a==a");
| ~~~~~~~~~ ~

error: used `assert!` with an equality comparison
--> tests/ui/bool_assert_comparison.rs:172:5
|
LL | assert!("A" != "A".to_ascii_lowercase());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: replace it with `assert_ne!(..)`
|
LL | assert_ne!("A", "A".to_ascii_lowercase());
| ~~~~~~~~~ ~

error: used `assert!` with an equality comparison
--> tests/ui/bool_assert_comparison.rs:173:5
|
LL | assert!("A" != "A".to_ascii_lowercase(), "A!=a");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: replace it with `assert_ne!(..)`
|
LL | assert_ne!("A", "A".to_ascii_lowercase(), "A!=a");
| ~~~~~~~~~ ~

error: aborting due to 37 previous errors

2 changes: 1 addition & 1 deletion tests/ui/infinite_loops.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@no-rustfix
//@aux-build:proc_macros.rs

#![allow(clippy::never_loop)]
#![allow(clippy::never_loop, clippy::bool_assert_comparison)]
#![warn(clippy::infinite_loop)]

extern crate proc_macros;
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/missing_asserts_for_indexing.fixed
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(unused)]
#![allow(unused, clippy::bool_assert_comparison)]
#![warn(clippy::missing_asserts_for_indexing)]

// ok
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/missing_asserts_for_indexing.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(unused)]
#![allow(unused, clippy::bool_assert_comparison)]
#![warn(clippy::missing_asserts_for_indexing)]

// ok
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/panic_in_result_fn_assertions.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![warn(clippy::panic_in_result_fn)]
#![allow(clippy::uninlined_format_args, clippy::unnecessary_wraps)]

#![allow(clippy::bool_assert_comparison)]
struct A;

impl A {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/panic_in_result_fn_debug_assertions.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![warn(clippy::panic_in_result_fn)]
#![allow(clippy::uninlined_format_args, clippy::unnecessary_wraps)]

#![allow(clippy::bool_assert_comparison)]
// debug_assert should never trigger the `panic_in_result_fn` lint

struct A;
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/uninit_vec.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![warn(clippy::uninit_vec)]

#![allow(clippy::bool_assert_comparison)]
use std::cell::UnsafeCell;
use std::mem::MaybeUninit;

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/uninlined_format_args_panic.edition2018.fixed
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@revisions: edition2018 edition2021
//@[edition2018] edition:2018
//@[edition2021] edition:2021

#![allow(clippy::bool_assert_comparison)]
#![warn(clippy::uninlined_format_args)]

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/uninlined_format_args_panic.edition2021.fixed
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@revisions: edition2018 edition2021
//@[edition2018] edition:2018
//@[edition2021] edition:2021

#![allow(clippy::bool_assert_comparison)]
#![warn(clippy::uninlined_format_args)]

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/uninlined_format_args_panic.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@revisions: edition2018 edition2021
//@[edition2018] edition:2018
//@[edition2021] edition:2021

#![allow(clippy::bool_assert_comparison)]
#![warn(clippy::uninlined_format_args)]

fn main() {
Expand Down

0 comments on commit a20cb02

Please sign in to comment.