Skip to content

Commit

Permalink
Switch from macro_expansion to early pass
Browse files Browse the repository at this point in the history
  • Loading branch information
@GuillaumeGomez
GuillaumeGomez committed Apr 16, 2021
1 parent 47fdfc8 commit 66f5ecb
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 99 deletions.
124 changes: 36 additions & 88 deletions clippy_lints/src/bool_assert_comparison.rs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet;
use rustc_ast::ast::{MacArgs, MacCall};
use rustc_ast::token::{Token, TokenKind};
use rustc_ast::tokenstream::TokenTree;
use clippy_utils::{ast_utils, is_direct_expn_of};
use rustc_ast::ast::{Expr, ExprKind, Lit, LitKind};
use rustc_errors::Applicability;
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::kw::{False, True};
use rustc_span::DUMMY_SP;

declare_clippy_lint! {
/// **What it does:** This lint warns about boolean comparisons in assert-like macros.
Expand All @@ -32,96 +30,46 @@ declare_clippy_lint! {

declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);

fn is_bool(tt: Option<&TokenTree>) -> bool {
match tt {
Some(TokenTree::Token(Token {
kind: TokenKind::Ident(i, _),
..
})) => *i == True || *i == False,
_ => false,
}
}

impl EarlyLintPass for BoolAssertComparison {
fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &MacCall) {
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
let macros = ["assert_eq", "debug_assert_eq"];
let inverted_macros = ["assert_ne", "debug_assert_ne"];

if let MacArgs::Delimited(_, _, ts) = &*mac.args {
let name;
if let [seg] = &*mac.path.segments {
name = seg.ident.name.as_str().to_string();
if !macros.contains(&name.as_str()) && !inverted_macros.contains(&name.as_str()) {
return;
}
} else {
return;
}
let mut has_true = false;
let mut has_false = false;
let mut bool_span = None;
let mut spans = Vec::new();
let mut nb_comma = 0;
let mut iter = ts.trees().peekable();
while let Some(tt) = iter.next() {
if let TokenTree::Token(ref token) = tt {
match token.kind {
TokenKind::Ident(i, _) => {
// We only want to check the comparison arguments, nothing else!
if nb_comma < 2 {
if i == True {
has_true = true;
bool_span = Some(token.span);
continue;
} else if i == False {
has_false = true;
bool_span = Some(token.span);
continue;
}
}
},
TokenKind::Comma => {
nb_comma += 1;
if nb_comma > 1 || !is_bool(iter.peek()) {
spans.push((token.span, true));
}
continue;
},
_ => {},
for mac in macros.iter().chain(inverted_macros.iter()) {
if is_direct_expn_of(e.span, mac).is_some() {
if let Some(macro_args) = ast_utils::extract_assert_macro_args(e) {
let mut nb_bool_args = 0;
let mut bool_span = DUMMY_SP;
for arg in macro_args {
if let ExprKind::Lit(Lit {
kind: LitKind::Bool(_), ..
}) = arg.kind
{
nb_bool_args += 1;
bool_span = arg.span;
}
}
}
spans.push((tt.span(), false));
}
#[allow(clippy::if_same_then_else)] // It allows better code reability here.
if has_false && has_true {
// Don't know what's going on here, but better leave things as is...
return;
} else if !has_true && !has_false {
// No problem detected!
return;
}
let text = spans
.into_iter()
.map(|(s, needs_whitespace)| {
let mut s = snippet(cx, s, "arg").to_string();
if needs_whitespace {
s.push(' ');
#[allow(clippy::if_same_then_else)] // It allows better code reability here.
if nb_bool_args > 1 {
// Don't know what's going on here, but better leave things as is...
return;
} else if nb_bool_args == 0 {
// No problem detected!
return;
}
s
})
.collect::<String>();
let is_cond_inverted = inverted_macros.contains(&name.as_str());
let extra = (has_true && is_cond_inverted) || has_false;

span_lint_and_sugg(
cx,
BOOL_ASSERT_COMPARISON,
bool_span.expect("failed to get the bool span"),
"assert macro with a boolean comparison",
"replace it with",
format!("{}!({}{})", &name[..name.len() - 3], if extra { "!" } else { "" }, text),
Applicability::MachineApplicable,
);
span_lint_and_sugg(
cx,
BOOL_ASSERT_COMPARISON,
bool_span,
"assert macro with a boolean comparison",
"replace it with",
format!("{}!", &mac[..mac.len() - 3]),
Applicability::MachineApplicable,
);
}
break;
}
}
}
}
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,6 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) {
store.register_pre_expansion_pass(|| box write::Write::default());
store.register_pre_expansion_pass(|| box attrs::EarlyAttributes);
store.register_pre_expansion_pass(|| box dbg_macro::DbgMacro);
store.register_pre_expansion_pass(|| box bool_assert_comparison::BoolAssertComparison);
}

#[doc(hidden)]
Expand Down Expand Up @@ -1273,6 +1272,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
store.register_late_pass(|| box manual_map::ManualMap);
store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv));
store.register_early_pass(|| box bool_assert_comparison::BoolAssertComparison);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(arithmetic::FLOAT_ARITHMETIC),
Expand Down
34 changes: 34 additions & 0 deletions clippy_utils/src/ast_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#![allow(clippy::similar_names, clippy::wildcard_imports, clippy::enum_glob_use)]

use crate::{both, over};
use if_chain::if_chain;
use rustc_ast::ptr::P;
use rustc_ast::{self as ast, *};
use rustc_span::symbol::Ident;
Expand Down Expand Up @@ -571,3 +572,36 @@ pub fn eq_mac_args(l: &MacArgs, r: &MacArgs) -> bool {
_ => false,
}
}

/// 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(e: &Expr) -> Option<Vec<&P<Expr>>> {
fn get_args(e: &Expr) -> Option<Vec<&P<Expr>>> {
if_chain! {
if let ExprKind::Block(ref block, _) = e.kind;
if let StmtKind::Expr(ref expr) = block.stmts.get(0)?.kind;
if let ExprKind::Match(ref match_expr, _) = expr.kind;
if let ExprKind::Tup(ref tup) = match_expr.kind;
then {
return Some(tup.iter().filter_map(|t| match t.kind {
ExprKind::AddrOf(_, _, ref expr) => Some(expr),
_ => None,
}).collect::<Vec<_>>());
}
}
None
}
if_chain! {
if let ExprKind::If(_, ref block, _) = e.kind;
if let StmtKind::Semi(ref e) = block.stmts.get(0)?.kind;
then {
return get_args(e);
}
}
get_args(e)
}
20 changes: 10 additions & 10 deletions tests/ui/bool_assert_comparison.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:5:32
|
LL | assert_eq!("a".is_empty(), false);
| ^^^^^ help: replace it with: `assert!(!"a".is_empty())`
| ^^^^^ help: replace it with: `assert!`
|
= note: `-D clippy::bool-assert-comparison` implied by `-D warnings`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:6:31
|
LL | assert_eq!("".is_empty(), true);
| ^^^^ help: replace it with: `assert!("".is_empty())`
| ^^^^ help: replace it with: `assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:7:16
Expand All @@ -22,13 +22,13 @@ error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:10:32
|
LL | assert_ne!("a".is_empty(), false);
| ^^^^^ help: replace it with: `assert!(!"a".is_empty())`
| ^^^^^ help: replace it with: `assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:11:31
|
LL | assert_ne!("".is_empty(), true);
| ^^^^ help: replace it with: `assert!(!"".is_empty())`
| ^^^^ help: replace it with: `assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:12:16
Expand All @@ -40,13 +40,13 @@ error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:15:38
|
LL | debug_assert_eq!("a".is_empty(), false);
| ^^^^^ help: replace it with: `debug_assert!(!"a".is_empty())`
| ^^^^^ help: replace it with: `debug_assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:16:37
|
LL | debug_assert_eq!("".is_empty(), true);
| ^^^^ help: replace it with: `debug_assert!("".is_empty())`
| ^^^^ help: replace it with: `debug_assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:17:22
Expand All @@ -58,13 +58,13 @@ error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:20:38
|
LL | debug_assert_ne!("a".is_empty(), false);
| ^^^^^ help: replace it with: `debug_assert!(!"a".is_empty())`
| ^^^^^ help: replace it with: `debug_assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:21:37
|
LL | debug_assert_ne!("".is_empty(), true);
| ^^^^ help: replace it with: `debug_assert!(!"".is_empty())`
| ^^^^ help: replace it with: `debug_assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:22:22
Expand All @@ -76,13 +76,13 @@ error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:27:32
|
LL | assert_eq!("a".is_empty(), false, "tadam {}", 1);
| ^^^^^ help: replace it with: `assert!(!"a".is_empty(), "tadam {}", 1)`
| ^^^^^ help: replace it with: `assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:28:32
|
LL | assert_eq!("a".is_empty(), false, "tadam {}", true);
| ^^^^^ help: replace it with: `assert!(!"a".is_empty(), "tadam {}", true)`
| ^^^^^ help: replace it with: `assert!`

error: assert macro with a boolean comparison
--> $DIR/bool_assert_comparison.rs:29:16
Expand Down

0 comments on commit 66f5ecb

Please sign in to comment.