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/ClassEqualityComparison cop #8833

Merged
merged 1 commit into from Oct 8, 2020
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.md
Expand Up @@ -5,6 +5,7 @@
### New features

* [#8796](https://github.com/rubocop-hq/rubocop/pull/8796): Add new `Lint/HashCompareByIdentity` cop. ([@fatkodima][])
* [#8833](https://github.com/rubocop-hq/rubocop/pull/8833): Add new `Style/ClassEqualityComparison` cop. ([@fatkodima][])
* [#8668](https://github.com/rubocop-hq/rubocop/pull/8668): Add new `Lint/RedundantSafeNavigation` cop. ([@fatkodima][])

### Bug fixes
Expand Down
10 changes: 10 additions & 0 deletions config/default.yml
Expand Up @@ -2724,6 +2724,16 @@ Style/ClassCheck:
- is_a?
- kind_of?

Style/ClassEqualityComparison:
Description: 'Enforces the use of `Object#instance_of?` instead of class comparison for equality.'
StyleGuide: '#instance-of-vs-class-comparison'
Enabled: pending
VersionAdded: '0.93'
IgnoredMethods:
- ==
- equal?
- eql?

Style/ClassMethods:
Description: 'Use self when defining module/class methods.'
StyleGuide: '#def-self-class-methods'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -351,6 +351,7 @@ In the following section you find all available cops:
* xref:cops_style.adoc#stylecharacterliteral[Style/CharacterLiteral]
* xref:cops_style.adoc#styleclassandmodulechildren[Style/ClassAndModuleChildren]
* xref:cops_style.adoc#styleclasscheck[Style/ClassCheck]
* xref:cops_style.adoc#styleclassequalitycomparison[Style/ClassEqualityComparison]
* xref:cops_style.adoc#styleclassmethods[Style/ClassMethods]
* xref:cops_style.adoc#styleclassmethodsdefinitions[Style/ClassMethodsDefinitions]
* xref:cops_style.adoc#styleclassvars[Style/ClassVars]
Expand Down
43 changes: 43 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -1052,6 +1052,49 @@ var.kind_of?(String)

* https://rubystyle.guide#is-a-vs-kind-of

== Style/ClassEqualityComparison

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Pending
| Yes
| Yes
| 0.93
| -
|===

This cop enforces the use of `Object#instance_of?` instead of class comparison
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, you can use kind_of? as well. I think we have some other cop for this, so that one should honor the config of the existing cop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kind_of? is an alias for is_a?, which checks also superclasses. So this should be instance_of? for sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I keep mixing those up. :-)

for equality.

=== Examples

[source,ruby]
----
# bad
var.class == Date
var.class.equal?(Date)
var.class.eql?(Date)
var.class.name == 'Date'

# good
var.instance_of?(Date)
----

=== Configurable attributes

|===
| Name | Default value | Configurable values

| IgnoredMethods
| `==`, `equal?`, `eql?`
| Array
|===

=== References

* https://rubystyle.guide#instance-of-vs-class-comparison

== Style/ClassMethods

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -399,6 +399,7 @@
require_relative 'rubocop/cop/style/character_literal'
require_relative 'rubocop/cop/style/class_and_module_children'
require_relative 'rubocop/cop/style/class_check'
require_relative 'rubocop/cop/style/class_equality_comparison'
require_relative 'rubocop/cop/style/class_methods'
require_relative 'rubocop/cop/style/class_methods_definitions'
require_relative 'rubocop/cop/style/class_vars'
Expand Down
49 changes: 49 additions & 0 deletions lib/rubocop/cop/style/class_equality_comparison.rb
@@ -0,0 +1,49 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# This cop enforces the use of `Object#instance_of?` instead of class comparison
# for equality.
#
# @example
# # bad
# var.class == Date
# var.class.equal?(Date)
# var.class.eql?(Date)
# var.class.name == 'Date'
#
# # good
# var.instance_of?(Date)
#
class ClassEqualityComparison < Base
include RangeHelp
include IgnoredMethods
extend AutoCorrector

MSG = 'Use `Object.instance_of?` instead of comparing classes.'

RESTRICT_ON_SEND = %i[== equal? eql?].freeze

def_node_matcher :class_comparison_candidate?, <<~PATTERN
(send
{$(send _ :class) (send $(send _ :class) :name)}
{:== :equal? :eql?} $_)
PATTERN

def on_send(node)
def_node = node.each_ancestor(:def, :defs).first
return if def_node && ignored_method?(def_node.method_name)

class_comparison_candidate?(node) do |receiver_node, class_node|
range = range_between(receiver_node.loc.selector.begin_pos, node.source_range.end_pos)

add_offense(range) do |corrector|
corrector.replace(range, "instance_of?(#{class_node.source})")
end
end
end
end
end
end
end
46 changes: 46 additions & 0 deletions spec/rubocop/cop/style/class_equality_comparison_spec.rb
@@ -0,0 +1,46 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::ClassEqualityComparison, :config do
let(:cop_config) do
{ 'IgnoredMethods' => [] }
end

it 'registers an offense and corrects when comparing class for equality' do
expect_offense(<<~RUBY)
var.class == Date
^^^^^^^^^^^^^ Use `Object.instance_of?` instead of comparing classes.
var.class.equal?(Date)
^^^^^^^^^^^^^^^^^^ Use `Object.instance_of?` instead of comparing classes.
var.class.eql?(Date)
^^^^^^^^^^^^^^^^ Use `Object.instance_of?` instead of comparing classes.
RUBY
end

it 'registers an offense and corrects when comparing class name for equality' do
expect_offense(<<~RUBY)
var.class.name == "Date"
^^^^^^^^^^^^^^^^^^^^ Use `Object.instance_of?` instead of comparing classes.
RUBY
end

it 'does not register an offense when using `instance_of?`' do
expect_no_offenses(<<~RUBY)
var.instance_of?(Date)
RUBY
end

context 'when IgnoredMethods is specified' do
let(:cop_config) do
{ 'IgnoredMethods' => ['=='] }
end

it 'does not register an offense when comparing class for equality' do
expect_no_offenses(<<~RUBY)
def ==(other)
self.class == other.class &&
name == other.name
end
RUBY
end
end
end