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

Consider disabling eq_op in test code #7080

Closed
rail-rain opened this issue Apr 14, 2021 · 5 comments
Closed

Consider disabling eq_op in test code #7080

rail-rain opened this issue Apr 14, 2021 · 5 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@rail-rain
Copy link
Contributor

The eq_op lint has a quite large number of allows, ranking in "The most commonly ignored lints" (#5418), and it seems the number has largely increased because of the inclusion of the assert! family (#6167). I've looked at some of the details and found the overwhelming majority was in test code to ensure they implemented functions and traits like PartialEq correctly; here and here for example.

I'm not sure if I consider this case as a false positive, but legit supression at this scale doesn't seem right although no one except this comment reported this at the moment. However, disabling in test code altogether makes a new FN in case of unintensional eq_ops. If Clippy don't do this, it means the intension of this lint is silence it in these tests at the smallest scale possible so it can catch other mistakes in test code.

I'd like to ask other people if trading off the FN for eliminating this FP (sort of) is worth or not though I'm in favour of disabling eq_op in test code.

As a bonus, this can close #6635 nicely.

The implementation will probably be detecting #[test] attriubtes so the minimal repro (?) would be:

#[test]
fn equality() {
  let a = 42;
  assert_eq!(a, a);
}

I expected to see this happen: no errors

Instead, this happened (with --all-targets):

error: identical args used in this `assert_eq!` macro call
 --> src/lib.rs:6:20
  |
6 |         assert_eq!(a, a);
  |                    ^^^^
  |
  = note: `#[deny(clippy::eq_op)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#eq_op

Meta

  • cargo clippy -V: clippy 0.1.51 (2fd73fa 2021-03-23)
  • rustc -Vv:
    rustc 1.51.0 (2fd73fabe 2021-03-23)
    binary: rustc
    commit-hash: 2fd73fabe469357a12c2c974c140f67e7cdd76d0
    commit-date: 2021-03-23
    host: x86_64-unknown-linux-gnu
    release: 1.51.0
    LLVM version: 11.0.1
    
@rail-rain rail-rain added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Apr 14, 2021
@Jarcho
Copy link
Contributor

Jarcho commented Apr 15, 2021

I agree the example is obviously a test for PartialEq. This couldn't be extended in general though. It would still need to catch something like

#[test]
fn test() {
    let x = some_fn();
    let y = x.some_transformation();
    assert_eq!(x.field1, y.field1);
    assert_eq!(x.field2, x.field2); // oops
}

It could be restricted to tests which only have a single variable.

@rail-rain
Copy link
Contributor Author

Yes, that is what I tried to say in the second paragraph of the original post, which was too cumbersome. My concern is that this behavior might annoy people too much so they start to disable the lint at a large scale like the example from tikv; there, the lint cannot catch other mistakes that could happen around here. However, it is true that there are more tests where the lint is relevant than the tests where it is a FP.

I'm not sure if checking the number of variables in tests is worth to implement because some people including me tend to put multiple tests in one function like both tikv and indexmap, so it is quite rare that a test function has only one variable.

@Jarcho
Copy link
Contributor

Jarcho commented Apr 21, 2021

I would suggest putting all the self comparison tests into their own test function and put an allow on just that test. Only helps if people actually do that though.

I think it's better to keep checking on test code. It's only needed for non-derived PartialEq and PartialOrd impls, which is unusual enough as is. Disabling it at the function level is also easy enough and leaves you no worse off than if you it didn't check test code in the first place.

With the example from tikv I had to make a quick double take to make sure it was on purpose. It has the check with the other variable already there, so it's easy enough to figure out what's happening, but it would have been clearer with the allow added to each statement (or a comment to the group of them). A little wordier, yes, but I wouldn't have needed to read the surrounding code again.

@camsteffen
Copy link
Contributor

camsteffen commented May 3, 2021

How bout this?

macro_rules! assert_eq_self {
	($e:expr) => {
		#[allow(clippy::eq_op)]
		assert_eq!($e, $e);
	};
}

@rail-rain
Copy link
Contributor Author

My apologies for a late reply. There were good arguments and interesting suggestions here. However, to my surprise, my original suggestion is implemented (#7811) independently from this issue. I think the current state is good enough, so I'm closing this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

No branches or pull requests

3 participants