Add ASSERTIONS_ON_COLLECTION_EMPTINESS lint

[MihirMohapatra]

Add ASSERTIONS_ON_COLLECTION_EMPTINESS lint

Suggests replacing assert!(x.is_empty()) and assert!(!x.is_empty())
with assert_eq!(x, []) / assert_ne!(x, []) (or assert_eq!(x, "") /
assert_ne!(x, "") for String types), so the collection contents are
shown on failure.

changelog: Added [assertions_on_collection_emptiness] to nursery

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @dswij (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[github-actions]

Lintcheck changes for 0470b43

Lint	Added	Removed	Changed
clippy::assert_is_empty	97	0	0

This comment will be updated if you push new changes

[dswij]

r? clippy

[llogiq]

This looks like a good start. We should probably also have a test for macro-expanded assertions, and perhaps assertions outside test code.

View changes since this review

[DanielEScherzer]

Community review: assert_eq! and assert_ne! will fail to compile unless the collection elements implement at least PartialEq and Debug. Based on my reading of the code, the collection element typed is not checked at all - can you confirm that for elements that cannot be compared with assert_eq! (or assert_ne!) there isn't an invalid suggestion? Maybe add a test to confirm?

View changes since this review

[joshka]

Thanks for pushing this forward. I had already put up a deeper implementation for the same lint in #17149, so I took a pass through this PR and the review comments to see whether there was anything useful to carry over there.

The main useful bit I found here is the ExprKind::If fast-path before walking macro backtraces. That is a good perf guard for assert-like macros, and I’ve carried it over to #17149 in 7763bb8.

The two PRs are also at fairly different depths. This PR is around 250 lines of changes, while #17149 is currently around 755 inserted lines. That extra size is mostly in type checks, source-preserving suggestions, docs, and UI coverage rather than just implementation bulk.

A few gaps in this implementation are already covered over there:

• It checks the Debug and PartialEq requirements before suggesting assert_eq! / assert_ne!. Without that, the lint can produce suggestions that do not compile for element types that cannot be printed or compared.
• It uses typed empty arrays for Vec<T> suggestions:

assert_eq!(items, [] as [T; 0]);
assert_ne!(items, [] as [T; 0]);

That avoids type inference failures from a bare [].

• It handles debug_assert! by suggesting debug_assert_eq! / debug_assert_ne!, preserving the debug-only assertion behavior.
• It handles borrowed vectors, mutable borrowed vectors, arrays, borrowed arrays, slices, String, and str with suggestions that type-check.
• It preserves custom assertion messages by editing only the macro suffix and the condition expression, rather than replacing the whole macro call.
• It has negative coverage for unsupported or unsafe suggestions, including missing Debug, missing PartialEq, both missing, map-like collections without a compact empty literal, and macro expansion cases.

I also prefer the lint docs and diagnostic wording in #17149. The docs there spell out the actual failure mode this lint is aimed at: in CI or another remote test service, an assert!(items.is_empty()) failure tells you the boolean check failed but not what values were present, so the usual next step is reproducing locally, adding temporary logging, or changing the test to expose the value. The docs also cover the common chained shape where an emptiness assertion prevents a later content assertion from running.

The diagnostic wording in #17149 is also more precise about both the checked state and the reason for the replacement. This PR currently describes the predicate as an "empty collection check" / "non-empty collection check" and uses a generic "replace with" help. That is understandable, but it leaves the reader to infer why the replacement is preferred and it gets a little awkward for the negated case.

For example, this PR currently reports a non-empty check like this:

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, [])`

#17149 reports the same shape like this:

error: used `assert!` to check that a value is not empty
  --> tests/ui/assert_is_empty.rs:9:5
   |
LL |     assert!(!vec.is_empty());
   |     ^^^^^^^^^^^^^^^^^^^^^^^^
   |
help: use `assert_ne!` to show the value on failure
   |
LL -     assert!(!vec.is_empty());
LL +     assert_ne!(vec, [] as [i32; 0]);
   |

The empty case follows the same pattern:

error: used `assert!` to check that a value is empty
  --> tests/ui/assert_is_empty.rs:7:5
   |
LL |     assert!(vec.is_empty());
   |     ^^^^^^^^^^^^^^^^^^^^^^^
   |
help: use `assert_eq!` to show the value on failure
   |
LL -     assert!(vec.is_empty());
LL +     assert_eq!(vec, [] as [i32; 0]);
   |

That wording ties the diagnostic to the actual assertion meaning (empty vs. not empty) and makes the help message explain the diagnostic value of the replacement: the suggested assertion shows the value on failure.

On macro expansion specifically: this PR emits help-only diagnostics for macro-expanded assert macros. That means it still lints, but it prints textual guidance instead of a machine-applicable rewrite because the relevant span comes from generated code. #17149 intentionally skips macro-expanded assertion conditions/assertions instead.

I think that is the better first-version behavior. If the lint fires inside another macro’s expansion, the diagnostic may identify a real pattern, but the user often cannot fix it at the call site, and changing the macro definition may not be correct for every caller. That makes the warning noisier and harder to act on. Keeping the lint on source-level assertions preserves the stronger property that a warning points at code the user can usually rewrite directly.

Given the overlap, I think it would be best to consolidate review on #17149 and avoid splitting the discussion across two implementations.