From 6bb06b26d3a0b7ead18afc85d9e72cc3013593cc Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 17 Sep 2024 12:25:03 +0900 Subject: [PATCH] [Fix #468] Fix false positives for `Performance/BigDecimalWithNumericArgument` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #468. This PR fixes false positives for `Performance/BigDecimalWithNumericArgument` when using float argument for `BigDecimal`. In Ruby 3.1 and later, cases where numbers are faster than strings are limited to `Integer`. For `Float`, strings are still faster: ```console $ cat example.rb require 'benchmark/ips' require 'bigdecimal' require 'bigdecimal/util' Benchmark.ips do |x| x.report('float string') { BigDecimal('4.2') } x.report('float with prec') { BigDecimal(4.2, Float::DIG + 1) } x.report('to_d string without prec') { '4.2'.to_d } x.report('to_d float without prec') { 4.2.to_d } x.report('to_d float with prec') { 4.2.to_d(Float::DIG + 1) } x.compare! end ``` ```console ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22] Warming up -------------------------------------- float string 246.214k i/100ms float with prec 173.880k i/100ms to_d string without prec 255.950k i/100ms to_d float without prec 181.033k i/100ms to_d float with prec 151.091k i/100ms Calculating ------------------------------------- float string 2.418M (± 5.5%) i/s - 12.064M in 5.004969s float with prec 1.685M (± 4.0%) i/s - 8.520M in 5.064059s to_d string without prec 2.460M (± 4.2%) i/s - 12.286M in 5.002392s to_d float without prec 1.781M (± 6.5%) i/s - 8.871M in 5.007829s to_d float with prec 1.584M (± 5.7%) i/s - 8.008M in 5.072184s Comparison: to_d string without prec: 2460462.7 i/s float string: 2418003.6 i/s - same-ish: difference falls within error to_d float without prec: 1781070.6 i/s - 1.38x slower float with prec: 1685372.9 i/s - 1.46x slower to_d float with prec: 1584419.5 i/s - 1.55x slower ``` --- ...mance_big_decimal_with_numeric_argument.md | 1 + .../big_decimal_with_numeric_argument.rb | 59 +++++++---- .../big_decimal_with_numeric_argument_spec.rb | 100 +++++++++--------- 3 files changed, 91 insertions(+), 69 deletions(-) create mode 100644 changelog/fix_false_positives_for_performance_big_decimal_with_numeric_argument.md diff --git a/changelog/fix_false_positives_for_performance_big_decimal_with_numeric_argument.md b/changelog/fix_false_positives_for_performance_big_decimal_with_numeric_argument.md new file mode 100644 index 0000000000..6db6995a73 --- /dev/null +++ b/changelog/fix_false_positives_for_performance_big_decimal_with_numeric_argument.md @@ -0,0 +1 @@ +* [#468](https://github.com/rubocop/rubocop-performance/issues/468): Fix false positives for `Performance/BigDecimalWithNumericArgument` when using float argument for `BigDecimal`. ([@koic][]) diff --git a/lib/rubocop/cop/performance/big_decimal_with_numeric_argument.rb b/lib/rubocop/cop/performance/big_decimal_with_numeric_argument.rb index 2041f32c79..2aa52b6fc7 100644 --- a/lib/rubocop/cop/performance/big_decimal_with_numeric_argument.rb +++ b/lib/rubocop/cop/performance/big_decimal_with_numeric_argument.rb @@ -3,22 +3,28 @@ module RuboCop module Cop module Performance - # Identifies places where string argument to `BigDecimal` should be - # converted to numeric. Initializing from Integer is faster - # than from String for BigDecimal. + # Identifies places where a float argument to BigDecimal should be converted to a string. + # Initializing from String is faster than from Float for BigDecimal. + # + # Also identifies places where an integer string argument to BigDecimal should be converted to + # an integer. Initializing from Integer is faster than from String for BigDecimal. # # @example # # bad - # BigDecimal('1', 2) - # BigDecimal('4', 6) + # BigDecimal(1.2, 3, exception: true) + # 4.5.to_d(6, exception: true) + # + # # good # BigDecimal('1.2', 3, exception: true) # BigDecimal('4.5', 6, exception: true) # + # # bad + # BigDecimal('1', 2) + # BigDecimal('4', 6) + # # # good # BigDecimal(1, 2) # 4.to_d(6) - # BigDecimal(1.2, 3, exception: true) - # 4.5.to_d(6, exception: true) # class BigDecimalWithNumericArgument < Base extend AutoCorrector @@ -26,30 +32,45 @@ class BigDecimalWithNumericArgument < Base minimum_target_ruby_version 3.1 - MSG = 'Convert string literal to numeric and pass it to `BigDecimal`.' + MSG_FROM_FLOAT_TO_STRING = 'Convert float literal to string and pass it to `BigDecimal`.' + MSG_FROM_INTEGER_TO_STRING = 'Convert string literal to integer and pass it to `BigDecimal`.' RESTRICT_ON_SEND = %i[BigDecimal to_d].freeze - def_node_matcher :big_decimal_with_numeric_argument?, <<~PATTERN - (send nil? :BigDecimal $str_type? ...) + def_node_matcher :big_decimal_with_numeric_argument, <<~PATTERN + (send nil? :BigDecimal ${float_type? str_type?} ...) PATTERN - def_node_matcher :to_d?, <<~PATTERN - (send [!nil? $str_type?] :to_d ...) + def_node_matcher :to_d, <<~PATTERN + (send [!nil? ${float_type? str_type?}] :to_d ...) PATTERN + # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength def on_send(node) - if (string = big_decimal_with_numeric_argument?(node)) - add_offense(string.source_range) do |corrector| - corrector.replace(string, string.value) + if (numeric = big_decimal_with_numeric_argument(node)) + if numeric.numeric_type? + add_offense(numeric, message: MSG_FROM_FLOAT_TO_STRING) do |corrector| + corrector.wrap(numeric, "'", "'") + end + elsif numeric.value.match?(/\A\d+\z/) + add_offense(numeric, message: MSG_FROM_INTEGER_TO_STRING) do |corrector| + corrector.replace(numeric, numeric.value) + end end - elsif (string_to_d = to_d?(node)) - add_offense(string_to_d.source_range) do |corrector| - big_decimal_args = node.arguments.map(&:source).unshift(string_to_d.value).join(', ') + elsif (numeric_to_d = to_d(node)) + if numeric_to_d.numeric_type? + add_offense(numeric_to_d, message: MSG_FROM_FLOAT_TO_STRING) do |corrector| + big_decimal_args = node.arguments.map(&:source).unshift("'#{numeric_to_d.source}'").join(', ') - corrector.replace(node, "BigDecimal(#{big_decimal_args})") + corrector.replace(node, "BigDecimal(#{big_decimal_args})") + end + elsif numeric_to_d.value.match?(/\A\d+\z/) + add_offense(numeric_to_d, message: MSG_FROM_INTEGER_TO_STRING) do |corrector| + corrector.replace(node, "#{numeric_to_d.value}.to_d") + end end end end + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength end end end diff --git a/spec/rubocop/cop/performance/big_decimal_with_numeric_argument_spec.rb b/spec/rubocop/cop/performance/big_decimal_with_numeric_argument_spec.rb index e15247db6b..fc9a691c9f 100644 --- a/spec/rubocop/cop/performance/big_decimal_with_numeric_argument_spec.rb +++ b/spec/rubocop/cop/performance/big_decimal_with_numeric_argument_spec.rb @@ -2,128 +2,128 @@ RSpec.describe RuboCop::Cop::Performance::BigDecimalWithNumericArgument, :config do context 'when Ruby >= 3.1', :ruby31 do - it 'registers an offense and corrects when using `BigDecimal` with string' do - expect_offense(<<~RUBY) - BigDecimal('1') - ^^^ Convert string literal to numeric and pass it to `BigDecimal`. + it 'does not register an offense and corrects when using `BigDecimal` with integer' do + expect_no_offenses(<<~RUBY) + BigDecimal(1) RUBY + end - expect_correction(<<~RUBY) - BigDecimal(1) + it 'does not register an offense and corrects when using `Integer#to_d` for integer' do + expect_no_offenses(<<~RUBY) + 1.to_d RUBY end - it 'registers an offense and corrects when using `String#to_d`' do + it 'registers an offense and corrects when using `BigDecimal` with float' do expect_offense(<<~RUBY) - '1'.to_d - ^^^ Convert string literal to numeric and pass it to `BigDecimal`. + BigDecimal(1.5, exception: true) + ^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(1) + BigDecimal('1.5', exception: true) RUBY end - it 'registers an offense and corrects when using `BigDecimal` with float string' do + it 'registers an offense and corrects when using `Float#to_d`' do expect_offense(<<~RUBY) - BigDecimal('1.5', exception: true) - ^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + 1.5.to_d(exception: true) + ^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(1.5, exception: true) + BigDecimal('1.5', exception: true) RUBY end - it 'registers an offense and corrects when using float `String#to_d`' do + it 'registers an offense when using `BigDecimal` with float and precision' do expect_offense(<<~RUBY) - '1.5'.to_d(exception: true) - ^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + BigDecimal(3.14, 1) + ^^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(1.5, exception: true) + BigDecimal('3.14', 1) RUBY end - it 'registers an offense when using `BigDecimal` with float string and precision' do + it 'registers an offense when using `Float#to_d` with precision' do expect_offense(<<~RUBY) - BigDecimal('3.14', 1) - ^^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + 3.14.to_d(1) + ^^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(3.14, 1) + BigDecimal('3.14', 1) RUBY end - it 'registers an offense when using float `String#to_d` with precision' do + it 'registers an offense when using `BigDecimal` with float and non-literal precision' do expect_offense(<<~RUBY) - '3.14'.to_d(1) - ^^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + precision = 1 + BigDecimal(3.14, precision) + ^^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(3.14, 1) + precision = 1 + BigDecimal('3.14', precision) RUBY end - it 'registers an offense when using `BigDecimal` with float string and non-literal precision' do + it 'registers an offense when using `Float#to_d` with non-literal precision' do expect_offense(<<~RUBY) precision = 1 - BigDecimal('3.14', precision) - ^^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + 3.14.to_d(precision) + ^^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) precision = 1 - BigDecimal(3.14, precision) + BigDecimal('3.14', precision) RUBY end - it 'registers an offense when using float `String#to_d` with non-literal precision' do + it 'registers an offense when using `BigDecimal` with float, precision, and a keyword argument' do expect_offense(<<~RUBY) - precision = 1 - '3.14'.to_d(precision) - ^^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + BigDecimal(3.14, 1, exception: true) + ^^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - precision = 1 - BigDecimal(3.14, precision) + BigDecimal('3.14', 1, exception: true) RUBY end - it 'registers an offense when using `BigDecimal` with float string, precision, and a keyword argument' do + it 'registers an offense when using `Float#to_d` with precision and a keyword argument' do expect_offense(<<~RUBY) - BigDecimal('3.14', 1, exception: true) - ^^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + 3.14.to_d(1, exception: true) + ^^^^ Convert float literal to string and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(3.14, 1, exception: true) + BigDecimal('3.14', 1, exception: true) RUBY end - it 'registers an offense when using float `String#to_d` with precision and a keyword argument' do + it 'registers an offense when using `BigDecimal` with integer string' do expect_offense(<<~RUBY) - '3.14'.to_d(1, exception: true) - ^^^^^^ Convert string literal to numeric and pass it to `BigDecimal`. + BigDecimal('1') + ^^^ Convert string literal to integer and pass it to `BigDecimal`. RUBY expect_correction(<<~RUBY) - BigDecimal(3.14, 1, exception: true) + BigDecimal(1) RUBY end - it 'does not register an offense when using `BigDecimal` with integer' do - expect_no_offenses(<<~RUBY) - BigDecimal(1) + it 'registers an offense when using `String#to_d` for integer string' do + expect_offense(<<~RUBY) + '1'.to_d + ^^^ Convert string literal to integer and pass it to `BigDecimal`. RUBY - end - it 'does not register an offense when using `Integer#to_d`' do - expect_no_offenses(<<~RUBY) + expect_correction(<<~RUBY) 1.to_d RUBY end