diff --git a/CHANGELOG.md b/CHANGELOG.md index 689d56d0fb7c..4982ad10a107 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6639,6 +6639,7 @@ Released 2018-09-13 [`as_pointer_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_pointer_underscore [`as_ptr_cast_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut [`as_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore +[`assert_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#assert_is_empty [`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants [`assertions_on_result_states`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_result_states [`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern diff --git a/clippy_lints/src/assert_is_empty.rs b/clippy_lints/src/assert_is_empty.rs new file mode 100644 index 000000000000..5d6534744c79 --- /dev/null +++ b/clippy_lints/src/assert_is_empty.rs @@ -0,0 +1,105 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::macros::{find_assert_args, first_node_macro_backtrace}; +use clippy_utils::res::MaybeDef; +use clippy_utils::source::snippet_with_context; +use clippy_utils::sym; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, LangItem, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for `assert!(s.is_empty())` and `assert!(!s.is_empty())`. + /// + /// ### Why is this bad? + /// `assert!` only reports the condition was false, not what the collection + /// contained. Using `assert_eq!` / `assert_ne!` with an empty literal + /// prints the collection contents on failure, making debugging faster. + /// + /// ### Example + /// ```no_run + /// # let items = vec![1, 2, 3]; + /// assert!(items.is_empty()); + /// assert!(!items.is_empty()); + /// ``` + /// + /// Use instead: + /// ```no_run + /// # let items = vec![1, 2, 3]; + /// assert_eq!(items, []); + /// assert_ne!(items, []); + /// ``` + #[clippy::version = "1.97.0"] + pub ASSERT_IS_EMPTY, + pedantic, + "asserting on `.is_empty()` without showing the collection contents" +} + +declare_lint_pass!(AssertIsEmpty => [ASSERT_IS_EMPTY]); + +impl<'tcx> LateLintPass<'tcx> for AssertIsEmpty { + fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { + if !matches!(e.kind, ExprKind::If(..)) { + return; + } + + if let Some(macro_call) = first_node_macro_backtrace(cx, e).find(|macro_call| { + matches!( + cx.tcx.get_diagnostic_name(macro_call.def_id), + Some(sym::assert_macro | sym::debug_assert_macro), + ) + }) && matches!( + cx.tcx.get_diagnostic_name(macro_call.def_id), + Some(sym::assert_macro | sym::debug_assert_macro), + ) && let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) + && panic_expn.is_default_message() + { + let (method_name, recv, is_negated) = match condition.kind { + ExprKind::MethodCall(ms, r, [], _) => (ms.ident.name, r, false), + ExprKind::Unary(UnOp::Not, inner) if let ExprKind::MethodCall(ms, r, [], _) = inner.kind => { + (ms.ident.name, r, true) + }, + _ => return, + }; + + if method_name != sym::is_empty { + return; + } + + let message = if is_negated { + "used `assert!` with a non-empty collection check" + } else { + "used `assert!` with an empty collection check" + }; + + let recv_ty = cx.typeck_results().expr_ty(recv); + let empty_literal = empty_literal_for_type(cx, recv_ty); + + span_lint_and_then(cx, ASSERT_IS_EMPTY, macro_call.span, message, |diag| { + let mut app = Applicability::MachineApplicable; + let recv_snippet = snippet_with_context(cx, recv.span, condition.span.ctxt(), "..", &mut app).0; + + let sugg = if is_negated { + format!("assert_ne!({recv_snippet}, {empty_literal})") + } else { + format!("assert_eq!({recv_snippet}, {empty_literal})") + }; + if macro_call.span.from_expansion() { + diag.help(format!("replace with: `{sugg}`")); + } else { + diag.span_suggestion(macro_call.span, "replace with", sugg, app); + } + }); + } + } +} + +fn empty_literal_for_type(cx: &LateContext<'_>, ty: Ty<'_>) -> &'static str { + if ty.is_str() || ty.peel_refs().is_str() || ty.is_lang_item(cx, LangItem::String) { + "\"\"" + } else { + "[]" + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 51a848d022d8..28745f5ea81b 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -11,6 +11,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::as_conversions::AS_CONVERSIONS_INFO, crate::asm_syntax::INLINE_ASM_X86_ATT_SYNTAX_INFO, crate::asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX_INFO, + crate::assert_is_empty::ASSERT_IS_EMPTY_INFO, crate::assertions_on_constants::ASSERTIONS_ON_CONSTANTS_INFO, crate::assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES_INFO, crate::assigning_clones::ASSIGNING_CLONES_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8f550786819e..c94f653d12f4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -69,6 +69,7 @@ mod arbitrary_source_item_ordering; mod arc_with_non_send_sync; mod as_conversions; mod asm_syntax; +mod assert_is_empty; mod assertions_on_constants; mod assertions_on_result_states; mod assigning_clones; @@ -665,6 +666,7 @@ rustc_lint::late_lint_methods!( RedundantClone: redundant_clone::RedundantClone = redundant_clone::RedundantClone, SlowVectorInit: slow_vector_initialization::SlowVectorInit = slow_vector_initialization::SlowVectorInit, UnnecessaryWraps: unnecessary_wraps::UnnecessaryWraps = unnecessary_wraps::UnnecessaryWraps::new(conf), + AssertIsEmpty: assert_is_empty::AssertIsEmpty = assert_is_empty::AssertIsEmpty, AssertionsOnConstants: assertions_on_constants::AssertionsOnConstants = assertions_on_constants::AssertionsOnConstants::new(conf), AssertionsOnResultStates: assertions_on_result_states::AssertionsOnResultStates = assertions_on_result_states::AssertionsOnResultStates, InherentToString: inherent_to_string::InherentToString = inherent_to_string::InherentToString, diff --git a/tests/lint_message_convention.rs b/tests/lint_message_convention.rs index 9229e2e8c496..7d8d832fba71 100644 --- a/tests/lint_message_convention.rs +++ b/tests/lint_message_convention.rs @@ -6,7 +6,7 @@ use std::sync::LazyLock; use regex::RegexSet; -#[derive(Debug)] +#[derive(Debug, PartialEq)] struct Message { path: PathBuf, bad_lines: Vec, @@ -113,5 +113,5 @@ fn lint_message_convention() { eprintln!("Check out the rustc-dev-guide for more information:"); eprintln!("https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-structure\n\n\n"); - assert!(bad_tests.is_empty()); + assert_eq!(bad_tests, []); } diff --git a/tests/ui/assert_is_empty.fixed b/tests/ui/assert_is_empty.fixed new file mode 100644 index 000000000000..a386c4e175c9 --- /dev/null +++ b/tests/ui/assert_is_empty.fixed @@ -0,0 +1,29 @@ +#![warn(clippy::assert_is_empty)] +#![allow(clippy::len_zero, clippy::comparison_to_empty)] + +fn main() { + // Test with Vec + let v: Vec = vec![]; + assert_eq!(v, []); + //~^ assert_is_empty + assert_ne!(v, []); + //~^ assert_is_empty + + // Test with String + let s = String::new(); + assert_eq!(s, ""); + //~^ assert_is_empty + assert_ne!(s, ""); + //~^ assert_is_empty + + // Should not lint: custom message + assert!(v.is_empty(), "vec is not empty"); + assert!(!v.is_empty(), "vec is empty"); + + // Should not lint: assert_ne!/assert_eq! already fine + assert_eq!(v, []); + assert_ne!(v, []); + + // Should not lint: not is_empty + assert!(v.len() == 0); +} diff --git a/tests/ui/assert_is_empty.rs b/tests/ui/assert_is_empty.rs new file mode 100644 index 000000000000..7bc439b7176e --- /dev/null +++ b/tests/ui/assert_is_empty.rs @@ -0,0 +1,29 @@ +#![warn(clippy::assert_is_empty)] +#![allow(clippy::len_zero, clippy::comparison_to_empty)] + +fn main() { + // Test with Vec + let v: Vec = vec![]; + assert!(v.is_empty()); + //~^ assert_is_empty + assert!(!v.is_empty()); + //~^ assert_is_empty + + // Test with String + let s = String::new(); + assert!(s.is_empty()); + //~^ assert_is_empty + assert!(!s.is_empty()); + //~^ assert_is_empty + + // Should not lint: custom message + assert!(v.is_empty(), "vec is not empty"); + assert!(!v.is_empty(), "vec is empty"); + + // Should not lint: assert_ne!/assert_eq! already fine + assert_eq!(v, []); + assert_ne!(v, []); + + // Should not lint: not is_empty + assert!(v.len() == 0); +} diff --git a/tests/ui/assert_is_empty.stderr b/tests/ui/assert_is_empty.stderr new file mode 100644 index 000000000000..f374d000c8a9 --- /dev/null +++ b/tests/ui/assert_is_empty.stderr @@ -0,0 +1,29 @@ +error: used `assert!` with an empty collection check + --> tests/ui/assert_is_empty.rs:7:5 + | +LL | assert!(v.is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^ help: replace with: `assert_eq!(v, [])` + | + = note: `-D clippy::assert-is-empty` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::assert_is_empty)]` + +error: used `assert!` with a non-empty collection check + --> tests/ui/assert_is_empty.rs:9:5 + | +LL | assert!(!v.is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `assert_ne!(v, [])` + +error: used `assert!` with an empty collection check + --> tests/ui/assert_is_empty.rs:14:5 + | +LL | assert!(s.is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^ help: replace with: `assert_eq!(s, "")` + +error: used `assert!` with a non-empty collection check + --> tests/ui/assert_is_empty.rs:16:5 + | +LL | assert!(!s.is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `assert_ne!(s, "")` + +error: aborting due to 4 previous errors + diff --git a/tests/ui/assert_is_empty_macro.rs b/tests/ui/assert_is_empty_macro.rs new file mode 100644 index 000000000000..639e7c3bd56e --- /dev/null +++ b/tests/ui/assert_is_empty_macro.rs @@ -0,0 +1,24 @@ +//@no-rustfix: macro-expanded suggestions are intentionally help-only +#![warn(clippy::assert_is_empty)] + +macro_rules! assert_empty { + ($expr:expr) => { + assert!($expr.is_empty()); + //~^ assert_is_empty + }; +} + +macro_rules! assert_not_empty { + ($expr:expr) => { + assert!(!$expr.is_empty()); + //~^ assert_is_empty + }; +} + +fn main() { + let v: Vec = vec![]; + let s = String::new(); + + assert_empty!(v); + assert_not_empty!(s); +} diff --git a/tests/ui/assert_is_empty_macro.stderr b/tests/ui/assert_is_empty_macro.stderr new file mode 100644 index 000000000000..c0fcf673b951 --- /dev/null +++ b/tests/ui/assert_is_empty_macro.stderr @@ -0,0 +1,28 @@ +error: used `assert!` with an empty collection check + --> tests/ui/assert_is_empty_macro.rs:6:9 + | +LL | assert!($expr.is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | assert_empty!(v); + | ---------------- in this macro invocation + | + = help: replace with: `assert_eq!(v, [])` + = note: `-D clippy::assert-is-empty` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::assert_is_empty)]` + = note: this error originates in the macro `assert_empty` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: used `assert!` with a non-empty collection check + --> tests/ui/assert_is_empty_macro.rs:13:9 + | +LL | assert!(!$expr.is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | assert_not_empty!(s); + | -------------------- in this macro invocation + | + = help: replace with: `assert_ne!(s, "")` + = note: this error originates in the macro `assert_not_empty` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 2 previous errors +