From 795f7ccb80be01f2704fc670958b24ac3d79471b Mon Sep 17 00:00:00 2001 From: Michael Nikitochkin Date: Tue, 6 Feb 2024 15:11:18 +0100 Subject: [PATCH] Add Performance::NumericPredicate cop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...new_add_performancemumericpredicate_cop.md | 1 + config/default.yml | 6 ++ .../cop/performance/numeric_predicate.rb | 61 +++++++++++++ lib/rubocop/cop/performance_cops.rb | 1 + .../cop/performance/numeric_predicate_spec.rb | 88 +++++++++++++++++++ 5 files changed, 157 insertions(+) create mode 100644 changelog/new_add_performancemumericpredicate_cop.md create mode 100644 lib/rubocop/cop/performance/numeric_predicate.rb create mode 100644 spec/rubocop/cop/performance/numeric_predicate_spec.rb diff --git a/changelog/new_add_performancemumericpredicate_cop.md b/changelog/new_add_performancemumericpredicate_cop.md new file mode 100644 index 0000000000..a16c45f0e8 --- /dev/null +++ b/changelog/new_add_performancemumericpredicate_cop.md @@ -0,0 +1 @@ +* [#440](https://github.com/rubocop/rubocop-performance/pull/440): Add Performance::NumericPredicate cop. ([@miry][]) diff --git a/config/default.yml b/config/default.yml index a508a39827..17c4c5d8f7 100644 --- a/config/default.yml +++ b/config/default.yml @@ -205,6 +205,12 @@ Performance/MethodObjectAsBlock: Enabled: pending VersionAdded: '1.9' +Performance/NumericPredicate: + Description: 'Use compare operator instead of `Numeric#positive?`, `Numeric#negative?`, or `Numeric#zero?`.' + Enabled: pending + Safe: false + VersionAdded: '<>' + Performance/OpenStruct: Description: 'Use `Struct` instead of `OpenStruct`.' Enabled: false diff --git a/lib/rubocop/cop/performance/numeric_predicate.rb b/lib/rubocop/cop/performance/numeric_predicate.rb new file mode 100644 index 0000000000..85dfc73d08 --- /dev/null +++ b/lib/rubocop/cop/performance/numeric_predicate.rb @@ -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 `%s` instead of `%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.receiver + + 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 diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 2a18f26120..78cbc0a102 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -27,6 +27,7 @@ require_relative 'performance/map_compact' require_relative 'performance/map_method_chain' require_relative 'performance/method_object_as_block' +require_relative 'performance/numeric_predicate' require_relative 'performance/open_struct' require_relative 'performance/range_include' require_relative 'performance/io_readlines' diff --git a/spec/rubocop/cop/performance/numeric_predicate_spec.rb b/spec/rubocop/cop/performance/numeric_predicate_spec.rb new file mode 100644 index 0000000000..0495792be2 --- /dev/null +++ b/spec/rubocop/cop/performance/numeric_predicate_spec.rb @@ -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