Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing_assert_message lint #10362

Merged
merged 10 commits into from
Mar 8, 2023
Prev Previous commit
Next Next commit
Update lint description and add help section
Co-authored-by: Weihang Lo <me@weihanglo.tw>
  • Loading branch information
unexge and weihanglo committed Mar 8, 2023
commit 4eb6ccc9731095ed014bd5c047bcd7dd75bdf335
24 changes: 15 additions & 9 deletions clippy_lints/src/missing_assert_message.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use clippy_utils::diagnostics::span_lint;
use clippy_utils::diagnostics::span_lint_and_help;
use rustc_ast::ast;
use rustc_ast::{
token::{Token, TokenKind},
Expand All @@ -13,19 +13,23 @@ declare_clippy_lint! {
/// Checks assertions without a custom panic message.
///
/// ### Why is this bad?
/// If the assertion fails, the custom message may make it easier to understand what went wrong.
/// Without a good custom message, it'd be hard to understand what went wrong when the assertion fails.
/// A good custom message should be more about why the failure of the assertion is problematic
/// and not what is failed because the assertion already conveys that.
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a ### Known Problems section that admits we cannot check the assert messages for quality, and perhaps a link to some docs or guidelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the Known Problems section but couldn't find a good doc to link. The best thing I can find is the answers to https://softwareengineering.stackexchange.com/questions/301652/purpose-of-assertion-messages. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, let's not link to a site where the content could change at will. In that case, just leave the quality unspecified.

/// ### Example
/// ```rust
/// let threshold = 50;
/// let num = 42;
/// assert!(num < threshold);
/// # struct Service { ready: bool }
/// fn call(service: Service) {
/// assert!(service.ready);
/// }
/// ```
/// Use instead:
/// ```rust
/// let threshold = 50;
/// let num = 42;
/// assert!(num < threshold, "{num} is lower than threshold ({threshold})");
/// # struct Service { ready: bool }
/// fn call(service: Service) {
/// assert!(service.ready, "`service.poll_ready()` must be called first to ensure that service is ready to receive requests");
/// }
/// ```
#[clippy::version = "1.69.0"]
pub MISSING_ASSERT_MESSAGE,
unexge marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -56,11 +60,13 @@ impl EarlyLintPass for MissingAssertMessage {
let num_separators = num_commas_on_arguments(mac_call);

if num_separators < num_separators_needed {
span_lint(
span_lint_and_help(
cx,
MISSING_ASSERT_MESSAGE,
mac_call.span(),
"assert without any message",
None,
"consider describing why the failing assert is problematic",
);
}
}
Expand Down
31 changes: 31 additions & 0 deletions tests/ui/missing_assert_message.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,97 +4,128 @@ error: assert without any message
LL | assert!(foo());
| ^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic
= note: `-D clippy::missing-assert-message` implied by `-D warnings`

error: assert without any message
--> $DIR/missing_assert_message.rs:15:5
|
LL | assert_eq!(foo(), foo());
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:16:5
|
LL | assert_ne!(foo(), foo());
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:17:5
|
LL | debug_assert!(foo());
| ^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:18:5
|
LL | debug_assert_eq!(foo(), foo());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:19:5
|
LL | debug_assert_ne!(foo(), foo());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:24:5
|
LL | assert!(bar!(true));
| ^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:25:5
|
LL | assert!(bar!(true, false));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:26:5
|
LL | assert_eq!(bar!(true), foo());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:27:5
|
LL | assert_ne!(bar!(true, true), bar!(true));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:32:5
|
LL | assert!(foo(),);
| ^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:33:5
|
LL | assert_eq!(foo(), foo(),);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:34:5
|
LL | assert_ne!(foo(), foo(),);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:35:5
|
LL | debug_assert!(foo(),);
| ^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:36:5
|
LL | debug_assert_eq!(foo(), foo(),);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: assert without any message
--> $DIR/missing_assert_message.rs:37:5
|
LL | debug_assert_ne!(foo(), foo(),);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider describing why the failing assert is problematic

error: aborting due to 16 previous errors