Rubocop: Style/NumericPredicate unsafe auto-correct

[madwort]

Expected behavior

This is a trivial example, but we have some code with an effect like this:

my_var_may_be_nil =
  if something_is_true
    1
  else
    nil
  end

my_var_may_be_nil == 0

In the event that my_var_may_be_nil is nil, the result of the last statement is false.

Actual behavior

This cop auto-corrects our code to:

my_var_may_be_nil =
  if something_is_true
    1
  else
    nil
  end

my_var_may_be_nil.zero?

In the event that my_var_may_be_nil is nil, the result of the last statement is failure:

NoMethodError: undefined method `zero?' for nil:NilClass
    from (irb):1
    from /opt/rbenv/versions/2.2.2/bin/irb:11:in `<main>'

IMHO default configuration auto-correct would preferably not introduce semantic changes like this.

RuboCop version

$ rubocop -V
0.42.0 (using Parser 2.3.1.2, running on ruby 2.2.2 x86_64-linux)

[Drenmi]

This kind of code is the exact rationale for using the predicate methods. 😅 Returning an integer _or_ nil completely undermines polymorphism, and in worst case, you're unknowingly comparing to nil.

That said, I agree that auto-correct should be conservative, and am open to making it disabled by default.

Thoughts @bbatsov, @jonas054?

[madwort]

Yes, I agree .zero? should be preferred & this example looks very bad, the actual code is a bit different but I was just looking to most obviously demonstrate the point.

In general, we have inherited a large codebase with many issues, rubocop is helping us greatly clear up some of them, in this instance simply failing a rubocop requiring manual intervention / refactoring is much more useful to us than an autocorrect that passes the rubocop but introduces a subtle bug. This problem was caught straight away by our rspecs, but our rspecs don't cover everything...

[ptarjan]

+1 to not autocorrenting

I'd also like to pile on that this cop is dangerous and should not be on by default. Just because I'm comparing a variable to 0 does NOT imply that my object has a .zero? method.

[Drenmi]

@ptarjan: That is the rationale for using #zero? in the first place. If our variable doesn't respond to #zero?, we're doing something dangerous, either intentionally or by mistake. If we're relying on this behaviour, we have an implicit type check in our code. I.e.:

if foo.is_a?(Numeric)
  foo.zero?
else
  false
end

Having variables or return values that evaluate to a primitive type or nil, is always a bad idea, and it's making polymorphism nearly impossible.

[madwort]

Happy to have this cop enabled by default, as I want to be improving our code to do a nil test & then a .zero? test, but would prefer to see autocorrect disabled by default.

[ptarjan]

@Drenmi I respectfully disagree. When not doing OO programming, having a nullable type is perfectly reasonable. Heck, even when doing OO programming many languages allow nullable types by default (Java is the obvious one).

In the RDL type system it would be the difference between the type Integer and the type !Integer. Both are reasonable annotations. https://github.com/plum-umd/rdl#non-null-type. Calling == on Integer is typesafe. Calling .zero? on a Integer isn't typesafe since it might be null. Calling it on !Integer is perfectly reasonable.

Similarly union types are also reasonable, like Foo or Integer which again are unsound to call .zero? on. https://github.com/plum-umd/rdl#union-types

Without type annotations I argue that you have to assume anything that you are comparing with == to an integer is not guaranteed to be a !Integer, so this rule is unsound for the type system.

In practice when I applied this rule to our multi-milliion line codebase it broke tons of things. I spot-checked and each instance was completely reasonable code (mostly nullable but some union types of Object or Integer). So we'll be disabling it internally and I would recommend you do the same for open source projects as it sadly is unsound. I'm perfectly happy having unsound rules in rubocop, just having them on by default I would say is bad.

[Drenmi]

@ptarjan: I tend to be dogmatic about extremely few things, but unfortunately NULL is one of them. It doesn't model anything meaningful in any domain, and thus I can't go on to call it reasonable. Empirically, rationally, and otherwise, it has proven itself nothing but a major source of bugs.

If we reduce the argument to "we shouldn't use #zero?, because we might be sending it to nil", then we're implying we can never send any message unknown tonil to any object, because at any point, that object might be nil. _If you let it._ Which I contend you should not.

Using #zero? allows us to define a contract for a method's input. One that in turn encourages better contracts for method output. Union types in general, and nil in particular at least partially undo the benefits we get from duck typing.

That said, this might not be the right place for such a discussion. I am okay with making the auto-correct disabled by default since, after all, people tend to introduce a lot of nils in their programs, whether intentional or not. 😀

[noniq]

I just stumbled upon this, too. In my case the code that triggered the cop was:

Rails.logger.level = Logger::ERROR if env['PATH_INFO'].index("/assets/") == 0

String#index is documented to return nil if no match is found, so using #zero? here is not an option (same for Array#index, by the way).

One could argue though that this specific case should trigger another cop that recommends replacing index(str) == 0 with start_with?(str). But RuboCop should definitely _not_ recommend using #zero? here.

[ptarjan]

@Drenmi I personally agree with you. Whenever possible types should be non-nullable. Most of my company's utility methods and core code uses non-nullable types. But up in userland and closer to the top of the stack, null is a very real and present thing in code.

[Drenmi]

@ptarjan: Indeed. We can't really get away from that fact, and because of the dynamic nature of the language, we're very limited in what we can do outside of runtime. 😞 This is the biggest limitation for RuboCop, as you have rightly pointed out.

[montulli]

.zero? and .nonzero? bit us too. It turns out that "Time" and "Datetime" do not support ".zero?" and ".nonzero?" and we failed. ;(

I would prefer that this check be off by default.

[Drenmi]

It turns out that "Time" and "Datetime" do not support ".zero?" and ".nonzero?" and we failed.

A good question to ask first is "why are we comparing a Time to a number?", since it is essentially the same as using if false. 😀 '

Considering the number of issues this caused in the wild, I will submit a PR to make the auto-correct disabled by default. 😀