Skip to content

Commit

Permalink
Add new Style/InvertibleUnlessCondition cop
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima authored and bbatsov committed Jan 23, 2023
1 parent 7fdb89b commit 8750cd6
Show file tree
Hide file tree
Showing 5 changed files with 239 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_add_new_invertible_unless_condition_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#11410](https://github.com/rubocop/rubocop/issues/11410): Add new `Style/InvertibleUnlessCondition` cop. ([@fatkodima][])
29 changes: 29 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4036,6 +4036,35 @@ Style/InverseMethods:
:select: :reject
:select!: :reject!

Style/InvertibleUnlessCondition:
Description: 'Favor `if` with inverted condition over `unless`.'
Enabled: false
VersionAdded: '<<next>>'
# `InverseMethods` are methods that can be inverted in a `unless` condition.
# The relationship of inverse methods needs to be defined in both directions.
# Keys and values both need to be defined as symbols.
InverseMethods:
:!=: :==
:>: :<=
:<=: :>
:<: :>=
:>=: :<
:!~: :=~
:zero?: :nonzero?
:nonzero?: :zero?
:any?: :none?
:none?: :any?
:even?: :odd?
:odd?: :even?
# `ActiveSupport` defines some common inverse methods. They are listed below,
# and not enabled by default.
# :present?: :blank?
# :blank?: :present?
# :include?: :exclude?
# :exclude?: :include?
# :one?: :many?
# :many?: :one?

This comment has been minimized.

Copy link
@dduugg

dduugg Apr 18, 2023

Contributor

one? and many? aren't inverse methods, in case you're motivated to remove these examples (e.g. they return the same result on empty collections)

This comment has been minimized.

Copy link
@koic

koic Apr 19, 2023

Member

Good catch! The incorrect examples has been removed in c117aa1. Thank you.


Style/IpAddresses:
Description: "Don't include literal IP addresses in code."
Enabled: false
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@
require_relative 'rubocop/cop/style/infinite_loop'
require_relative 'rubocop/cop/style/inverse_methods'
require_relative 'rubocop/cop/style/inline_comment'
require_relative 'rubocop/cop/style/invertible_unless_condition'
require_relative 'rubocop/cop/style/ip_addresses'
require_relative 'rubocop/cop/style/keyword_parameters_order'
require_relative 'rubocop/cop/style/lambda'
Expand Down
114 changes: 114 additions & 0 deletions lib/rubocop/cop/style/invertible_unless_condition.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# Checks for usages of `unless` which can be replaced by `if` with inverted condition.
# Code without `unless` is easier to read, but that is subjective, so this cop
# is disabled by default.
#
# Methods that can be inverted should be defined in `InverseMethods`. Note that
# the relationship of inverse methods needs to be defined in both directions.
# For example,
# InverseMethods:
# :!=: :==
# :even?: :odd?
# :odd?: :even?
#
# will suggest both `even?` and `odd?` to be inverted, but only `!=` (and not `==`).
#
# @safety
# This cop is unsafe because it cannot be guaranteed that the method
# and its inverse method are both defined on receiver, and also are
# actually inverse of each other.
#
# @example
# # bad (simple condition)
# foo unless !bar
# foo unless x != y
# foo unless x >= 10
# foo unless x.even?
#
# # good
# foo if bar
# foo if x == y
# foo if x < 10
# foo if x.odd?
#
# # bad (complex condition)
# foo unless x != y || x.even?
#
# # good
# foo if x == y && x.odd?
#
# # good (if)
# foo if !condition
#
class InvertibleUnlessCondition < Base
extend AutoCorrector

MSG = 'Favor `if` with inverted condition over `unless`.'

def on_if(node)
return unless node.unless?

condition = node.condition
return unless invertible?(condition)

add_offense(node) do |corrector|
corrector.replace(node.loc.keyword, node.inverse_keyword)
autocorrect(corrector, condition)
end
end

private

def invertible?(node)
case node.type
when :begin
invertible?(node.children.first)
when :send
return if inheritance_check?(node)

node.method?(:!) || inverse_methods.key?(node.method_name)
when :or, :and
invertible?(node.lhs) && invertible?(node.rhs)
else
false
end
end

def inheritance_check?(node)
argument = node.first_argument
node.method?(:<) &&
(argument.const_type? && argument.short_name.to_s.upcase != argument.short_name.to_s)
end

def autocorrect(corrector, node)
case node.type
when :begin
autocorrect(corrector, node.children.first)
when :send
autocorrect_send_node(corrector, node)
when :or, :and
corrector.replace(node.loc.operator, node.inverse_operator)
autocorrect(corrector, node.lhs)
autocorrect(corrector, node.rhs)
end
end

def autocorrect_send_node(corrector, node)
if node.method?(:!)
corrector.remove(node.loc.selector)
else
corrector.replace(node.loc.selector, inverse_methods[node.method_name])
end
end

def inverse_methods
@inverse_methods ||= cop_config['InverseMethods']
end
end
end
end
end
94 changes: 94 additions & 0 deletions spec/rubocop/cop/style/invertible_unless_condition_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::InvertibleUnlessCondition, :config do
let(:cop_config) do
{
'InverseMethods' => {
:!= => :==,
:even? => :odd?,
:odd? => :even?,
:>= => :<,
:< => :>=,
:zero? => :nonzero?
}
}
end

context 'when invertible `unless`' do
it 'registers an offense and corrects when using `!` negation' do
expect_offense(<<~RUBY)
foo unless !x
^^^^^^^^^^^^^ Favor `if` with inverted condition over `unless`.
foo unless !!x
^^^^^^^^^^^^^^ Favor `if` with inverted condition over `unless`.
RUBY

expect_correction(<<~RUBY)
foo if x
foo if !x
RUBY
end

it 'registers an offense and corrects when using simple operator condition' do
expect_offense(<<~RUBY)
foo unless x != y
^^^^^^^^^^^^^^^^^ Favor `if` with inverted condition over `unless`.
RUBY

expect_correction(<<~RUBY)
foo if x == y
RUBY
end

it 'registers an offense and corrects when using simple method condition' do
expect_offense(<<~RUBY)
foo unless x.odd?
^^^^^^^^^^^^^^^^^ Favor `if` with inverted condition over `unless`.
RUBY

expect_correction(<<~RUBY)
foo if x.even?
RUBY
end

it 'registers an offense and corrects when using simple bracketed condition' do
expect_offense(<<~RUBY)
foo unless ((x != y))
^^^^^^^^^^^^^^^^^^^^^ Favor `if` with inverted condition over `unless`.
RUBY

expect_correction(<<~RUBY)
foo if ((x == y))
RUBY
end

it 'registers an offense and corrects when using complex condition' do
expect_offense(<<~RUBY)
foo unless x != y && (((x.odd?) || (((y >= 5)))) || z.zero?)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Favor `if` with inverted condition over `unless`.
RUBY

expect_correction(<<~RUBY)
foo if x == y || (((x.even?) && (((y < 5)))) && z.nonzero?)
RUBY
end
end

it 'does not register an offense when using non invertible `unless`' do
expect_no_offenses(<<~RUBY)
foo unless x != y || x.awesome?
RUBY
end

it 'does not register an offense when checking for inheritance' do
expect_no_offenses(<<~RUBY)
foo unless x < Foo
RUBY
end

it 'does not register an offense when using invertible `if`' do
expect_no_offenses(<<~RUBY)
foo if !condition
RUBY
end
end

0 comments on commit 8750cd6

Please sign in to comment.