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

Prefer assert_eq!(.., false) to assert!(!..) #7990

Closed
wants to merge 1 commit into from

Conversation

couchand
Copy link
Contributor

In numerous discussions over the years with developers at many different organizations, I've never heard a developer suggest that assert!(!condition); is better than assert_eq!(condition, false);. At best, developers are indifferent, but those who think deeply about such matters reliably prefer comparing against the explicit literal. It is easy to see why: the inversion of the condition is very likely to be missed when reading a list of assertions.

This suggests that the bool_assert_comparison lint is backwards, at least in the case of comparing against false. When comparing against true, there are cases where a plain assert!() is appropriate (such as assert!(maybe.is_none());), and there are times when comparing against a literal is appropriate (e.g. assert_eq!(result, true);), so there's no single right answer we should encourage.

This PR adds a new lint bool_assert_inverted that checks to be sure you are not writing code of the form assert!(!condition); where the inversion of the condition could be easily missed. It also moves the competing bool_assert_comparison to pedantic to disable it by default. I'm not familiar enough with the clippy dev process to know how to remove it entirely, but that seems to be the right move, since the two lints cannot be enabled at the same time.

changelog: Moves [bool_assert_comparison] to pedantic and introduces [bool_assert_inverted] to check for easily-missed logic reversal in assertions.

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 17, 2021
@couchand
Copy link
Contributor Author

Looks like the test failures are for debug_assert_with_mut_call, which uses a number of examples that run afoul of this lint such as debug_assert!(!bool_ref(&3));. Happy to update the test cases as appropriate; in keeping with the style expected of this lint, this case should be written debug_assert_eq!(bool_ref(&3), false);.

@llogiq
Copy link
Contributor

llogiq commented Nov 17, 2021

I for one find assert!(!condition) more readable than assert_eq!(false, condition).

@ghost
Copy link

ghost commented Nov 18, 2021

I want to caution against introducing a new warn-by-default lint that is contrary to a current one. It is going to infuriate some users - see #7387 . There should be a compelling case for the change and this one already looks controversial.

@flip1995
Copy link
Member

flip1995 commented Nov 18, 2021

So from a pure process perspective: If we should introduce this new lint, we will have two contradicting lints, so at least one of them has to go into restriction, if not both. This is to avoid the situation @mikerite described above.

Personally, I also prefer assert!(!cond).

Since you already mentioned in your first sentence of the PR, that people are indifferent about it, prefer the comparison to a literal, or, as you heard here from @llogiq and now me, prefer the shorter version, this is at best opinionated. So with your argument, we should not warn-by-default on both variants.

I'm open to the argument that this lint is opinionated and should be allow-by-default. But looking at #7666, the numbers there don't really support that argument.

So I would say, if this new lint gets added, it has to go into the restriction group, since it contradicts an existing warn-by-default lint.

For downgrading the existing lint, I'm not really convinced.

@couchand
Copy link
Contributor Author

After this conversation, I'm sympathetic to the idea that this new lint should not be warn-by-default, it's clearly more contentious than I realized. That also seems wise given that completely swapping the default would amount to lots of churn.

So with your argument, we should not warn-by-default on both variants.

I now think that's the right approach. I don't see either of them being a slam dunk after hearing from you all. Importantly it's not a public API question and neither form is in itself an indicator of a bug (though I would argue that there's a potential to misread the inverted condition).

But looking at #7666, the numbers there don't really support that argument.

That table is a great data point, and can be very informative. But there are a few reasons why that's not the end of the story:

  • published crates are a relatively small fraction of all Rust code,
  • there's no reason to expect published crates are a representative sample of Rust code, and
  • absence of evidence is not evidence of absence.

In particular, I would expect that published crates are significantly more likely to comply with the default clippy settings, given the Rust community's shared values. That is, when authoring a crate, a dev is more likely to clippy apply, and when authoring private code, they are more likely to #[allow].


(The below is an attempt to make the question less about individual style preference. Feel free to skip.)

I've seen this exact question come up when helping to write and edit style guides at two different organizations in the past. Those style guides are unfortunately not public resources. I tried finding public style guides that comment on this question, but it is hard to find. Much more common is a discussion of avoiding comparison to literals in if conditions, but clearly the context there is different from an assertion.

One interesting point is that many testing environments actually have a more specific semantic for this case. For instance, rubocop (Ruby) says that both of these forms are bad, and refute should be used instead (https://github.com/rubocop/minitest-style-guide#refute-false), and chai (JS) has assert.isOk/assert.isNotOk in the assertion-style, and expect(..).to.be.false in the expect-style. A possibly-relevant section of the documentation states "Add .not earlier in the chain to negate .false. However, it’s often best to assert that the target is equal to its expected value, rather than not equal to false." (https://www.chaijs.com/api/bdd/#false).

Perhaps the most directly-relevant public reference to the question is this accepted Stack Overflow answer which recommends asserting equality to false. But another (higher-voted, so yes it's contentious) answer cites PEP8's ban on literals in if conditions as a reason to avoid them in assertions (which I'd argue is not a perfect contextual match). Plus, that's in the context of Python, which has squishy equality, which throws everything off.

Rust is somewhat unique, in that the built-in test framework (particularly in combination with the expressive type system) is "good enough". It's not common to see custom assertion frameworks where a refute style operator would be available.

The other environment I've seen that has a similar context is Salesforce, where the built-in test framework is "good enough" and so custom assertions are not prevalent. There, as in Rust, the entire list of assertions is System.assert, System.assertEquals, and System.assertNotEquals. This is the context in which the previously-mentioned style guides were used, and at both organizations, we determined that System.assertEquals(false, condition) was preferable to System.assert(!condition). These conversations revolved around the potential to misread the latter. I would suggest that in Rust, where the assertions are macros and therefore already have exclamation points, the risk of missing the inversion is that much higher.

@giraffate giraffate added S-needs-discussion Status: Needs further discussion before merging or work can be started and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 24, 2021
@camsteffen
Copy link
Contributor

camsteffen commented Nov 28, 2021

I think this is just one instance of a wider aversion to the ! (not) operator felt by some. I understand the reasoning, but I'm personally convinced that the not operator is idiomatic Rust and more verbose forms to avoid the not operator are preferred by a minority. So I think the current warn-by-default behavior is okay.

One option is to split bool_assert_comparison into two lints, allowing people to write #![allow(assert_eq_false)]. I think this is a reasonable option, and we could also consider putting assert_eq_false in pedantic.

For the new lint added in the PR bool_assert_inverted - I would definitely make it restriction (and would maybe name it assert_not).

@couchand
Copy link
Contributor Author

couchand commented Nov 29, 2021

I think this is just one instance of a wider aversion to the ! (not) operator felt by some.

I guess I didn't explain clearly enough, because this is definitely not a general aversion to !, it's a specific concern about the readability of assertions. To wit, the fact that the condition in assert!(!condition); has been inverted does not jump out in a quick reading. There are a few factors at play:

  • there are two exclamation points in close succession
  • assertions are usually grouped together
  • assertions are different from conditionals: they state an expectation about the world, and so encourage the reader to "trust them" in a way

There are cases where assert! makes the most sense, for instance:

let mut state = InitialState;

state.transition(&input);

assert!(state.is_final());

There are cases where assert_eq! is preferable, for example:

let deserialized: MyStruct = serde_json::deserialize(&input);

assert_eq!("foobar", deserialized.name);
assert_eq!(42, deserialized.count);
assert_eq!(false, deserialized.is_active);

Both of these are assertions on boolean values, but they carry different semantics.

@camsteffen
Copy link
Contributor

  • there are two exclamation points in close succession

This is the point where I say yes, that is a real downside, but this feels like a matter of personal preference.

  • assertions are different from conditionals: they state an expectation about the world
assert_eq!(false, deserialized.is_active);

I see what you're getting at here and this is a good point. You are checking a value rather than a condition. And from that standpoint, assert_eq!(value, true) equally seems fine. Clippy has no way of knowing the difference. I would be amenable to re-categorizing the lint based on this point (and documenting a known problem). We could call this an "occasional false positive" and move the lint to pedantic. Or we could say this is too common of a false positive and make it restriction.

@giraffate
Copy link
Contributor

@couchand ping from triage. Should the discussion still continue? If you have any idea on this, feel free to comment here.

@couchand
Copy link
Contributor Author

@giraffate after the discussion above, I would agree with updating the new lint to restriction and the old lint to pedantic. I'm happy to update this PR

And from that standpoint, assert_eq!(value, true) equally seems fine.

I'd agree, while recognizing that this form doesn't have the same readability hazard.

@bors
Copy link
Collaborator

bors commented Jan 11, 2022

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

@flip1995
Copy link
Member

I would agree with updating the new lint to restriction and the old lint to pedantic. I'm happy to update this PR

I'm fine with that 👍

@giraffate
Copy link
Contributor

@couchand ping from triage. Can you have any update on this?

@couchand
Copy link
Contributor Author

@giraffate, I've rebased and updated per the most recent comments.

Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

The tests fails, so can tests/ui/bool_assert_inverted.stderr be updated?

///
/// ### Why is this bad?
/// It is all too easy to misread the semantics of an assertion when the
/// logic of the condition is reversed. Explicitly comparing to a boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

nits

Suggested change
/// logic of the condition is reversed. Explicitly comparing to a boolean
/// logic of the condition is reversed. Explicitly comparing to a boolean

macro_call.span,
&format!("used `{}!` with an inverted condition", macro_name),
"replace it with",
format!("{}!(.., false, ..)", eq_mac),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the third argument be removed if there is no custom panic message?

@giraffate giraffate added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-needs-discussion Status: Needs further discussion before merging or work can be started labels Feb 25, 2022
@bors
Copy link
Collaborator

bors commented Mar 4, 2022

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

@giraffate
Copy link
Contributor

@couchand ping from triage. Can you have any questions to work on this?

@giraffate
Copy link
Contributor

@couchand ping from triage. According the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

@giraffate giraffate closed this Mar 24, 2022
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants