Add `missing_assert_message` lint #10362

unexge · 2023-02-16T17:04:54Z

changelog: new lint: [missing_assert_message]: A new lint for checking assertions that doesn't have a custom panic message.
#10362

r? @llogiq

rustbot · 2023-02-16T17:04:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

llogiq

Great work! I think the macro stuff is really tricky.

I have but a few small doc suggestions and I'll have to look up if we need an msrv.

clippy_lints/src/missing_assert_message.rs

matthiaskrgr · 2023-02-16T17:47:59Z

clippy_utils/src/numeric_literal.rs

-    debug_assert!(lit_kind.is_numeric());
+    debug_assert!(lit_kind.is_numeric(), "`lit_kind` should be numeric");
    lit_suffix_length(lit_kind)


My two cents: these changes display the very problem with this lint.
It encourages people to just add some kind of assert-message to silent the lint, but the message provides zero information.

If foo.is_numeric() returns false and blows up an assert, I already know that foo is not numeric and that this is problematic.
Why would would I want to waste time and add the same exact information "foo should be numeric" again, in a textual form (which just adds additional maintenance burden because now I have to additionally update the message every time I change the variable name, etc..) ?

IMO it would be good to add an additional message to the lint, something along the lines of
help: try to describe why the failing assert is problematic. The inserted assert-message should be more about the why is it important to know that this failed and not what exactly failed here, because the code inside the assert already conveys that.

I agree that the messages provided here are a bit meh. However, I would not say that this is a problem with the lint; we simply have no way of knowing whether an assert message will satisfy. However, having them at all will at least put them under the eyes of a potential reviewer. Also I think your point should be added to the lint's documentation.

Perhaps tne lint should be put in restriction instead of pedantic.

Yes you're right about the messages, since we are not very familiar with the codebase we tried to come up with messages by quickly looking at the assertion but it was hard to come up with good messages.
I've added a help message and updated the lint's description.
Didn't know about restriction, it makes more sense for this lint. I've reverted the "dogfood" commit, I believe someone with more familiarity with the codebase can enable the lint and come up with good assert messages in a follow-up PR.

llogiq · 2023-02-16T18:03:24Z

I just looked and assert!s allowed messages back in 1.0.0, so no MSRV necessary.

Alexendoo · 2023-02-17T15:58:27Z

clippy_lints/src/lib.rs

@@ -911,6 +912,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
    store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
    store.register_late_pass(|_| Box::new(multiple_unsafe_ops_per_block::MultipleUnsafeOpsPerBlock));
    store.register_late_pass(|_| Box::new(extra_unused_type_parameters::ExtraUnusedTypeParameters));
+    store.register_pre_expansion_pass(|| Box::<missing_assert_message::MissingAssertMessage>::default());


Unfortunately we don't want to be adding more pre expansion passes because it comes with a good number of bugs - #6610, it would want to be a late pass

We do have some utilities that would help out there: find_assert_args and find_assert_eq_args. Usage would be similar to

rust-clippy/clippy_lints/src/assertions_on_constants.rs

Lines 34 to 40 in 6444621

let Some(macro_call) = root_macro_call_first_node(cx, e) else { return };

let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) {

Some(sym::debug_assert_macro) => true,

Some(sym::assert_macro) => false,

_ => return,

};

let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) else { return };

We started with a late pass and find_assert_args functions, but I think there was a recent change in the compiler and assert macros expand to something else than find_assert_args expects. So those functions don't return the correct PanicExpn, we tried to fix it but couldn't do it :) I'll give it another go!

So it seems like starting from Rust 1.52 assert_{eq,ne} macros use panicking::assert_failed instead of panic! (See assert_eq in 1.52 and in 1.51).

a2502da addresses Rust 1.52 changes
569fa96 converts this lint to a late lint pass

bors · 2023-02-23T16:11:17Z

☔ The latest upstream changes (presumably #10392) made this pull request unmergeable. Please resolve the merge conflicts.

unexge · 2023-03-02T20:08:42Z

/ping 😄 I believe all the comments are addressed and it's ready for another round of review

llogiq · 2023-03-03T07:32:59Z

@Alexendoo since you commented on the implementation, do you have an idea on how to get this to work as a LateLintPass?

Alexendoo · 2023-03-03T14:31:33Z

It's working as a late pass from the latest commit, it looks good to me at a glance

llogiq

This looks good to me, I only have a last small doc suggestion.

llogiq · 2023-03-03T22:49:42Z

clippy_lints/src/missing_assert_message.rs

+    /// 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.
+    ///


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?

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?

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

bors · 2023-03-07T13:54:37Z

☔ The latest upstream changes (presumably #10415) made this pull request unmergeable. Please resolve the merge conflicts.

Co-authored-by: Weihang Lo <[email protected]>

Co-authored-by: llogiq <[email protected]>

Co-authored-by: Weihang Lo <[email protected]>

This reverts commit ec65357.

Co-authored-by: Weihang Lo <[email protected]>

llogiq · 2023-03-08T09:07:09Z

Thank you!

@bors r+

bors · 2023-03-08T09:07:10Z

📌 Commit b554ff4 has been approved by llogiq

It is now in the queue for this repository.

bors · 2023-03-08T09:07:18Z

⌛ Testing commit b554ff4 with merge 216aefb...

bors · 2023-03-08T09:19:31Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 216aefb to master...

mickvangelderen · 2023-09-22T16:36:36Z

Would it be possible to not trigger this lint in testing code? In tests the assert expression usually suffices but outside of tests the additional message is much more likely to be exposed to a non-developer.

This is done for other lints such as unwrap_used.

unexge · 2023-09-22T22:06:05Z

Hey @mickvangelderen, that should already be the case. There might be some cases we fail to identify as a test function though, could you please share the code if possible?

mickvangelderen · 2023-09-23T11:33:27Z

@unexge ah, I only read the lint documentation, not the code. Perhaps it should be added because I misjudged the false positive rate of this lint based on the current description.

unexge · 2023-09-27T20:28:40Z

@mickvangelderen Yes, that's definitely a good point! I'll submit a PR. Thanks!

Mention that `missing_assert_message` lint ignores test functions Updates `missing_assert_message`'s docs to reflect that it ignores test functions as pointed out by `@mickvangelderen` in #10362 (comment) --- changelog: [`missing_assert_message`]: Update docs to reflect this lint ignores test functions

rustbot assigned llogiq Feb 16, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2023

llogiq reviewed Feb 16, 2023

View reviewed changes

matthiaskrgr reviewed Feb 16, 2023

View reviewed changes

Alexendoo reviewed Feb 17, 2023

View reviewed changes

unexge force-pushed the missing-assert-message-lint branch from cc616a4 to 569fa96 Compare February 24, 2023 00:30

llogiq reviewed Mar 3, 2023

View reviewed changes

unexge and others added 10 commits March 8, 2023 08:50

Add missing_assert_message lint

ea2547b

Co-authored-by: Weihang Lo <[email protected]>

Dogfood missing_assert_message on Clippy

8f3ac65

Co-authored-by: Weihang Lo <[email protected]>

Apply suggestions from code review

099d610

Co-authored-by: llogiq <[email protected]>

Update lint description and add help section

4eb6ccc

Co-authored-by: Weihang Lo <[email protected]>

Move lint to restriction category

6fac73b

Co-authored-by: Weihang Lo <[email protected]>

Revert "Dogfood missing_assert_message on Clippy"

b4b2b12

This reverts commit ec65357.

Revert tests/ui/filter_map_next_fixable.rs

e7065ef

Co-authored-by: Weihang Lo <[email protected]>

Update assertion macro parsing logic for Rust 1.52 changes

682d52c

Co-authored-by: Weihang Lo <[email protected]>

Use late lint pass for missing_assert_message lint

87f58a1

Co-authored-by: Weihang Lo <[email protected]>

Add Known problems section

b554ff4

Co-authored-by: Weihang Lo <[email protected]>

unexge force-pushed the missing-assert-message-lint branch from 9c5ec69 to b554ff4 Compare March 8, 2023 08:52

bors merged commit 216aefb into rust-lang:master Mar 8, 2023

unexge deleted the missing-assert-message-lint branch March 8, 2023 09:22

bors mentioned this pull request Mar 8, 2023

Added the [unnecessary_box_returns] lint #9102

Merged

unexge mentioned this pull request Sep 27, 2023

Mention that missing_assert_message lint ignores test functions #11574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `missing_assert_message` lint #10362

Add `missing_assert_message` lint #10362

unexge commented Feb 16, 2023 •

edited by xFrednet

Loading

rustbot commented Feb 16, 2023

llogiq left a comment

matthiaskrgr Feb 16, 2023

llogiq Feb 16, 2023

unexge Feb 18, 2023

llogiq commented Feb 16, 2023

Alexendoo Feb 17, 2023

unexge Feb 18, 2023

unexge Feb 24, 2023

bors commented Feb 23, 2023

unexge commented Mar 2, 2023

llogiq commented Mar 3, 2023

Alexendoo commented Mar 3, 2023

llogiq left a comment

llogiq Mar 3, 2023

unexge Mar 5, 2023

llogiq Mar 7, 2023

bors commented Mar 7, 2023

llogiq commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

mickvangelderen commented Sep 22, 2023 •

edited

Loading

unexge commented Sep 22, 2023

mickvangelderen commented Sep 23, 2023

unexge commented Sep 27, 2023

	let Some(macro_call) = root_macro_call_first_node(cx, e) else { return };
	let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) {
	Some(sym::debug_assert_macro) => true,
	Some(sym::assert_macro) => false,
	_ => return,
	};
	let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) else { return };

Add missing_assert_message lint #10362

Add missing_assert_message lint #10362

Conversation

unexge commented Feb 16, 2023 • edited by xFrednet Loading

rustbot commented Feb 16, 2023

llogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 23, 2023

unexge commented Mar 2, 2023

llogiq commented Mar 3, 2023

Alexendoo commented Mar 3, 2023

llogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 7, 2023

llogiq commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

mickvangelderen commented Sep 22, 2023 • edited Loading

unexge commented Sep 22, 2023

mickvangelderen commented Sep 23, 2023

unexge commented Sep 27, 2023

Add `missing_assert_message` lint #10362

Add `missing_assert_message` lint #10362

unexge commented Feb 16, 2023 •

edited by xFrednet

Loading

mickvangelderen commented Sep 22, 2023 •

edited

Loading