Skip to content

Commit

Permalink
Add AllowRegexpMatch option to `Performance/RedundantEqualityCompar…
Browse files Browse the repository at this point in the history
…isonBlock`

Follow up #340.

This PR adds `AllowRegexpMatch` option to `Performance/RedundantEqualityComparisonBlock`.

```ruby
# `AllowRegexpMatch: true` (default)

# good
items.all? { |item| item =~ pattern }
items.all? { |item| item.match?(pattern) }

# `AllowRegexpMatch: false`

# bad
items.all? { |item| item =~ pattern }
items.all? { |item| item.match?(pattern) }
```

`AllowRegexpMatch: false` by default because `regexp.match?('string')` often used in block
changes to the opposite result:

```ruby
[/pattern/].all? { |regexp| regexp.match?('pattern') } # => true
[/pattern/].all? { |regexp| regexp =~ 'pattern' }      # => true
[/pattern/].all?('pattern')                            # => false
```
  • Loading branch information
koic committed Mar 13, 2023
1 parent 59a4509 commit a160f92
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 26 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#347](https://github.com/rubocop/rubocop-performance/pull/347): Add `AllowRegexpMatch` option to `Performance/RedundantEqualityComparisonBlock`. ([@koic][])
1 change: 1 addition & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ Performance/RedundantEqualityComparisonBlock:
Reference: 'https://github.com/rails/rails/pull/41363'
Enabled: pending
Safe: false
AllowRegexpMatch: false
VersionAdded: '1.10'

Performance/RedundantMatch:
Expand Down
37 changes: 33 additions & 4 deletions lib/rubocop/cop/performance/redundant_equality_comparison_block.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,42 @@ module Performance
# behavior is appropriately overridden in subclass. For example,
# `Range#===` returns `true` when argument is within the range.
#
# This cop has `AllowRegexpMatch` option and it is false by default because
# `regexp.match?('string')` often used in block changes to the opposite result:
#
# [source,ruby]
# ----
# [/pattern/].all? { |regexp| regexp.match?('pattern') } # => true
# [/pattern/].all? { |regexp| regexp =~ 'pattern' } # => true
# [/pattern/].all?('pattern') # => false
# ----
#
# @safety
# This cop is unsafe because `===` and `==` do not always behave the same.
#
# @example
# # bad
# items.all? { |item| pattern === item }
# items.all? { |item| item == other }
# items.all? { |item| item =~ pattern }
# items.all? { |item| item.is_a?(Klass) }
# items.all? { |item| item.kind_of?(Klass) }
# items.all? { |item| item.match?(pattern) }
#
# # good
# items.all?(pattern)
# items.all?(Klass)
#
# @example AllowRegexpMatch: true (default)
#
# # good
# items.all? { |item| item =~ pattern }
# items.all? { |item| item.match?(pattern) }
#
# @example AllowRegexpMatch: false
#
# # bad
# items.all? { |item| item =~ pattern }
# items.all? { |item| item.match?(pattern) }
#
class RedundantEqualityComparisonBlock < Base
extend AutoCorrector
extend TargetRubyVersion
Expand All @@ -35,7 +55,8 @@ class RedundantEqualityComparisonBlock < Base
MSG = 'Use `%<prefer>s` instead of block.'

TARGET_METHODS = %i[all? any? one? none?].freeze
COMPARISON_METHODS = %i[== === =~ is_a? kind_of? match?].freeze
COMPARISON_METHODS = %i[== === is_a? kind_of?].freeze
REGEXP_METHODS = %i[=~ match?].freeze
IS_A_METHODS = %i[is_a? kind_of?].freeze

def on_block(node)
Expand Down Expand Up @@ -63,7 +84,11 @@ def one_block_argument?(block_arguments)
end

def use_equality_comparison_block?(block_body)
block_body.send_type? && COMPARISON_METHODS.include?(block_body.method_name)
return false unless block_body.send_type?

method_name = block_body.method_name

COMPARISON_METHODS.include?(method_name) || (!allow_regexp_match? && REGEXP_METHODS.include?(method_name))
end

def same_block_argument_and_is_a_argument?(block_body, block_argument)
Expand Down Expand Up @@ -102,6 +127,10 @@ def use_block_argument_in_method_argument_of_operand?(block_argument, operand)
def offense_range(node)
node.send_node.loc.selector.join(node.source_range.end)
end

def allow_regexp_match?
cop_config.fetch('AllowRegexpMatch', true)
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,6 @@
RUBY
end

it "registers and corrects an offense when using `#{method_name}` with `=~` comparison block" do
expect_offense(<<~RUBY, method_name: method_name)
items.#{method_name} { |item| item =~ other }
^{method_name}^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method_name}(other)` instead of block.
RUBY

expect_correction(<<~RUBY)
items.#{method_name}(other)
RUBY
end

it "registers and corrects an offense when using `#{method_name}` with `is_a?` comparison block" do
expect_offense(<<~RUBY, method_name: method_name)
items.#{method_name} { |item| item.is_a?(Klass) }
Expand All @@ -58,17 +47,6 @@
RUBY
end

it "registers and corrects an offense when using `#{method_name}` with `match?` comparison block" do
expect_offense(<<~RUBY, method_name: method_name)
items.#{method_name} { |item| item.match?(pattern) }
^{method_name}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method_name}(pattern)` instead of block.
RUBY

expect_correction(<<~RUBY)
items.#{method_name}(pattern)
RUBY
end

it "does not register an offense when using `#{method_name}` with `===` comparison block and" \
'block argument is not used as a receiver for `===`' do
expect_no_offenses(<<~RUBY, method_name: method_name)
Expand Down Expand Up @@ -129,6 +107,40 @@
items.do_something { |item| item == other }
RUBY
end

context 'when `AllowRegexpMatch: true`' do
let(:cop_config) { { 'AllowRegexpMatch' => true } }

it 'does not register an offense when using target method` with `=~` comparison block' do
expect_no_offenses(<<~RUBY)
items.all? { |item| item =~ pattern }
RUBY
end

it 'does not register an offense when using target method with `match?` comparison block' do
expect_no_offenses(<<~RUBY)
items.all? { |item| item.match?(pattern) }
RUBY
end
end

context 'when `AllowRegexpMatch: false`' do
let(:cop_config) { { 'AllowRegexpMatch' => false } }

it 'registers an offense when using target method with `=~` comparison block' do
expect_offense(<<~RUBY)
items.all? { |item| item =~ pattern }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `all?(pattern)` instead of block.
RUBY
end

it 'registers an offense when using target method with `match?` comparison block' do
expect_offense(<<~RUBY)
items.all? { |item| item.match?(pattern) }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `all?(pattern)` instead of block.
RUBY
end
end
end

context 'when TargetRubyVersion <= 2.4', :ruby24 do
Expand Down

0 comments on commit a160f92

Please sign in to comment.