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

Add new Style/InvertibleUnlessCondition cop #11432

Merged
merged 1 commit into from
Jan 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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?

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

# 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