Register new offenses for

CHANGELOG.md

@@ -46,6 +46,7 @@
 * [#263](https://github.com/rubocop-hq/rubocop-rails/pull/263): Change terminology to `ForbiddenMethods` and `AllowedMethods`. ([@jcoyne][])
 * [#289](https://github.com/rubocop-hq/rubocop-rails/pull/289): Update `Rails/SkipsModelValidations` to register an offense for `insert_all`, `touch_all`, `upsert_all`, etc. ([@eugeneius][])
 * [#293](https://github.com/rubocop-hq/rubocop-rails/pull/293): Require RuboCop 0.87 or higher. ([@koic][])
+* [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][])
 
 ## 2.6.0 (2020-06-08)
 
@@ -247,3 +248,4 @@
 [@jcoyne]: https://github.com/jcoyne
 [@fatkodima]: https://github.com/fatkodima
 [@taylorthurlow]: https://github.com/taylorthurlow
+[@philcoggins]: https://github.com/philcoggins

docs/modules/ROOT/pages/cops_rails.adoc

@@ -3379,6 +3379,55 @@ def change
 end
 ----
 
+[source,ruby]
+----
+# remove_columns
+
+# bad
+def change
+  remove_columns :users, :name, :email
+end
+
+# good
+def change
+  reversible do |dir|
+    dir.up do
+      remove_columns :users, :name, :email
+    end
+
+    dir.down do
+      add_column :users, :name, :string
+      add_column :users, :name, :email
+    end
+  end
+end
+
+# good (Rails >= 6.1, see https://github.com/rails/rails/pull/36589)
+def change
+  remove_columns :users, :name, :email, type: :string
+end
+----
+
+[source,ruby]
+----
+# remove_index
+
+# bad
+def change
+  remove_index :users, name: :index_users_on_email
+end
+
+# good
+def change
+  remove_index :users, :email
+end
+
+# good
+def change
+  remove_index :users, column: :email
+end
+----
+
 === Configurable attributes
 
 |===

lib/rubocop/cop/rails/reversible_migration.rb

@@ -129,6 +129,51 @@ module Rails
       #     end
       #   end
       #
+      # @example
+      #   # remove_columns
+      #
+      #   # bad
+      #   def change
+      #     remove_columns :users, :name, :email
+      #   end
+      #
+      #   # good
+      #   def change
+      #     reversible do |dir|
+      #       dir.up do
+      #         remove_columns :users, :name, :email
+      #       end
+      #
+      #       dir.down do
+      #         add_column :users, :name, :string
+      #         add_column :users, :name, :email
+      #       end
+      #     end
+      #   end
+      #
+      #   # good (Rails >= 6.1, see https://github.com/rails/rails/pull/36589)
+      #   def change
+      #     remove_columns :users, :name, :email, type: :string
+      #   end
+      #
+      # @example
+      #   # remove_index
+      #
+      #   # bad
+      #   def change
+      #     remove_index :users, name: :index_users_on_email
+      #   end
+      #
+      #   # good
+      #   def change
+      #     remove_index :users, :email
+      #   end
+      #
+      #   # good
+      #   def change
+      #     remove_index :users, column: :email
+      #   end
+      #
       # @see https://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html
       class ReversibleMigration < Cop
         MSG = '%<action>s is not reversible.'
@@ -153,6 +198,14 @@ class ReversibleMigration < Cop
           (send nil? :change_table $_ ...)
         PATTERN
 
+        def_node_matcher :remove_columns_call, <<~PATTERN
+          (send nil? :remove_columns ... $_)
+        PATTERN
+
+        def_node_matcher :remove_index_call, <<~PATTERN
+          (send nil? :remove_index _ $_)
+        PATTERN
+
         def on_send(node)
           return unless within_change_method?(node)
           return if within_reversible_or_up_only_block?(node)
@@ -162,6 +215,8 @@ def on_send(node)
           check_reversible_hash_node(node)
           check_remove_column_node(node)
           check_remove_foreign_key_node(node)
+          check_remove_columns_node(node)
+          check_remove_index_node(node)
         end
 
         def on_block(node)
@@ -237,6 +292,30 @@ def check_change_table_node(node, block)
           end
         end
 
+        def check_remove_columns_node(node)
+          remove_columns_call(node) do |args|
+            unless all_hash_key?(args, :type)
+              action = target_rails_version >= 6.1 ? 'remove_columns(without type)' : 'remove_columns'
+
+              add_offense(
+                node,
+                message: format(MSG, action: action)
+              )
+            end
+          end
+        end
+
+        def check_remove_index_node(node)
+          remove_index_call(node) do |args|
+            if args.hash_type? && !all_hash_key?(args, :column)
+              add_offense(
+                node,
+                message: format(MSG, action: 'remove_index(without column)')
+              )
+            end
+          end
+        end
+
         def check_change_table_offense(receiver, node)
           method_name = node.method_name
           return if receiver != node.receiver &&

spec/rubocop/cop/rails/reversible_migration_spec.rb

@@ -212,4 +212,28 @@ def change
       end
     RUBY
   end
+
+  context 'remove_columns' do
+    it_behaves_like 'accepts', 'remove_columns (with type)', <<~RUBY
+      remove_columns(:posts, :title, :body, type: :text)
+    RUBY
+
+    it_behaves_like 'offense', 'remove_columns', <<~RUBY
+      remove_columns(:posts, :title, :body)
+    RUBY
+  end
+
+  context 'remove_index' do
+    it_behaves_like 'accepts', 'remove_index (with column)', <<~RUBY
+      remove_index(:posts, column: :body)
+    RUBY
+
+    it_behaves_like 'accepts', 'remove_index (with two args)', <<~RUBY
+      remove_index(:posts, :body)
+    RUBY
+
+    it_behaves_like 'offense', 'remove_index(without column)', <<~RUBY
+      remove_index(:posts, name: :index_columns_on_body)
+    RUBY
+  end
 end