Skip to content

Commit 795f7cc

Browse files
committed
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 <[email protected]>
1 parent 4230dc0 commit 795f7cc

File tree

5 files changed

+157
-0
lines changed

5 files changed

+157
-0
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#440](https://github.com/rubocop/rubocop-performance/pull/440): Add Performance::NumericPredicate cop. ([@miry][])

config/default.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,12 @@ Performance/MethodObjectAsBlock:
205205
Enabled: pending
206206
VersionAdded: '1.9'
207207

208+
Performance/NumericPredicate:
209+
Description: 'Use compare operator instead of `Numeric#positive?`, `Numeric#negative?`, or `Numeric#zero?`.'
210+
Enabled: pending
211+
Safe: false
212+
VersionAdded: '<<next>>'
213+
208214
Performance/OpenStruct:
209215
Description: 'Use `Struct` instead of `OpenStruct`.'
210216
Enabled: false
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Performance
6+
# Identifies places where numeric uses predicates `positive?`, and `negative?` should be
7+
# converted to compare operator.
8+
#
9+
# @safety
10+
# This cop is unsafe because it cannot be guaranteed that the receiver
11+
# defines the predicates or can be compared to a number, which may lead
12+
# to a false positive for non-standard classes.
13+
#
14+
# @example
15+
# # bad
16+
# 1.positive?
17+
# 1.43.negative?
18+
# -4.zero?
19+
#
20+
# # good
21+
# 1 > 0
22+
# 1.43 < 0.0
23+
# -4 == 0
24+
#
25+
class NumericPredicate < Base
26+
extend AutoCorrector
27+
28+
MSG = 'Use compare operator `%<good>s` instead of `%<bad>s`.'
29+
RESTRICT_ON_SEND = %i[positive? zero? negative?].freeze
30+
REPLACEMENTS = { negative?: '<', positive?: '>', zero?: '==' }.freeze
31+
32+
def_node_matcher :num_predicate?, <<~PATTERN
33+
(send $numeric_type? ${:negative? :positive? :zero?})
34+
PATTERN
35+
36+
def_node_matcher :instance_predicate?, <<~PATTERN
37+
(send $!nil? ${:negative? :positive?})
38+
PATTERN
39+
40+
def on_send(node)
41+
return unless num_predicate?(node) || instance_predicate?(node)
42+
return unless node.receiver
43+
44+
good_method = build_good_method(node.receiver, node.method_name)
45+
message = format(MSG, good: good_method, bad: node.source)
46+
add_offense(node, message: message) do |corrector|
47+
corrector.replace(node, good_method)
48+
end
49+
end
50+
51+
private
52+
53+
def build_good_method(receiver, method)
54+
operation = REPLACEMENTS[method]
55+
zero = receiver&.float_type? ? 0.0 : 0
56+
"#{receiver.source} #{operation} #{zero}"
57+
end
58+
end
59+
end
60+
end
61+
end

lib/rubocop/cop/performance_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
require_relative 'performance/map_compact'
2828
require_relative 'performance/map_method_chain'
2929
require_relative 'performance/method_object_as_block'
30+
require_relative 'performance/numeric_predicate'
3031
require_relative 'performance/open_struct'
3132
require_relative 'performance/range_include'
3233
require_relative 'performance/io_readlines'
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Performance::NumericPredicate, :config do
4+
let(:message) { RuboCop::Cop::Performance::NumericPredicate::MSG }
5+
6+
shared_examples 'common functionality' do |method, op|
7+
it 'for integer' do
8+
expect_offense(<<~RUBY, method: method)
9+
1.#{method}
10+
^^^{method} Use compare operator `1 #{op} 0` instead of `1.#{method}`.
11+
RUBY
12+
13+
expect_correction(<<~RUBY)
14+
1 #{op} 0
15+
RUBY
16+
end
17+
18+
it 'for float' do
19+
expect_offense(<<~RUBY, method: method)
20+
1.2.#{method}
21+
^^^^^{method} Use compare operator `1.2 #{op} 0.0` instead of `1.2.#{method}`.
22+
RUBY
23+
24+
expect_correction(<<~RUBY)
25+
1.2 #{op} 0.0
26+
RUBY
27+
end
28+
29+
it 'ignore big decimal' do
30+
next if method == 'zero?'
31+
32+
expect_offense(<<~RUBY, method: method)
33+
BigDecimal('1', 2).#{method}
34+
^^^^^^^^^^^^^^^^^^^^{method} Use compare operator `BigDecimal('1', 2) #{op} 0` instead of `BigDecimal('1', 2).#{method}`.
35+
RUBY
36+
37+
expect_correction(<<~RUBY)
38+
BigDecimal('1', 2) #{op} 0
39+
RUBY
40+
end
41+
42+
it 'for variable' do
43+
next if method == 'zero?'
44+
45+
expect_offense(<<~RUBY, method: method)
46+
foo = 1
47+
foo.#{method}
48+
^^^^^{method} Use compare operator `foo #{op} 0` instead of `foo.#{method}`.
49+
RUBY
50+
51+
expect_correction(<<~RUBY)
52+
foo = 1
53+
foo #{op} 0
54+
RUBY
55+
end
56+
57+
it 'in condition statements' do
58+
next if method == 'zero?'
59+
60+
expect_offense(<<~RUBY, method: method)
61+
foo = 1
62+
if foo.#{method}
63+
^^^^^{method} Use compare operator `foo #{op} 0` instead of `foo.#{method}`.
64+
end
65+
RUBY
66+
67+
expect_correction(<<~RUBY)
68+
foo = 1
69+
if foo #{op} 0
70+
end
71+
RUBY
72+
end
73+
74+
it 'in map statement' do
75+
next if method == 'zero?'
76+
77+
expect_no_offenses(<<~RUBY, method: method)
78+
foo = [1, 2, 3]
79+
if foo.all?(&:#{method})
80+
end
81+
RUBY
82+
end
83+
end
84+
85+
it_behaves_like 'common functionality', 'positive?', '>'
86+
it_behaves_like 'common functionality', 'negative?', '<'
87+
it_behaves_like 'common functionality', 'zero?', '=='
88+
end

0 commit comments

Comments
 (0)