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

Why does Rubocop equate != 0 with #nonzero? #3598

Closed
georgeu2000 opened this issue Oct 12, 2016 · 8 comments

Comments

Projects
None yet
5 participants
@georgeu2000
Copy link

commented Oct 12, 2016

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.

@georgeu2000 georgeu2000 changed the title Why does rubocop equate != 0 with #nonzero? Why does Rubocop equate != 0 with #nonzero? Oct 12, 2016

@Drenmi

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Author

commented Oct 12, 2016

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 comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2016

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. 😀

jonas054 added a commit to jonas054/rubocop that referenced this issue Nov 6, 2016

[Fix rubocop-hq#3598] Don't auto-correct between x.nonzero? and x != 0
The two expressions are not equivalent. `x != 0` returns `true`
or `false` while `x.nonzero?` returns `self`, i.e. `x`, or `nil`.
The user needs to be aware of these changes by making them manually.
That way a breaking change is easier to spot.

jonas054 added a commit to jonas054/rubocop that referenced this issue Nov 6, 2016

[Fix rubocop-hq#3598] Don't auto-correct between x.nonzero? and x != 0
The two expressions are not equivalent. `x != 0` returns `true`
or `false` while `x.nonzero?` returns `self`, i.e. `x`, or `nil`.
The user needs to be aware of these changes by making them manually.
That way a breaking change is easier to spot.

jonas054 added a commit to jonas054/rubocop that referenced this issue Nov 18, 2016

[Fix rubocop-hq#3598] Disregard x.nonzero? and x != 0 in NumericPredi…
…cate

The two expressions are not equivalent. `x != 0` returns `true`
or `false` while `x.nonzero?` returns `self`, i.e. `x`, or `nil`.

jonas054 added a commit to jonas054/rubocop that referenced this issue Nov 21, 2016

[Fix rubocop-hq#3598] Disregard x.nonzero? and x != 0 in NumericPredi…
…cate

The two expressions are not equivalent. `x != 0` returns `true`
or `false` while `x.nonzero?` returns `self`, i.e. `x`, or `nil`.

@bbatsov bbatsov closed this in bb9ac67 Nov 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.