[Fix #8950] Add IgnoredMethods and IgnoredClasses to `Lint/Number…

…Conversion` (#8956)

- Updated the hardcoded classes to ignore to be configurable instead

changelog/change_add_ignoredmethods_and_ignoredclasses_to.md

@@ -0,0 +1 @@
+* [#8950](https://github.com/rubocop-hq/rubocop/issues/8950): Add `IgnoredMethods` and `IgnoredClasses` to `Lint/NumberConversion`. ([@dvandersluis][])

config/default.yml

@@ -1694,8 +1694,12 @@ Lint/NumberConversion:
   Description: 'Checks unsafe usage of number conversion methods.'
   Enabled: false
   VersionAdded: '0.53'
-  VersionChanged: '0.70'
+  VersionChanged: '1.1'
   SafeAutoCorrect: false
+  IgnoredMethods: []
+  IgnoredClasses:
+    - Time
+    - DateTime
 
 Lint/OrderedMagicComments:
   Description: 'Checks the proper ordering of magic comments and whether a magic comment is not placed before a shebang.'

docs/modules/ROOT/pages/cops_lint.adoc

@@ -2611,13 +2611,24 @@ end
 | Yes
 | Yes (Unsafe)
 | 0.53
-| 0.70
+| 1.1
 |===
 
 This cop warns the usage of unsafe number conversions. Unsafe
 number conversion can cause unexpected error if auto type conversion
 fails. Cop prefer parsing with number class instead.
 
+Conversion with `Integer`, `Float`, etc. will raise an `ArgumentError`
+if given input that is not numeric (eg. an empty string), whereas
+`to_i`, etc. will try to convert regardless of input (`''.to_i => 0`).
+As such, this cop is disabled by default because it's not necessarily
+always correct to raise if a value is not numeric.
+
+NOTE: Some values cannot be converted properly using one of the `Kernel`
+method (for instance, `Time` and `DateTime` values are allowed by this
+cop by default). Similarly, Rails' duration methods do not work well
+with `Integer()` and can be ignored with `IgnoredMethods`.
+
 === Examples
 
 [source,ruby]
@@ -2635,6 +2646,36 @@ Float('10.2')
 Complex('10')
 ----
 
+==== IgnoredMethods: [minutes]
+
+[source,ruby]
+----
+# good
+10.minutes.to_i
+----
+
+==== IgnoredClasses: [Time, DateTime] (default)
+
+[source,ruby]
+----
+# good
+Time.now.to_datetime.to_i
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| IgnoredMethods
+| `[]`
+| Array
+
+| IgnoredClasses
+| `Time`, `DateTime`
+| Array
+|===
+
 == Lint/OrderedMagicComments
 
 |===

lib/rubocop/cop/lint/number_conversion.rb

@@ -7,6 +7,17 @@ module Lint
       # number conversion can cause unexpected error if auto type conversion
       # fails. Cop prefer parsing with number class instead.
       #
+      # Conversion with `Integer`, `Float`, etc. will raise an `ArgumentError`
+      # if given input that is not numeric (eg. an empty string), whereas
+      # `to_i`, etc. will try to convert regardless of input (`''.to_i => 0`).
+      # As such, this cop is disabled by default because it's not necessarily
+      # always correct to raise if a value is not numeric.
+      #
+      # NOTE: Some values cannot be converted properly using one of the `Kernel`
+      # method (for instance, `Time` and `DateTime` values are allowed by this
+      # cop by default). Similarly, Rails' duration methods do not work well
+      # with `Integer()` and can be ignored with `IgnoredMethods`.
+      #
       # @example
       #
       #   # bad
@@ -20,8 +31,19 @@ module Lint
       #   Integer('10', 10)
       #   Float('10.2')
       #   Complex('10')
+      #
+      # @example IgnoredMethods: [minutes]
+      #
+      #   # good
+      #   10.minutes.to_i
+      #
+      # @example IgnoredClasses: [Time, DateTime] (default)
+      #
+      #   # good
+      #   Time.now.to_datetime.to_i
       class NumberConversion < Base
         extend AutoCorrector
+        include IgnoredMethods
 
         CONVERSION_METHOD_CLASS_MAPPING = {
           to_i: "#{Integer.name}(%<number_object>s, 10)",
@@ -38,13 +60,9 @@ class NumberConversion < Base
           (send $_ ${:to_i :to_f :to_c})
         PATTERN
 
-        def_node_matcher :datetime?, <<~PATTERN
-          (send (const {nil? (cbase)} {:Time :DateTime}) ...)
-        PATTERN
-
         def on_send(node)
           to_method(node) do |receiver, to_method|
-            next if receiver.nil? || date_time_object?(receiver)
+            next if receiver.nil? || ignore_receiver?(receiver)
 
             message = format(
               MSG,
@@ -60,18 +78,33 @@ def on_send(node)
 
         private
 
-        def date_time_object?(node)
-          child = node
-          while child&.send_type?
-            return true if datetime? child
+        def correct_method(node, receiver)
+          format(CONVERSION_METHOD_CLASS_MAPPING[node.method_name],
+                 number_object: receiver.source)
+        end
 
-            child = child.children[0]
+        def ignore_receiver?(receiver)
+          if receiver.send_type? && ignored_method?(receiver.method_name)
+            true
+          elsif (receiver = top_receiver(receiver))
+            receiver.const_type? && ignored_class?(receiver.const_name)
+          else
+            false
           end
         end
 
-        def correct_method(node, receiver)
-          format(CONVERSION_METHOD_CLASS_MAPPING[node.method_name],
-                 number_object: receiver.source)
+        def top_receiver(node)
+          receiver = node
+          receiver = receiver.receiver until receiver.receiver.nil?
+          receiver
+        end
+
+        def ignored_classes
+          cop_config.fetch('IgnoredClasses', [])
+        end
+
+        def ignored_class?(name)
+          ignored_classes.include?(name.to_s)
         end
       end
     end

spec/rubocop/cop/lint/number_conversion_spec.rb

@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Lint::NumberConversion do
-  subject(:cop) { described_class.new(config) }
-
-  let(:config) { RuboCop::Config.new }
-
+RSpec.describe RuboCop::Cop::Lint::NumberConversion, :config do
   context 'registers an offense' do
     it 'when using `#to_i`' do
       expect_offense(<<~RUBY)
@@ -120,6 +116,16 @@
       RUBY
     end
 
+    it 'when `#to_i` called without a receiver' do
+      expect_no_offenses(<<~RUBY)
+        to_i
+      RUBY
+    end
+  end
+
+  context 'IgnoredClasses' do
+    let(:cop_config) { { 'IgnoredClasses' => %w[Time DateTime] } }
+
     it 'when using Time' do
       expect_no_offenses(<<~RUBY)
         Time.now.to_i
@@ -149,10 +155,21 @@
                 .to_i
       RUBY
     end
+  end
 
-    it 'when `#to_i` called without a receiver' do
+  context 'IgnoredMethods' do
+    let(:cop_config) { { 'IgnoredMethods' => %w[minutes] } }
+
+    it 'does not register an offense for an ignored method' do
       expect_no_offenses(<<~RUBY)
-        to_i
+        10.minutes.to_i
+      RUBY
+    end
+
+    it 'registers an offense for other methods' do
+      expect_offense(<<~RUBY)
+        10.hours.to_i
+        ^^^^^^^^^^^^^ Replace unsafe number conversion with number class parsing, instead of using 10.hours.to_i, use stricter Integer(10.hours, 10).
       RUBY
     end
   end