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

Replace assert!(a==b) with assert_eq!(a,b) as part of bool_assert_comparison lint #13333

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Sep 2, 2024

Expand lint bool_assert_comparison to catch assert!(a == b) and assert!(a != b) (or their debug versions), and suggest to replace them with the corresponding assert_eq!(a, b) or assert_ne!(a, b).

TODO:

  • Decide if this lint should expand bool_assert_comparison or be a new lint (name?). Note that existing lint description fits well to the new functionality.
  • Are we ok to keep this as style? Note that IntelliJ has a similar lint built-in, and it shows up similar to styling guide.

Closes #13252

changelog: [bool_assert_comparison]: now also converts assert!(a==b) into assert_eq!(a,b) and similar

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2024

r? @xFrednet

rustbot has assigned @xFrednet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 2, 2024
@workingjubilee
Copy link
Member

does this lint on assert!(x == y) in const contexts, where they are not replaceable with assert_eq! in today's Rust?

@@ -1,4 +1,4 @@
#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op)]
#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op, clippy::bool_assert_comparison)]
Copy link
Member

Choose a reason for hiding this comment

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

oh, it does!

given the sheer amount of pushback you gave me when you had to reorganize your code for linting reasons, I find it frankly astonishing that you would introduce a lint that forces every user of const fn to do the exact same thing you were complaining about having to do: rearchitect their code purely for linting reasons if they want this lint to remain useful for their code that doesn't use const assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, worry not, this lint needs feedback. So i guess you point is that i should use is_in_const_context check in this lint so that it doesn't get triggered for const context.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that seems like it would be sufficient, I think?

@workingjubilee
Copy link
Member

rustc has reliably gotten the user feedback of "these two things should not be the same lint" for basically every single lint expansion into a new category of code patterns. the fact that this new lint on some instances of assert! would have to be allowed every time someone wrote a const fn makes treating this as the same thing as assert_eq!(expr, false) especially baffling.

@nyurik
Copy link
Contributor Author

nyurik commented Sep 2, 2024

any suggestions for the new lint name? And yes, if this (or any other) lint shows when its not applicable, its a false positive and should be fixed

@workingjubilee
Copy link
Member

why not just assert_on_eq?

@nyurik
Copy link
Contributor Author

nyurik commented Sep 2, 2024

Perhaps. Usecase could be allow assert_on_eq to mean that assert!(foo==bar) is ok...

@smoelius
Copy link
Contributor

smoelius commented Sep 2, 2024

While I very much like this idea, Clippy had a lint for this before. See #2156, especially the issues/PRs referenced at the bottom.

@nyurik
Copy link
Contributor Author

nyurik commented Sep 4, 2024

Reading the other discussion, it seems there was hope that assert!(foo==bar) would somehow document the values of foo and bar, and later these efforts did not get too far (if i am reading it correctly). If so, we should merge related PRs into one lint and proceed with assert! -> assert_eq! suggestions. If on the other hand there is a way to improve assert! machinery in some foreseeable future, let's postpone this work.

@xFrednet
Copy link
Member

xFrednet commented Sep 9, 2024

Decide if this lint should expand bool_assert_comparison or be a new lint (name?). Note that existing lint description fits well to the new functionality.

That's a good question. I've created https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/New.20lint.20or.20expand.20.60bool_assert_comparison.60.20rust-clippy.231333/near/468909875

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Excellent start, I left comments, mostly related to your TODO comments :D

Comment on lines +170 to +175
// TODO: is `cond.span.from_expansion()` correct / needed?
if (node != BinOpKind::Eq && node != BinOpKind::Ne) || cond.span.from_expansion() {
Copy link
Member

Choose a reason for hiding this comment

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

// TODO: is cond.span.from_expansion() correct / needed?

It should be correct 👍


let new_name = format!("{macro_name}_{}", if node == BinOpKind::Eq { "eq" } else { "ne" });
let msg = format!("replace it with `{new_name}!(..)`");
span_lint_and_then(
Copy link
Member

Choose a reason for hiding this comment

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

is_in_const_context should be checked some time before here, and it would be good to have tests for it.

|diag| {
let macro_name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');
// TODO: should this be `cond.span`, expanded to include preceding whitespace? If so, how?
let equality_span = Span::new(lhs.span.hi(), rhs.span.lo(), span.ctxt(), span.parent());
Copy link
Member

Choose a reason for hiding this comment

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

We know that the == actually comes from the source code, so I would probably use cond.span.with_lo(lhs.span.hi()).with_high(rhs.span.lo()). But this should result in the same span, so either way should be fine :)

Expand lint `bool_assert_comparison` to catch `assert!(a == b)` and `assert!(a != b)` (or their debug versions), and suggest to replace them with the corresponding `assert_eq!(a, b)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lints to encourage use of specialized assert macros
5 participants