Why does Rubocop equate != 0 with #nonzero?

[georgeu2000]

Rubocop warns that I should

Style/NumericPredicate: Use foo.nonzero? instead of foo != 0.

Expected behavior

I'm not sure why this is a warning when the functionality is different.

Actual behavior

1.nonzero? => 1
1 != 0 => true

Steps to reproduce the problem

1 != 0
rubocop

RuboCop version

$ rubocop -V
0.43.0

This is not an issue on 0.40

I tried to correct with !! foo.nonzero? but that raises another warning.

[Drenmi]

Hm. This looks like it is just a brain fart on my end. I have only ever used this method in predicate expressions, because I was under the impression that the ? suffix in Ruby indicates a boolean return value. (This particular method could be a legacy, as it has been in Ruby since 1.8.3.)

It seems a lot of people are wrestling with this cop for other reasons, I think this might be the straw that broke the camel's back, and I think we should remove it. (Or at least disable it.)

WDYT @bbatsov, @jonas054?

[bbatsov]

One of the many crazy things in the core library. Predicate methods are supposed to return booleans, but I guess the core team forgot this here and there. Amusingly the zero? method works as expected. I'll think about this.

[savef]

I like this cop but think it should probably have its autocorrect disabled by default because that's sometimes dangerous.

Regarding this specific issue, might it be worth limiting the scope of the cop to a whitelist of places where we know that only the truthiness is being checked? Such as if, elsif, unless, while, until, and in ternary operators. I'm thinking that this would still cover the majority of comparisons because they will be most likely to appear in flow control statements.

So these would still give an offence:

call unless number > 0
x = number > 0 ? :a : :b

But these no longer would:

x = number > 0
return number > 0
expect(number).to be > 0

[georgeu2000]

Regarding this specific issue, might it be worth limiting the scope of the cop to a whitelist of places where we know that only the truthiness is being checked? Such as if, elsif, unless, while, until, and in ternary operators.

FWIW, this seems like a great solution.

[bbatsov]

This cop actually checks for 4 predicates, not just one. All of them except nonzero? are equivalent (more or less) with using comparison ops. Keep this in mind.

One other option would be to suggest replacing x != 0 checks with !x.zero?. I'd also add a cop warning about the use of nonzero?. :-)

[savef]

One other option would be to suggest replacing x != 0 checks with !x.zero?. I'd also add a cop warning about the use of nonzero?. :-)

I prefer this to my idea. 👍

I was thinking my proposal would offer some extra safety and ease some of the issues people had with the cop in #3371, but on second thought it wouldn't, any benefits would be coincidental.

[jonas054]

Having read through https://bugs.ruby-lang.org/issues/9123 I've come to the conclusion that I want to side with the people who say that there's nothing wrong with Numeric#nonzero? returning self or nil. It might be unexpected for some people, but those people have unrealistic expectations.

So I don't see a need for a new cop to report usages of nonzero?. On the other hand, there is a risk of Style/NumericPredicate auto-correct breaking code that relies on x != 0 being true or false, and such a breakage could be hard to sort out, when the changed behavior is noticed.

I suggest that we just avoid auto-correcting x != 0 (or x.nonzero? depending on configuration), and later improve this special handling by only avoiding auto-correct when the returned value might be used for something other than as a condition. This is based on the idea that @savef put forward and then retracted, but limited to x != 0 vs x.nonzero?, and limited to auto-correct (i.e. still reporting).

[Drenmi]

I've come to the conclusion that I want to side with the people who say that there's nothing wrong with Numeric#nonzero? returning self or nil. It might be unexpected for some people, but those people have unrealistic expectations.

That's interesting. I am on the other side. 😊 However, I think the real mistake was tacking that ? onto #nonzero. If it was just #nonzero, everyone would be happy, and it's in line with methods like ActiveSupport's #presence.

I don't think it affects RuboCop that much though. I still side with your proposal on how to handle it. 😀