Register new offenses for

CHANGELOG.md

@@ -15,6 +15,7 @@
 ### Changes
 
 * [#312](https://github.com/rubocop-hq/rubocop-rails/pull/312): Mark `Rails/MailerName` as unsafe for auto-correct. ([@eugeneius][])
+* [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][])
 
 ## 2.7.1 (2020-07-26)
 
@@ -258,3 +259,4 @@
 [@fatkodima]: https://github.com/fatkodima
 [@taylorthurlow]: https://github.com/taylorthurlow
 [@krim]: https://github.com/krim
+[@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, :email, :string
+      #       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) && target_rails_version >= 6.1
+              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,40 @@ def change
       end
     RUBY
   end
+
+  context 'remove_columns' do
+    context 'Rails >= 6.1', :rails61 do
+      it_behaves_like 'accepts', 'remove_columns(with type)', <<~RUBY
+        remove_columns(:posts, :title, :body, type: :text)
+      RUBY
+
+      it_behaves_like 'offense', 'remove_columns(without type)', <<~RUBY
+        remove_columns(:posts, :title, :body)
+      RUBY
+    end
+
+    context 'Rails < 6.1', :rails60 do
+      it_behaves_like 'offense', 'remove_columns', <<~RUBY
+        remove_columns(:posts, :title, :body, type: :text)
+      RUBY
+    end
+
+    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