Add assert_is_empty lint

[joshka]

View all comments

changelog: new lint: [assert_is_empty]

fixes #17114

Adds a pedantic lint for assert! and debug_assert! calls without custom messages that only check whether a supported value is empty. The lint suggests assert_eq!, assert_ne!, debug_assert_eq!, or debug_assert_ne! forms so failures print the unexpected value.

Rationale:

Boolean emptiness assertions only report that the predicate failed. In test failures, especially CI failures, that often leaves the developer without the collection contents needed to diagnose the problem. Comparing against an empty value gives the assertion machinery a value to print, and it also avoids hiding later content checks such as assert_eq!(items[0], "bar") behind a preceding assert!(!items.is_empty()) failure.

Supported suggestions currently cover strings, slices, arrays, and Vec. Other collection types are skipped when there is no compact empty-literal comparison or when the replacement assertion would not provide useful failure output.

Implementation notes:

• Assertions with custom messages are skipped, following the manual_assert_eq precedent that a custom message is a signal that the author chose the diagnostic output.
• Vec<T> suggestions use [] as [T; 0] so the empty operand carries enough type information for both assert_eq! and assert_ne!.
• Arrays and borrowed vectors are compared through .as_slice() because direct comparison with [] does not compile for those receiver types.
• Suggestions are only emitted when the replacement assertion can compare and print the value. Sequence element types must implement Debug and PartialEq.
• Macro-expanded assertion conditions are skipped because rewriting generated spans can produce confusing or invalid suggestions.
• Map-like collections are intentionally not covered yet. Comparing against HashMap::new() may be worth revisiting, but this PR keeps the first version to compact empty-literal comparisons.
• The lint docs call out that printing the asserted value can be the wrong tradeoff for very large values or values containing sensitive information. In those cases, keep the boolean assertion and allow the lint at that assertion.

New-lint checklist:

• Followed lint naming conventions
• Added passing UI tests, including committed .stderr and .fixed files
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

Validation run locally:

• cargo dev update_lints
• cargo dev fmt
• TESTNAME=assert_is_empty cargo uitest
• cargo test --test dogfood
• cargo test

[rustbot]

Thanks for the pull request, and welcome! The Rust team 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 4d2c39f

Lint	Added	Removed	Changed
clippy::assert_is_empty	43	0	0

This comment will be updated if you push new changes

[DanielEScherzer]

Just a note that it seems someone else has since had this idea at #17320
Looks like you properly checked for PartialEq and Debug that the other patch was missing :)

[joshka]

@rustbot assign @llogiq
@rustbot ready

[joshka]

Following up on Daniel’s note above: I reviewed #17320 and left the fuller gap analysis here: #17320 (comment).

The useful implementation detail from that PR was the early ExprKind::If check before walking macro backtraces. I carried that over here in 7763bb81ba0e. assert! and debug_assert! expand to if, so this lets the lint skip non-assert-shaped expressions before doing the more expensive macro-backtrace work.

The main differences from this side are intentional:

• This PR checks Debug and PartialEq before suggesting assert_eq! / assert_ne!, so the lint should not suggest replacements that fail to compile for element types that cannot be printed or compared.
• Vec<T> suggestions use a typed empty array, for example:

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

That avoids type inference failures from a bare [].

• This PR handles debug_assert!, borrowed vectors, mutable borrowed vectors, arrays, borrowed arrays, slices, String, and str.
• Suggestions preserve custom assertion messages by editing the macro suffix and condition expression instead of replacing the whole macro call.
• The UI coverage includes unsupported or unsafe suggestion cases, including missing Debug, missing PartialEq, both missing, map-like collections without a compact empty literal, and macro expansion cases.
• The lint docs here spell out the CI/debugging failure mode and the common chained-assertion case where an emptiness assertion prevents a later contents assertion from running.
• This PR intentionally skips macro-expanded assertion conditions/assertions. The duplicate PR instead emits help-only diagnostics for those cases. I think skipping is the better first-version behavior because macro-expanded diagnostics can point at code the user cannot fix at the call site, and changing the macro definition may not be correct for every caller.

One diagnostic wording difference that came up while comparing them: this PR now uses polarity-aware wording and explains why the replacement helps.

For a non-empty check, this PR reports:

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

The duplicate currently reports the same shape as:

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

Both point in the same direction, but I think the wording here is clearer: the diagnostic names the actual state being checked, and the help text says that the replacement is useful because it shows the value on failure.

[llogiq]

@rustbot label lint-nominated

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[llogiq]

Implementation looks good to me. Let's start the final comment period.

View changes since this review

[llogiq]

As @kpreid notes on zulip, we should document the potential drawback of divulging what's inside the nonempty vector/string, which could contain sensitive information.

[joshka]

Good point. I added a Known problems section for this in adac6320e27a.

I looked for nearby precedent before changing behavior. The closest shape I found was documenting the tradeoff and recommending a narrow allow where the lint is not appropriate, rather than changing the lint to distinguish test and non-test assertions. For example, cast_possible_truncation documents that the cast may be intentional and shows #[allow(clippy::cast_possible_truncation)] as one valid outcome. missing_assert_message does have special test-context handling, but it is a restriction lint whose docs explicitly say it would be too noisy in tests. This lint is pedantic and the main value is test/CI diagnostics, so I think documenting the drawback is the better first version.

The new docs now call out that printing the asserted value can be undesirable outside tests, especially for very large values or values containing sensitive information. In those cases, the suggested fix is to keep the boolean assertion and allow this lint at that assertion.

[llogiq]

We do have an is_in_test method. Together with a configuration option, we could by default make the lint only emit on test code.

[joshka]

We do have an is_in_test method. Together with a configuration option, we could by default make the lint only emit on test code.

I don't think we should, but I don't tend to use asserts in non test code and I don't know of any specific use cases that you would hit this based on that. Worth releasing, or cratering and seeing if it gets any push back?

[llogiq]

It's totally ok as is. Let's see if the FCP gets us more comments though.