Skip to content

Commit

Permalink
change to a structure for parsing assert macro
Browse files Browse the repository at this point in the history
  • Loading branch information
ABouttefeux committed Oct 14, 2021
1 parent f2f5ca6 commit ce66869
Show file tree
Hide file tree
Showing 7 changed files with 307 additions and 102 deletions.
14 changes: 8 additions & 6 deletions clippy_lints/src/bool_assert_comparison.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, sugg::Sugg, ty::implements_trait};
use clippy_utils::{
diagnostics::span_lint_and_sugg, higher::AssertExpn, is_direct_expn_of, sugg::Sugg, ty::implements_trait,
};
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, Lit};
Expand Down Expand Up @@ -79,7 +81,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
.chain(inverted_macros.iter().map(|el| (el, false)))
{
if let Some(span) = is_direct_expn_of(expr.span, mac) {
if let Some(args) = higher::extract_assert_macro_args(expr) {
if let Some(args) = AssertExpn::parse(expr).map(|v| v.argument_vector()) {
if let [a, b, ref fmt_args @ ..] = args[..] {
let (lit_value, other_expr) = match (bool_lit(a), bool_lit(b)) {
(Some(lit), None) => (lit, b),
Expand All @@ -101,7 +103,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
}

let non_eq_mac = &mac[..mac.len() - 3];
let mut applicability = Applicability::MaybeIncorrect;
let mut applicability = Applicability::MachineApplicable;
let sugg = Sugg::hir_with_applicability(cx, other_expr, "..", &mut applicability);
let expr_string = if lit_value ^ is_eq {
format!("!{}", sugg.maybe_par())
Expand Down Expand Up @@ -130,9 +132,9 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
},
};
let suggestion = if let Some(spans) = arg_span {
format!("{}!({}, {})", non_eq_mac, expr_string, spans)
format!("{}!({}, {});", non_eq_mac, expr_string, spans)
} else {
format!("{}!({})", non_eq_mac, expr_string)
format!("{}!({});", non_eq_mac, expr_string)
};
span_lint_and_sugg(
cx,
Expand All @@ -141,7 +143,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
&format!("used `{}!` with a literal bool", mac),
"replace it with",
suggestion,
Applicability::MaybeIncorrect,
applicability,
);
return;
}
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/eq_op.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then};
use clippy_utils::source::snippet;
use clippy_utils::ty::{implements_trait, is_copy};
use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher, in_macro, is_expn_of};
use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher::AssertExpn, in_macro, is_expn_of};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, StmtKind};
Expand Down Expand Up @@ -77,7 +77,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp {
if_chain! {
if is_expn_of(stmt.span, amn).is_some();
if let StmtKind::Semi(matchexpr) = stmt.kind;
if let Some(macro_args) = higher::extract_assert_macro_args(matchexpr);
if let Some(macro_args) = AssertExpn::parse(matchexpr).map(|v| v.argument_vector());
if macro_args.len() == 2;
let (lhs, rhs) = (macro_args[0], macro_args[1]);
if eq_expr_value(cx, lhs, rhs);
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/mutable_debug_assertion.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint;
use clippy_utils::{higher, is_direct_expn_of};
use clippy_utils::{higher::AssertExpn, is_direct_expn_of};
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
use rustc_hir::{BorrowKind, Expr, ExprKind, MatchSource, Mutability};
use rustc_lint::{LateContext, LateLintPass};
Expand Down Expand Up @@ -39,7 +39,7 @@ impl<'tcx> LateLintPass<'tcx> for DebugAssertWithMutCall {
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
for dmn in &DEBUG_MACRO_NAMES {
if is_direct_expn_of(e.span, dmn).is_some() {
if let Some(macro_args) = higher::extract_assert_macro_args(e) {
if let Some(macro_args) = AssertExpn::parse(e).map(|v| v.argument_vector()) {
for arg in macro_args {
let mut visitor = MutArgVisitor::new(cx);
visitor.visit_expr(arg);
Expand Down
125 changes: 85 additions & 40 deletions clippy_utils/src/higher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,24 +418,73 @@ pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind {
}
}

/// Extract args from an assert-like macro.
/// Currently working with:
/// - `assert!`, `assert_eq!` and `assert_ne!`
/// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!`
/// For example:
/// `assert!(expr)` will return `Some([expr])`
/// `debug_assert_eq!(a, b)` will return `Some([a, b])`
pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option<Vec<&'tcx Expr<'tcx>>> {
/// A parsed
/// assert!`, `assert_eq!` or `assert_ne!`,
/// debug_assert!`, `debug_assert_eq!` or `debug_assert_ne!`
/// macro.
pub struct AssertExpn<'tcx> {
/// the first agrument of the assret e.g. `var` in element `assert!(var, ...)`
pub first_assert_argument: &'tcx Expr<'tcx>,
/// second argument of the asset for case `assert_eq!`,
/// `assert_ne!` etc ... Eg var_2 in `debug_assert_eq!(x, var_2,..)`
pub second_assert_argument: Option<&'tcx Expr<'tcx>>,
/// The format argument passed at the end of the macro
pub format_arg: Option<FormatArgsExpn<'tcx>>,
}

impl<'tcx> AssertExpn<'tcx> {
/// Extract args from an assert-like macro.
/// Currently working with:
/// - `assert!`, `assert_eq!` and `assert_ne!`
/// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!`
/// For example:
/// `assert!(expr)` will return `Some(AssertExpn { first_assert_argument: expr,
/// second_assert_argument: None, format_arg:None })` `debug_assert_eq!(a, b)` will return
/// `Some(AssertExpn { first_assert_argument: a, second_assert_argument: Some(b),
/// format_arg:None })`
pub fn parse(e: &'tcx Expr<'tcx>) -> Option<Self> {
if let ExprKind::Block(block, _) = e.kind {
if block.stmts.len() == 1 {
if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind {
// macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`)
if_chain! {
if let Some(If { cond, .. }) = If::hir(matchexpr);
if let ExprKind::Unary(UnOp::Not, condition) = cond.kind;
then {
return Some(Self {
first_assert_argument: condition,
second_assert_argument: None,
format_arg: None, // FIXME actually parse the aguments
});
}
}

// debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
if_chain! {
if let ExprKind::Block(matchblock,_) = matchexpr.kind;
if let Some(matchblock_expr) = matchblock.expr;
then {
return Self::ast_matchblock(matchblock_expr);
}
}
}
} else if let Some(matchblock_expr) = block.expr {
// macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
return Self::ast_matchblock(matchblock_expr);
}
}
None
}

/// Try to match the AST for a pattern that contains a match, for example when two args are
/// compared
fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option<Vec<&Expr<'_>>> {
fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option<Self> {
if_chain! {
if let ExprKind::Match(headerexpr, arms, _) = &matchblock_expr.kind;
if let ExprKind::Tup([lhs, rhs]) = &headerexpr.kind;
if let ExprKind::AddrOf(BorrowKind::Ref, _, lhs) = lhs.kind;
if let ExprKind::AddrOf(BorrowKind::Ref, _, rhs) = rhs.kind;
then {
let mut vec_arg = vec![lhs, rhs];
if_chain! {
if !arms.is_empty();
if let ExprKind::Block(Block{expr: Some(if_expr),..},_) = arms[0].body.kind;
Expand All @@ -446,47 +495,43 @@ pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option<Vec<&'tcx
| StmtKind::Semi(call_assert_failed) = stmts_if_block[1].kind;
if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind;
if args_assert_failed.len() >= 4;
if let ExprKind::Call(_, args) = args_assert_failed[3].kind;
if !args.is_empty();
if let Some (mut format_arg_expn) = FormatArgsExpn::parse(&args[0]);
if let ExprKind::Call(_, [arg, ..]) = args_assert_failed[3].kind;
if let Some(format_arg_expn) = FormatArgsExpn::parse(&arg);
then {
vec_arg.push(format_arg_expn.format_string);
vec_arg.append(&mut format_arg_expn.value_args);
return Some(AssertExpn {
first_assert_argument: lhs,
second_assert_argument: Some(rhs),
format_arg: Some(format_arg_expn)
});
}
else {
return Some(AssertExpn {
first_assert_argument: lhs,
second_assert_argument:
Some(rhs),
format_arg: None
});
}
}
return Some(vec_arg);
}
}
None
}

if let ExprKind::Block(block, _) = e.kind {
if block.stmts.len() == 1 {
if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind {
// macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`)
if_chain! {
if let Some(If { cond, .. }) = If::hir(matchexpr);
if let ExprKind::Unary(UnOp::Not, condition) = cond.kind;
then {
return Some(vec![condition]);
}
}

// debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
if_chain! {
if let ExprKind::Block(matchblock,_) = matchexpr.kind;
if let Some(matchblock_expr) = matchblock.expr;
then {
return ast_matchblock(matchblock_expr);
}
}
/// Gives the argument as a vector
pub fn argument_vector(&self) -> Vec<&'tcx Expr<'tcx>> {
let mut expr_vec = vec![self.first_assert_argument];
if let Some(sec_agr) = self.second_assert_argument {
expr_vec.push(sec_agr);
}
if let Some(ref format_arg) = self.format_arg {
expr_vec.push(format_arg.format_string);
for arg in &format_arg.value_args {
expr_vec.push(arg)
}
} else if let Some(matchblock_expr) = block.expr {
// macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
return ast_matchblock(matchblock_expr);
}
expr_vec
}
None
}

/// A parsed `format!` expansion
Expand Down
138 changes: 138 additions & 0 deletions tests/ui/bool_assert_comparison.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// run-rustfix

#![warn(clippy::bool_assert_comparison)]

use std::ops::Not;

macro_rules! a {
() => {
true
};
}
macro_rules! b {
() => {
true
};
}

// Implements the Not trait but with an output type
// that's not bool. Should not suggest a rewrite
#[derive(Debug, Copy, Clone)]
#[allow(dead_code)]
enum ImplNotTraitWithoutBool {
VariantX(bool),
VariantY(u32),
}

impl PartialEq<bool> for ImplNotTraitWithoutBool {
fn eq(&self, other: &bool) -> bool {
match *self {
ImplNotTraitWithoutBool::VariantX(b) => b == *other,
_ => false,
}
}
}

impl Not for ImplNotTraitWithoutBool {
type Output = Self;

fn not(self) -> Self::Output {
match self {
ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b),
ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1),
ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0),
}
}
}

// This type implements the Not trait with an Output of
// type bool. Using assert!(..) must be suggested
#[derive(Debug, Clone, Copy)]
struct ImplNotTraitWithBool;

impl PartialEq<bool> for ImplNotTraitWithBool {
fn eq(&self, _other: &bool) -> bool {
false
}
}

impl Not for ImplNotTraitWithBool {
type Output = bool;

fn not(self) -> Self::Output {
true
}
}

fn main() {
let a = ImplNotTraitWithoutBool::VariantX(true);
let b = ImplNotTraitWithBool;

assert_eq!("a".len(), 1);
assert!(!"a".is_empty());
assert!("".is_empty());
assert!("".is_empty());
assert_eq!(a!(), b!());
assert_eq!(a!(), "".is_empty());
assert_eq!("".is_empty(), b!());
assert_eq!(a, true);
assert!(b);

assert_ne!("a".len(), 1);
assert!("a".is_empty());
assert!(!"".is_empty());
assert!(!"".is_empty());
assert_ne!(a!(), b!());
assert_ne!(a!(), "".is_empty());
assert_ne!("".is_empty(), b!());
assert_ne!(a, true);
assert!(!b);

debug_assert_eq!("a".len(), 1);
debug_assert!(!"a".is_empty());
debug_assert!("".is_empty());
debug_assert!("".is_empty());
debug_assert_eq!(a!(), b!());
debug_assert_eq!(a!(), "".is_empty());
debug_assert_eq!("".is_empty(), b!());
debug_assert_eq!(a, true);
debug_assert!(b);

debug_assert_ne!("a".len(), 1);
debug_assert!("a".is_empty());
debug_assert!(!"".is_empty());
debug_assert!(!"".is_empty());
debug_assert_ne!(a!(), b!());
debug_assert_ne!(a!(), "".is_empty());
debug_assert_ne!("".is_empty(), b!());
debug_assert_ne!(a, true);
debug_assert!(!b);

// assert with error messages
assert_eq!("a".len(), 1, "tadam {}", 1);
assert_eq!("a".len(), 1, "tadam {}", true);
assert!(!"a".is_empty(), "tadam {}", 1);
assert!(!"a".is_empty(), "tadam {}", true);
assert!(!"a".is_empty(), "tadam {}", true);
assert_eq!(a, true, "tadam {}", false);
assert!("a".is_empty(), "tadam {} {}", false, 6);

debug_assert_eq!("a".len(), 1, "tadam {}", 1);
debug_assert_eq!("a".len(), 1, "tadam {}", true);
debug_assert!(!"a".is_empty(), "tadam {}", 1);
debug_assert!(!"a".is_empty(), "tadam {}", true);
debug_assert!(!"a".is_empty(), "tadam {}", true);
debug_assert_eq!(a, true, "tadam {}", false);
debug_assert!("a".is_empty(), "tadam {} {}", false, "b");

// Without ; at the end
{
debug_assert!(!"a".is_empty(), "tadam {}", true);
};
{
assert_eq!("a".len(), 1, "tadam {}", 1)
};
{
assert_ne!("a".len(), 1, "tadam {}", true)
};
}
Loading

0 comments on commit ce66869

Please sign in to comment.