Merge pull request #754 from koic/make_redundant_receiver_in_with_options_and_reversible_migration_aware_of_numblock

Make `Rails/RedundantReceiverInWithOptions` and `Rails/ReversibleMigration` aware of numblock

Author: koic

.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

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][])

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

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
 

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)

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)

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)

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

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