diff --git a/changelog/change_add_ignoredmethods_and_ignoredclasses_to.md b/changelog/change_add_ignoredmethods_and_ignoredclasses_to.md new file mode 100644 index 00000000000..0e278990079 --- /dev/null +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 443d9a82fbd..cb8e3c16237 100644 --- a/config/default.yml +++ b/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.' diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index 376a10ebf63..4cc743c61c1 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/lint/number_conversion.rb b/lib/rubocop/cop/lint/number_conversion.rb index 25db90002b2..02d9c023971 100644 --- a/lib/rubocop/cop/lint/number_conversion.rb +++ b/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}(%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 diff --git a/spec/rubocop/cop/lint/number_conversion_spec.rb b/spec/rubocop/cop/lint/number_conversion_spec.rb index e3492ec8038..949d596a314 100644 --- a/spec/rubocop/cop/lint/number_conversion_spec.rb +++ b/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