Add lint to check for boolean comparison in assert macro calls #7083

GuillaumeGomez · 2021-04-15T13:43:42Z

This PR adds a lint to check if an assert macro is using a boolean as "comparison value". For example:

assert_eq!("a".is_empty(), false);

Could be rewritten as:

assert!(!"a".is_empty());

PS: The dev guidelines are amazing. Thanks a lot for writing them!

changelog: Add bool_assert_comparison lint

rust-highfive · 2021-04-15T13:43:45Z

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

camsteffen · 2021-04-15T14:13:05Z

I'm pretty sure we are moving away from pre-expansion passes. Sorry that that means you need to re-work this to inspect expanded code. We have a util function extract_assert_macro_args to do this with the HIR. But it would be nice to implement this in an early pass (AST).

Tagging @rust-lang/clippy for input on this.

GuillaumeGomez · 2021-04-15T14:56:11Z

Makes sense. I'll rewrite it then.

GuillaumeGomez · 2021-04-15T21:41:10Z

Rewrite done. I'm very unhappy with the result though: if any of the macro gets updated, the lint will break. Also, the suggestion now is much worse since I can't simply show the macro arguments... Anyway, at least now it's fully running on early pass and doesn't require macro expansion anymore!

Manishearth · 2021-04-15T23:05:22Z

Yeah, that's fine, we have a fair number of lints that do that kind of thing, which is why we have all these desugaring functions in utils. We catch breakages when they happen and update the code; this isn't a huge deal since clippy is shipped with rustc

GuillaumeGomez · 2021-04-16T08:59:29Z

CI finally passed! \o/

camsteffen

Not sure what the file /bool_assert_eq is. Probably need to remove.

clippy_lints/src/bool_assert_comparison.rs

camsteffen · 2021-04-16T16:53:49Z

clippy_lints/src/bool_assert_comparison.rs

+        for mac in macros.iter().chain(inverted_macros.iter()) {
+            if is_direct_expn_of(e.span, mac).is_some() {


You can use the Span from on is_direct_expn_of to lint the entire macro invocation, not just the bool arg. This makes more sense because you are suggesting to use a different macro.

Right, now that I don't show the full result anymore, it doesn't make much sense anymore...

What do you mean by "the full result"?

My suggestion doesn't show anymore what the assert! call should look like since that the snippet span returns the macro "definition" and not the arguments anymore.

When you use snippet, you have to use spans that only include the arg passed to the macro. For example, assert_eq!(a, true) will expand to match (&a, &true) { .. }. From that, you need to get the span of just a.

It's still not good in case you have more than two arguments. Getting the arguments starting from the third is a nightmare... It was easy to do in the pre_expansion pass, but now it's just trying to go through the macro source code from a token tree.

clippy_lints/src/bool_assert_comparison.rs

clippy_utils/src/ast_utils.rs

clippy_lints/src/bool_assert_comparison.rs

clippy_utils/src/ast_utils.rs

camsteffen · 2021-04-17T15:11:40Z

clippy_lints/src/bool_assert_comparison.rs

+        for mac in macros.iter().chain(inverted_macros.iter()) {
+            if is_direct_expn_of(e.span, mac).is_some() {


When you use snippet, you have to use spans that only include the arg passed to the macro. For example, assert_eq!(a, true) will expand to match (&a, &true) { .. }. From that, you need to get the span of just a.

clippy_lints/src/bool_assert_comparison.rs

tests/ui/bool_assert_comparison.stderr

clippy_lints/src/bool_assert_comparison.rs

clippy_utils/src/ast_utils.rs

GuillaumeGomez · 2021-04-18T14:23:58Z

Updated!

camsteffen

A couple minor changes and then LGTM! Please squash your commits.

clippy_utils/src/ast_utils.rs

clippy_lints/src/bool_assert_comparison.rs

GuillaumeGomez · 2021-04-19T11:46:31Z

A couple minor changes and then LGTM! Please squash your commits.

I'd rather keep the first commit if you don't mind. It was more complete (in its suggestion) and it can be used for potential future migrations if anyone needs to do the same (I might haha).

clippy_utils/src/ast_utils.rs

camsteffen · 2021-04-19T15:17:32Z

I'd rather keep the first commit if you don't mind.

No, I don't see how it would be useful. Pre-expansion passes are going away. Migrating to an early pass will always require re-thinking the implementation. I added a comment to hopefully stop people short of wring pre-exapnsion passes in the future.

GuillaumeGomez · 2021-04-19T15:18:52Z

Makes me sad but ok. Just waiting for our last discussion to be resolved and then I'll squash.

GuillaumeGomez · 2021-04-19T15:35:23Z

Removed the support of assert! in the function and squashed the commits.

camsteffen · 2021-04-19T16:01:58Z

Sorry just thought of one more case that I think is missing: assert_eq!(a!(), expr). One arg is a macro. Should not lint.

GuillaumeGomez · 2021-04-19T16:14:21Z

a()! isn't evaluated as true or false even if that's the only thing in it. I'll add a test case though. ;)

GuillaumeGomez · 2021-04-19T16:17:27Z

Ah nevermind! It actually throws an error. Interesting...

clippy_lints/src/bool_assert_comparison.rs

GuillaumeGomez · 2021-04-19T20:16:05Z

Updated!

GuillaumeGomez · 2021-04-21T12:32:48Z

Is there anything else for me to do in here?

camsteffen · 2021-04-21T13:22:09Z

Nope! Thanks!

@bors r+

bors · 2021-04-21T13:22:10Z

📌 Commit e2e104b has been approved by camsteffen

bors · 2021-04-21T13:22:18Z

⌛ Testing commit e2e104b with merge 37d2d26...

Add lint to check for boolean comparison in assert macro calls This PR adds a lint to check if an assert macro is using a boolean as "comparison value". For example: ```rust assert_eq!("a".is_empty(), false); ``` Could be rewritten as: ```rust assert!(!"a".is_empty()); ``` PS: The dev guidelines are amazing. Thanks a lot for writing them!

bors · 2021-04-21T13:22:56Z

💔 Test failed - checks-action_test

GuillaumeGomez · 2021-04-21T13:25:46Z

There is a failure because there is a missing "changelog" in the PR body but can't see anything about it in https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md. If someone has a link or a tip on what I need to update? :)

camsteffen · 2021-04-21T13:58:49Z

@bors retry

added a changelog line

bors · 2021-04-21T13:58:56Z

⌛ Testing commit e2e104b with merge bbc22e2...

GuillaumeGomez · 2021-04-21T13:59:44Z

added a changelog line

Thank you!

bors · 2021-04-21T14:12:30Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing bbc22e2 to master...

xFrednet · 2021-08-25T10:50:22Z

Hey, was there a reason why the lint suggestion doesn't snip the non literal expression into the suggested code change? 🙃

GuillaumeGomez · 2021-08-25T11:41:30Z

If you have an improvement in mind, don't hesitate to send it or at least open an issue with your suggestion.

rust-highfive assigned camsteffen Apr 15, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 15, 2021

GuillaumeGomez force-pushed the bool-assert-eq branch 2 times, most recently from 3375908 to 3f2c607 Compare April 15, 2021 14:00

GuillaumeGomez force-pushed the bool-assert-eq branch 3 times, most recently from 6b5312b to 1a43e15 Compare April 16, 2021 08:47

camsteffen suggested changes Apr 16, 2021

View reviewed changes

GuillaumeGomez force-pushed the bool-assert-eq branch 2 times, most recently from 66f5ecb to 7403737 Compare April 16, 2021 19:45

GuillaumeGomez commented Apr 16, 2021

View reviewed changes

clippy_lints/src/bool_assert_comparison.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the bool-assert-eq branch from 7403737 to cbb0039 Compare April 17, 2021 13:00

camsteffen reviewed Apr 17, 2021

View reviewed changes

GuillaumeGomez force-pushed the bool-assert-eq branch 3 times, most recently from 4f5e5bc to 6c8e925 Compare April 18, 2021 13:51

camsteffen suggested changes Apr 18, 2021

View reviewed changes

clippy_utils/src/ast_utils.rs Outdated Show resolved Hide resolved

clippy_lints/src/bool_assert_comparison.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the bool-assert-eq branch from 6c8e925 to c050a4d Compare April 19, 2021 11:45

GuillaumeGomez force-pushed the bool-assert-eq branch 2 times, most recently from aefee1a to 5a54c3f Compare April 19, 2021 12:21

camsteffen reviewed Apr 19, 2021

View reviewed changes

clippy_utils/src/ast_utils.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the bool-assert-eq branch from 5a54c3f to eb1d71f Compare April 19, 2021 15:34

GuillaumeGomez force-pushed the bool-assert-eq branch from eb1d71f to 1dd3210 Compare April 19, 2021 16:42

camsteffen reviewed Apr 19, 2021

View reviewed changes

clippy_lints/src/bool_assert_comparison.rs Outdated Show resolved Hide resolved

Add lint to check for boolean comparison in assert macro calls

e2e104b

GuillaumeGomez force-pushed the bool-assert-eq branch from 1dd3210 to e2e104b Compare April 19, 2021 20:15

bors merged commit bbc22e2 into rust-lang:master Apr 21, 2021

GuillaumeGomez deleted the bool-assert-eq branch April 21, 2021 14:15

		for mac in macros.iter().chain(inverted_macros.iter()) {
		if is_direct_expn_of(e.span, mac).is_some() {

Add lint to check for boolean comparison in assert macro calls #7083

Add lint to check for boolean comparison in assert macro calls #7083

Conversation

GuillaumeGomez commented Apr 15, 2021 • edited by camsteffen Loading

rust-highfive commented Apr 15, 2021

camsteffen commented Apr 15, 2021

GuillaumeGomez commented Apr 15, 2021

GuillaumeGomez commented Apr 15, 2021

Manishearth commented Apr 15, 2021

GuillaumeGomez commented Apr 16, 2021

camsteffen left a comment

Choose a reason for hiding this comment

camsteffen Apr 16, 2021

Choose a reason for hiding this comment

GuillaumeGomez Apr 16, 2021

Choose a reason for hiding this comment

camsteffen Apr 16, 2021

Choose a reason for hiding this comment

GuillaumeGomez Apr 17, 2021

Choose a reason for hiding this comment

camsteffen Apr 17, 2021

Choose a reason for hiding this comment

GuillaumeGomez Apr 18, 2021

Choose a reason for hiding this comment

camsteffen Apr 17, 2021

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 18, 2021

camsteffen left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 19, 2021

camsteffen commented Apr 19, 2021

GuillaumeGomez commented Apr 19, 2021

GuillaumeGomez commented Apr 19, 2021

camsteffen commented Apr 19, 2021

GuillaumeGomez commented Apr 19, 2021

GuillaumeGomez commented Apr 19, 2021

GuillaumeGomez commented Apr 19, 2021

GuillaumeGomez commented Apr 21, 2021

camsteffen commented Apr 21, 2021

bors commented Apr 21, 2021

bors commented Apr 21, 2021

bors commented Apr 21, 2021

GuillaumeGomez commented Apr 21, 2021

camsteffen commented Apr 21, 2021

bors commented Apr 21, 2021

GuillaumeGomez commented Apr 21, 2021

bors commented Apr 21, 2021

xFrednet commented Aug 25, 2021

GuillaumeGomez commented Aug 25, 2021

GuillaumeGomez commented Apr 15, 2021 •

edited by camsteffen

Loading