From 62ebebed58c0fb22febe83125d1f5e8a0d2ff4b9 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 16 Aug 2022 12:21:36 +0900 Subject: [PATCH] Make `Rails/RedundantReceiverInWithOptions` and `Rails/ReversibleMigration` aware of numblock This PR Make `Rails/RedundantReceiverInWithOptions` and `Rails/ReversibleMigration` cops aware of numbered block parameter. `Rails/ActionFilter` and `Rails/RakeEnvironment` cops have block arguments, so numblock operation is unnecessary. `Rails/IndexBy` and `Rails/IndexWith` cops is a slightly more complicated operation using multiple block arguments. It'll take care of it another time using `rubocop:todo` comment because I think it's a really rare case. --- .rubocop_todo.yml | 3 -- ..._reversible_migration_aware_of_numblock.md | 1 + lib/rubocop/cop/mixin/index_method.rb | 2 +- lib/rubocop/cop/rails/action_filter.rb | 2 +- lib/rubocop/cop/rails/rake_environment.rb | 2 +- .../redundant_receiver_in_with_options.rb | 54 ++++++++++--------- lib/rubocop/cop/rails/reversible_migration.rb | 2 + ...redundant_receiver_in_with_options_spec.rb | 30 +++++++++++ .../cop/rails/reversible_migration_spec.rb | 16 ++++++ 9 files changed, 80 insertions(+), 32 deletions(-) create mode 100644 changelog/fix_make_redundant_receiver_in_with_options_and_reversible_migration_aware_of_numblock.md diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index d521253047..87f4e3912f 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -14,9 +14,6 @@ InternalAffairs/NodeDestructuring: - 'lib/rubocop/cop/rails/relative_date_constant.rb' - 'lib/rubocop/cop/rails/time_zone.rb' -InternalAffairs/NumblockHandler: - Enabled: false - # Offense count: 10 Metrics/AbcSize: Max: 17 diff --git a/changelog/fix_make_redundant_receiver_in_with_options_and_reversible_migration_aware_of_numblock.md b/changelog/fix_make_redundant_receiver_in_with_options_and_reversible_migration_aware_of_numblock.md new file mode 100644 index 0000000000..be71cb7cc5 --- /dev/null +++ b/changelog/fix_make_redundant_receiver_in_with_options_and_reversible_migration_aware_of_numblock.md @@ -0,0 +1 @@ +* [#754](https://github.com/rubocop/rubocop-rails/pull/754): Make `Rails/RedundantReceiverInWithOptions` and `Rails/ReversibleMigration` cops aware of numbered block parameter. ([@koic][]) diff --git a/lib/rubocop/cop/mixin/index_method.rb b/lib/rubocop/cop/mixin/index_method.rb index bf864ad554..5cc2009486 100644 --- a/lib/rubocop/cop/mixin/index_method.rb +++ b/lib/rubocop/cop/mixin/index_method.rb @@ -6,7 +6,7 @@ module Cop module IndexMethod # rubocop:disable Metrics/ModuleLength RESTRICT_ON_SEND = %i[each_with_object to_h map collect []].freeze - def on_block(node) + def on_block(node) # rubocop:todo InternalAffairs/NumblockHandler on_bad_each_with_object(node) do |*match| handle_possible_offense(node, match, 'each_with_object') end diff --git a/lib/rubocop/cop/rails/action_filter.rb b/lib/rubocop/cop/rails/action_filter.rb index 83db0ade84..e144568e8a 100644 --- a/lib/rubocop/cop/rails/action_filter.rb +++ b/lib/rubocop/cop/rails/action_filter.rb @@ -69,7 +69,7 @@ class ActionFilter < Base RESTRICT_ON_SEND = FILTER_METHODS + ACTION_METHODS - def on_block(node) + def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler check_method_node(node.send_node) end diff --git a/lib/rubocop/cop/rails/rake_environment.rb b/lib/rubocop/cop/rails/rake_environment.rb index fdb6f76924..8772b2682c 100644 --- a/lib/rubocop/cop/rails/rake_environment.rb +++ b/lib/rubocop/cop/rails/rake_environment.rb @@ -39,7 +39,7 @@ class RakeEnvironment < Base (block $(send nil? :task ...) ...) PATTERN - def on_block(node) + def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler task_definition?(node) do |task_method| return if task_name(task_method) == :default return if with_dependencies?(task_method) diff --git a/lib/rubocop/cop/rails/redundant_receiver_in_with_options.rb b/lib/rubocop/cop/rails/redundant_receiver_in_with_options.rb index f289e76217..6a6b1e4827 100644 --- a/lib/rubocop/cop/rails/redundant_receiver_in_with_options.rb +++ b/lib/rubocop/cop/rails/redundant_receiver_in_with_options.rb @@ -60,15 +60,6 @@ class RedundantReceiverInWithOptions < Base MSG = 'Redundant receiver in `with_options`.' - def_node_matcher :with_options?, <<~PATTERN - (block - (send nil? :with_options - (...)) - (args - $_arg) - $_body) - PATTERN - def_node_search :all_block_nodes_in, <<~PATTERN (block ...) PATTERN @@ -78,29 +69,40 @@ class RedundantReceiverInWithOptions < Base PATTERN def on_block(node) - with_options?(node) do |arg, body| - return if body.nil? - return unless all_block_nodes_in(body).count.zero? - - send_nodes = all_send_nodes_in(body) - - if send_nodes.all? { |n| same_value?(arg, n.receiver) } - send_nodes.each do |send_node| - receiver = send_node.receiver - add_offense(receiver.source_range) do |corrector| - autocorrect(corrector, send_node) - end - end + return unless node.method?(:with_options) + return unless (body = node.body) + return unless all_block_nodes_in(body).count.zero? + + send_nodes = all_send_nodes_in(body) + return unless redundant_receiver?(send_nodes, node) + + send_nodes.each do |send_node| + receiver = send_node.receiver + add_offense(receiver.source_range) do |corrector| + autocorrect(corrector, send_node, node) end end end + alias on_numblock on_block + private - def autocorrect(corrector, node) - corrector.remove(node.receiver.source_range) - corrector.remove(node.loc.dot) - corrector.remove(block_argument_range(node)) + def autocorrect(corrector, send_node, node) + corrector.remove(send_node.receiver.source_range) + corrector.remove(send_node.loc.dot) + corrector.remove(block_argument_range(send_node)) unless node.numblock_type? + end + + def redundant_receiver?(send_nodes, node) + proc = if node.numblock_type? + ->(n) { n.receiver.lvar_type? && n.receiver.source == '_1' } + else + arg = node.arguments.first + ->(n) { same_value?(arg, n.receiver) } + end + + send_nodes.all?(&proc) end def block_argument_range(node) diff --git a/lib/rubocop/cop/rails/reversible_migration.rb b/lib/rubocop/cop/rails/reversible_migration.rb index 30f28b6584..5af544b000 100644 --- a/lib/rubocop/cop/rails/reversible_migration.rb +++ b/lib/rubocop/cop/rails/reversible_migration.rb @@ -229,6 +229,8 @@ def on_block(node) check_change_table_node(node.send_node, node.body) end + alias on_numblock on_block + private def check_irreversible_schema_statement_node(node) diff --git a/spec/rubocop/cop/rails/redundant_receiver_in_with_options_spec.rb b/spec/rubocop/cop/rails/redundant_receiver_in_with_options_spec.rb index 377465c879..951d0646d6 100644 --- a/spec/rubocop/cop/rails/redundant_receiver_in_with_options_spec.rb +++ b/spec/rubocop/cop/rails/redundant_receiver_in_with_options_spec.rb @@ -29,6 +29,36 @@ class Account < ApplicationRecord RUBY end + context 'Ruby >= 2.7', :ruby27 do + it 'registers an offense and corrects using explicit receiver in `with_options`' do + expect_offense(<<~RUBY) + class Account < ApplicationRecord + with_options dependent: :destroy do + _1.has_many :customers + ^^ Redundant receiver in `with_options`. + _1.has_many :products + ^^ Redundant receiver in `with_options`. + _1.has_many :invoices + ^^ Redundant receiver in `with_options`. + _1.has_many :expenses + ^^ Redundant receiver in `with_options`. + end + end + RUBY + + expect_correction(<<~RUBY) + class Account < ApplicationRecord + with_options dependent: :destroy do + has_many :customers + has_many :products + has_many :invoices + has_many :expenses + end + end + RUBY + end + end + it 'does not register an offense when using implicit receiver in `with_options`' do expect_no_offenses(<<~RUBY) class Account < ApplicationRecord diff --git a/spec/rubocop/cop/rails/reversible_migration_spec.rb b/spec/rubocop/cop/rails/reversible_migration_spec.rb index b5fb7a1f47..52a3677de5 100644 --- a/spec/rubocop/cop/rails/reversible_migration_spec.rb +++ b/spec/rubocop/cop/rails/reversible_migration_spec.rb @@ -35,6 +35,14 @@ def change end RUBY + context 'Ruby >= 2.7', :ruby27 do + it_behaves_like 'accepts', 'create_table using numbered parameter', <<~RUBY + create_table :users do + _1.string :name + end + RUBY + end + it_behaves_like 'offense', 'execute', <<~RUBY execute "ALTER TABLE `pages_linked_pages` ADD UNIQUE `page_id_linked_page_id` (`page_id`,`linked_page_id`)" RUBY @@ -227,6 +235,14 @@ def change end RUBY + context 'Ruby >= 2.7', :ruby27 do + it_behaves_like 'offense', 'change_table(with change_default)', <<~RUBY + change_table :users do + _1.change_default :authorized, 1 + end + RUBY + end + context 'remove' do context 'Rails >= 6.1', :rails61 do it_behaves_like 'accepts', 't.remove (with type)', <<~RUBY