Add new Style/InvertibleUnlessCondition cop

[fatkodima]

Closes #11410.

Tested on rails/rails - all the offenses looks valid to me.

As not everyone would agree that the new version becomes better (I am not one of them, I would rather prefer if over unless in any case), I decided to accept by default cases like the following:

foo unless x == y # otherwise would be converted to `foo if x != y`
foo unless x =~ y # otherwise would be converted to `foo if x !~ y`
foo unless x < Foo # checking for class inheritance, `x >= Foo` looks confusing imo?

[bbatsov]

I don't really like many of the changes this cop does, so I'd suggest to have it be disabled by default. E.g. I'd never make this change as I don't see any readability gains from it:

# before
return [] unless lockfile.dependencies.any?

# after
return [] if lockfile.dependencies.none?

I get that some people might dislike unless, but I think that only when the condition was negated there are some real gains.

[fatkodima]

Instead of completely disabling, we can tweak InverseMethods config, for example: remove :any?: :none? and :zero?: :nonzero? conversions, so we do not get if with negations, like in the example you provided.

If you insist on disabling by default, will do that with no problems.

[bbatsov]

I think the config makes sense for whoever wants to use the cop (that's why I think the config is fine as it is), but I don't really have issues with unless myself, so I'm not interested in applying this to RuboCop's codebase.

Keep in mind that we've been promising people for a while to enable fewer controversial cops (if I could start all over I'd probably disable half of all cops by default), that's why I think it's best to leave it disabled by default as well and leave it to whoever needs it to enable it.

[fatkodima]

so I'm interested in applying this to RuboCop's codebase.

Sorry, did you mean just to introduce the cop or to also enable it in the rubocop itself (in .rubocop.yml file)?

[koic]

IMO, RuboCop’s codebase doesn't need to be different from the default, so the .rubocop.yml shouldn't be applied differently than the default for the new rule.

[vlad-pisanov]

My 2c -- I think this cop should only discourage unless with an invertible negated condition, not all invertible unless statements (similar to my proposal #11399).

unless !x         # double negation ❌
unless x.none?    # double negation ❌
unless x.nonzero? # double negation ❌
unless x          # ok ✔️
unless x.any?     # ok ✔️
unless x.zero?    # ok ✔️

[koic]

#11399 is similar to this one, but different rule. #11399 features De Morgan’s laws.

[vlad-pisanov]

@koic yes, it's a somewhat different rule, but I feel it's driven by the same underlying sentiment -- readable code should negate things as few times as possible. 🙂

[bbatsov]

Sorry, did you mean just to introduce the cop or to also enable it in the rubocop itself (in .rubocop.yml file)?

I made a typo and forgot a "not". :-)

unless !x         # double negation ❌
unless x.none?    # double negation ❌
unless x.nonzero? # double negation ❌

Frankly, only the first one reads like a true negation to me. Yeah, you can x unless none and x if some are pretty much the same in terms of readability for me. I guess I'm more inclined to agree for nonzero as that is one pretty weird predicate, but my point stands - this is way too subjective to be encouraged broadly.

Funny enough the cop this make the following suggestion to flip an unless:

# original
return false unless (exponent.to_i % 3).zero?
# changed
return false if (exponent.to_i % 3).nonzero?

So here, we actually added a negation and made the predicate heavier to process mentally.

[fatkodima]

Made the cop disabled by default and reverted cop corrections.

@bbatsov

Funny enough the cop this make the following suggestion to flip an unless:

# original
return false unless (exponent.to_i % 3).zero?
# changed
return false if (exponent.to_i % 3).nonzero?

It would be easier to process (for me) if the changed version was written like return false if exponent.to_i % 3 != 0

But this is a style cop, so not everyone would like it.

@vlad-pisanov

My 2c -- I think this cop should only discourage unless with an invertible negated condition, not all invertible unless statements (similar to my proposal #11399).

It has a flexible InverseMethods, which can be configured to to handle this (for example, to not allow zero? => nonzero? conversion).

I will be happy to apply more flexibility, if there would be a need.

[bbatsov]

Probably it's a good idea to mention here that the idea is that code without unless is easier to read, but that's subjective that's why the cop is disabled by default.

[fatkodima]

Added.

[bbatsov]

I'm also wondering if a more generic version of this cop that suggests replacing negated methods with their complementary counterparts won't be useful.

[fatkodima]

There are already NegatedIf, NegatedUnless, NegatedWhile, InverseMethods (this looks the most like what you suggested) cops with a related purpose. Do we need to check something more that I missed? 🤔

[bbatsov]

Yeah, seems I was thinking of InverseMethods, but I had forgotten how we'd named it. Too many cops to remember them all! 😆