Skip to content

Commit

Permalink
[Fix #468] Fix false positives for `Performance/BigDecimalWithNumeric…
Browse files Browse the repository at this point in the history 
…Argument`

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
```
  • Loading branch information
@koic
koic committed Sep 17, 2024
1 parent f74a890 commit 6bb06b2
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 69 deletions.
1 change: 1 addition & 0 deletions changelog/fix_false_positives_for_performance_big_decimal_with_numeric_argument.md
View file
Original file line number Diff line number Diff line change
@@ -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][])
59 changes: 40 additions & 19 deletions lib/rubocop/cop/performance/big_decimal_with_numeric_argument.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,53 +3,74 @@
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
extend TargetRubyVersion

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
Expand Down
100 changes: 50 additions & 50 deletions spec/rubocop/cop/performance/big_decimal_with_numeric_argument_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 6bb06b2

Please sign in to comment.