Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add Performance::NumericPredicate cop
Performance::NumericPredicate cop identifies places where numeric uses predicates like `positive?`, `negative?` and for some cases `zero?` should be converted to compare operator. The `Performance::NumericPredicate` cop is added to identify instances where numeric predicates such as `positive?`, `negative?`, and occasionally `zero?` should be replaced with comparison operators for improved efficiency. Predicates incur a performance overhead by executing a method before comparison. A small benchmark comparison between using a comparison operator (`> 0`) and `positive?` illustrates the performance difference: ```ruby x.report("compare with 0") { arr.each {|i| i > 0 } } x.report("positive?") { arr.each {|i| i.positive? } } ``` Benchmark results on Ruby 3.3.0 (with YJIT) indicate a significant performance gain when using the comparison operator: ``` ruby 3.3.0 (2023-12-25 revision 5124f9ac75) +YJIT [arm64-darwin23] Warming up -------------------------------------- compare with 0 1.000 i/100ms positive? 1.000 i/100ms Calculating ------------------------------------- compare with 0 3.153 (± 0.0%) i/s - 95.000 in 30.132600s positive? 2.397 (± 0.0%) i/s - 72.000 in 30.042688s Comparison: compare with 0: 3.2 i/s positive?: 2.4 i/s - 1.32x slower ``` This cop is unsafe because it cannot be guaranteed that the receiver is Number and could be noisy. Signed-off-by: Michael Nikitochkin <michael.nikitochkin@gmx.net>
- Loading branch information
Showing
5 changed files
with
157 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#440](https://github.com/rubocop/rubocop-performance/pull/440): Add Performance::NumericPredicate cop. ([@miry][]) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Performance | ||
# Identifies places where numeric uses predicates `positive?`, and `negative?` should be | ||
# converted to compare operator. | ||
# | ||
# @safety | ||
# This cop is unsafe because it cannot be guaranteed that the receiver | ||
# defines the predicates or can be compared to a number, which may lead | ||
# to a false positive for non-standard classes. | ||
# | ||
# @example | ||
# # bad | ||
# 1.positive? | ||
# 1.43.negative? | ||
# -4.zero? | ||
# | ||
# # good | ||
# 1 > 0 | ||
# 1.43 < 0.0 | ||
# -4 == 0 | ||
# | ||
class NumericPredicate < Base | ||
extend AutoCorrector | ||
|
||
MSG = 'Use compare operator `%<good>s` instead of `%<bad>s`.' | ||
RESTRICT_ON_SEND = %i[positive? zero? negative?].freeze | ||
REPLACEMENTS = { negative?: '<', positive?: '>', zero?: '==' }.freeze | ||
|
||
def_node_matcher :num_predicate?, <<~PATTERN | ||
(send $numeric_type? ${:negative? :positive? :zero?}) | ||
PATTERN | ||
|
||
def_node_matcher :instance_predicate?, <<~PATTERN | ||
(send $!nil? ${:negative? :positive?}) | ||
PATTERN | ||
|
||
def on_send(node) | ||
return unless num_predicate?(node) || instance_predicate?(node) | ||
return unless node.children | ||
|
||
good_method = build_good_method(node.receiver, node.method_name) | ||
message = format(MSG, good: good_method, bad: node.source) | ||
add_offense(node, message: message) do |corrector| | ||
corrector.replace(node, good_method) | ||
end | ||
end | ||
|
||
private | ||
|
||
def build_good_method(receiver, method) | ||
operation = REPLACEMENTS[method] | ||
zero = receiver&.float_type? ? 0.0 : 0 | ||
"#{receiver.source} #{operation} #{zero}" | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.describe RuboCop::Cop::Performance::NumericPredicate, :config do | ||
let(:message) { RuboCop::Cop::Performance::NumericPredicate::MSG } | ||
|
||
shared_examples 'common functionality' do |method, op| | ||
it 'for integer' do | ||
expect_offense(<<~RUBY, method: method) | ||
1.#{method} | ||
^^^{method} Use compare operator `1 #{op} 0` instead of `1.#{method}`. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
1 #{op} 0 | ||
RUBY | ||
end | ||
|
||
it 'for float' do | ||
expect_offense(<<~RUBY, method: method) | ||
1.2.#{method} | ||
^^^^^{method} Use compare operator `1.2 #{op} 0.0` instead of `1.2.#{method}`. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
1.2 #{op} 0.0 | ||
RUBY | ||
end | ||
|
||
it 'ignore big decimal' do | ||
next if method == 'zero?' | ||
|
||
expect_offense(<<~RUBY, method: method) | ||
BigDecimal('1', 2).#{method} | ||
^^^^^^^^^^^^^^^^^^^^{method} Use compare operator `BigDecimal('1', 2) #{op} 0` instead of `BigDecimal('1', 2).#{method}`. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
BigDecimal('1', 2) #{op} 0 | ||
RUBY | ||
end | ||
|
||
it 'for variable' do | ||
next if method == 'zero?' | ||
|
||
expect_offense(<<~RUBY, method: method) | ||
foo = 1 | ||
foo.#{method} | ||
^^^^^{method} Use compare operator `foo #{op} 0` instead of `foo.#{method}`. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
foo = 1 | ||
foo #{op} 0 | ||
RUBY | ||
end | ||
|
||
it 'in condition statements' do | ||
next if method == 'zero?' | ||
|
||
expect_offense(<<~RUBY, method: method) | ||
foo = 1 | ||
if foo.#{method} | ||
^^^^^{method} Use compare operator `foo #{op} 0` instead of `foo.#{method}`. | ||
end | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
foo = 1 | ||
if foo #{op} 0 | ||
end | ||
RUBY | ||
end | ||
|
||
it 'in map statement' do | ||
next if method == 'zero?' | ||
|
||
expect_no_offenses(<<~RUBY, method: method) | ||
foo = [1, 2, 3] | ||
if foo.all?(&:#{method}) | ||
end | ||
RUBY | ||
end | ||
end | ||
|
||
it_behaves_like 'common functionality', 'positive?', '>' | ||
it_behaves_like 'common functionality', 'negative?', '<' | ||
it_behaves_like 'common functionality', 'zero?', '==' | ||
end |