diff --git a/CHANGELOG.md b/CHANGELOG.md index c9adf77c0d63..5bf72d8e8b21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3057,6 +3057,7 @@ Released 2018-09-13 [`blanket_clippy_restriction_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#blanket_clippy_restriction_lints [`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions [`bool_assert_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison +[`bool_assert_inverted`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_inverted [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison [`borrow_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_as_ptr [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index c50e214be288..eb2d7965d751 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -26,7 +26,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.53.0"] pub BOOL_ASSERT_COMPARISON, - style, + pedantic, "Using a boolean as comparison value in an assert_* macro when there is no need" } diff --git a/clippy_lints/src/bool_assert_inverted.rs b/clippy_lints/src/bool_assert_inverted.rs new file mode 100644 index 000000000000..1f451db260b9 --- /dev/null +++ b/clippy_lints/src/bool_assert_inverted.rs @@ -0,0 +1,64 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::macros::{find_assert_args, root_macro_call_first_node}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// This lint warns about the use of inverted conditions in assert-like macros. + /// + /// ### Why is this bad? + /// It is all too easy to misread the semantics of an assertion when the + /// logic of the condition is reversed. Explicitly comparing to a boolean + /// value is preferable. + /// + /// ### Example + /// ```rust + /// // Bad + /// assert!(!"a".is_empty()); + /// + /// // Good + /// assert_eq!("a".is_empty(), false); + /// + /// // Okay + /// assert_ne!("a".is_empty(), true); + /// ``` + #[clippy::version = "1.58.0"] + pub BOOL_ASSERT_INVERTED, + restriction, + "Asserting on an inverted condition" +} + +declare_lint_pass!(BoolAssertInverted => [BOOL_ASSERT_INVERTED]); + +fn is_inverted(e: &Expr<'_>) -> bool { + matches!(e.kind, ExprKind::Unary(UnOp::Not, _),) && !e.span.from_expansion() +} + +impl<'tcx> LateLintPass<'tcx> for BoolAssertInverted { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return }; + let macro_name = cx.tcx.item_name(macro_call.def_id); + if !matches!(macro_name.as_str(), "assert" | "debug_assert") { + return; + } + let Some ((a, _)) = find_assert_args(cx, expr, macro_call.expn) else { return }; + if !is_inverted(a) { + return; + } + + let macro_name = macro_name.as_str(); + let eq_mac = format!("{}_eq", macro_name); + span_lint_and_sugg( + cx, + BOOL_ASSERT_INVERTED, + macro_call.span, + &format!("used `{}!` with an inverted condition", macro_name), + "replace it with", + format!("{}!(.., false, ..)", eq_mac), + Applicability::MaybeIncorrect, + ); + } +} diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index d93e34e76b49..0e491041f487 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -18,7 +18,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(bit_mask::INEFFECTIVE_BIT_MASK), LintId::of(blacklisted_name::BLACKLISTED_NAME), LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), - LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(booleans::LOGIC_BUG), LintId::of(booleans::NONMINIMAL_BOOL), LintId::of(casts::CAST_REF_TO_MUT), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 7a62a99f5ffb..b61c7b6637cc 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -57,6 +57,7 @@ store.register_lints(&[ blacklisted_name::BLACKLISTED_NAME, blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS, bool_assert_comparison::BOOL_ASSERT_COMPARISON, + bool_assert_inverted::BOOL_ASSERT_INVERTED, booleans::LOGIC_BUG, booleans::NONMINIMAL_BOOL, borrow_as_ptr::BORROW_AS_PTR, diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 1292675f4a96..ae4a945e4d82 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -7,6 +7,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK), LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(bit_mask::VERBOSE_BIT_MASK), + LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(borrow_as_ptr::BORROW_AS_PTR), LintId::of(bytecount::NAIVE_BYTECOUNT), LintId::of(case_sensitive_file_extension_comparisons::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS), diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index 5a89fdb05a99..c2888b13ac3d 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -8,6 +8,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(as_conversions::AS_CONVERSIONS), LintId::of(asm_syntax::INLINE_ASM_X86_ATT_SYNTAX), LintId::of(asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX), + LintId::of(bool_assert_inverted::BOOL_ASSERT_INVERTED), LintId::of(casts::FN_TO_NUMERIC_CAST_ANY), LintId::of(create_dir::CREATE_DIR), LintId::of(dbg_macro::DBG_MACRO), diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index 05211476ff23..f1b791dd10d4 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -7,7 +7,6 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(assign_ops::ASSIGN_OP_PATTERN), LintId::of(blacklisted_name::BLACKLISTED_NAME), LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), - LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(casts::FN_TO_NUMERIC_CAST), LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e78f61873593..0ea634cf46ff 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -175,6 +175,7 @@ mod bit_mask; mod blacklisted_name; mod blocks_in_if_conditions; mod bool_assert_comparison; +mod bool_assert_inverted; mod booleans; mod borrow_as_ptr; mod bytecount; @@ -821,6 +822,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(manual_map::ManualMap)); store.register_late_pass(move || Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv))); store.register_late_pass(|| Box::new(bool_assert_comparison::BoolAssertComparison)); + store.register_late_pass(|| Box::new(bool_assert_inverted::BoolAssertInverted)); store.register_early_pass(move || Box::new(module_style::ModStyle)); store.register_late_pass(|| Box::new(unused_async::UnusedAsync)); let disallowed_types = conf.disallowed_types.clone(); diff --git a/tests/ui/bool_assert_inverted.rs b/tests/ui/bool_assert_inverted.rs new file mode 100644 index 000000000000..9be5ae94d2cd --- /dev/null +++ b/tests/ui/bool_assert_inverted.rs @@ -0,0 +1,55 @@ +#![warn(clippy::bool_assert_inverted)] + +use std::ops::Not; + +macro_rules! a { + () => { + true + }; +} +macro_rules! b { + () => { + true + }; +} + +#[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 = ImplNotTraitWithBool; + + assert!(!"a".is_empty()); + assert!("".is_empty()); + assert!(!a); + assert!(a); + + debug_assert!(!"a".is_empty()); + debug_assert!("".is_empty()); + debug_assert!(!a); + debug_assert!(a); + + assert!(!"a".is_empty(), "tadam {}", false); + assert!("".is_empty(), "tadam {}", false); + assert!(!a, "tadam {}", false); + assert!(a, "tadam {}", false); + + debug_assert!(!"a".is_empty(), "tadam {}", false); + debug_assert!("".is_empty(), "tadam {}", false); + debug_assert!(!a, "tadam {}", false); + debug_assert!(a, "tadam {}", false); +} diff --git a/tests/ui/bool_assert_inverted.stderr b/tests/ui/bool_assert_inverted.stderr new file mode 100644 index 000000000000..a48282145608 --- /dev/null +++ b/tests/ui/bool_assert_inverted.stderr @@ -0,0 +1,28 @@ +error: used `debug_assert!` with an inverted condition + --> $DIR/bool_assert_inverted.rs:41:5 + | +LL | debug_assert!(!"a".is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)` + | + = note: `-D clippy::bool-assert-inverted` implied by `-D warnings` + +error: used `debug_assert!` with an inverted condition + --> $DIR/bool_assert_inverted.rs:43:5 + | +LL | debug_assert!(!a); + | ^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)` + +error: used `debug_assert!` with an inverted condition + --> $DIR/bool_assert_inverted.rs:51:5 + | +LL | debug_assert!(!"a".is_empty(), "tadam {}", false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)` + +error: used `debug_assert!` with an inverted condition + --> $DIR/bool_assert_inverted.rs:53:5 + | +LL | debug_assert!(!a, "tadam {}", false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)` + +error: aborting due to 4 previous errors +