Add cop to check ActiveModel errors manipulation as hash directly.

These are deprecated in Rails 6.1 and will be removed in Rails 7.
See rails/rails#32313 for details.

The cop acts in two modes:

For files under `/models` directory, any `errors` call,
whether with receiver or not, will be checked.
For general files, only `errors` calls with receivers will be checked.

E.g. `errors[:bar] = []` is without receiver.
It will record an offense if it is a model file.
It will not record an offense if it is other general fie.

This is to reduce false-positives,
since other classes may also have a `errors` method.

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### New features
 
 * [#486](https://github.com/rubocop/rubocop-rails/issues/486): Add new `Rails/ExpandedDateRange` cop. ([@koic][])
+* [#491](https://github.com/rubocop/rubocop-rails/pull/491): Add new `Rails/ActiveModelErrorsDirectManipulation` cop. ([@lulalala][])
 
 ### Bug fixes
 
@@ -394,3 +395,4 @@
 [@drenmi]: https://github.com/drenmi
 [@rhymes]: https://github.com/rhymes
 [@jdelStrother]: https://github.com/jdelStrother
+[@lulalala]: https://github.com/lulalala

config/default.yml

@@ -49,6 +49,12 @@ Rails/ActionFilter:
  Include:
  - app/controllers/**/*.rb
 
+Rails/ActiveModelErrorsDirectManipulation:
+ Description: 'Avoid manipulating ActiveModel errors hash directly.'
+ Enabled: true
+ Safe: false
+ VersionAdded: '2.11'
+
 Rails/ActiveRecordAliases:
  Description: >-
  Avoid Active Record aliases:

docs/modules/ROOT/pages/cops.adoc

@@ -17,6 +17,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide].
 === Department xref:cops_rails.adoc[Rails]
 
 * xref:cops_rails.adoc#railsactionfilter[Rails/ActionFilter]
+* xref:cops_rails.adoc#railsactivemodelerrorsdirectmanipulation[Rails/ActiveModelErrorsDirectManipulation]
 * xref:cops_rails.adoc#railsactiverecordaliases[Rails/ActiveRecordAliases]
 * xref:cops_rails.adoc#railsactiverecordcallbacksorder[Rails/ActiveRecordCallbacksOrder]
 * xref:cops_rails.adoc#railsactiverecordoverride[Rails/ActiveRecordOverride]

docs/modules/ROOT/pages/cops_rails.adoc

@@ -63,6 +63,40 @@ skip_after_filter :do_stuff
 | Array
 |===
 
+== Rails/ActiveModelErrorsDirectManipulation
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Disabled
+| Yes
+| No
+| 2.11
+| -
+|===
+
+This cop checks direct manipulation of ActiveModel#errors as hash.
+These operations are deprecated in Rails 6.1 and will not work in Rails 7.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+user.errors[:name] << 'msg'
+user.errors.messages[:name] << 'msg'
+
+# good
+user.errors.add(:name, 'msg')
+
+# bad
+user.errors[:name].clear
+user.errors.messages[:name].clear
+
+# good
+user.errors.delete(:name)
+----
+
 == Rails/ActiveRecordAliases
 
 |===

lib/rubocop/cop/rails/active_model_errors_direct_manipulation.rb

@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+module RuboCop
+ module Cop
+ module Rails
+ # This cop checks direct manipulation of ActiveModel#errors as hash.
+ # These operations are deprecated in Rails 6.1 and will not work in Rails 7.
+ #
+ # @example
+ # # bad
+ # user.errors[:name] << 'msg'
+ # user.errors.messages[:name] << 'msg'
+ #
+ # # good
+ # user.errors.add(:name, 'msg')
+ #
+ # # bad
+ # user.errors[:name].clear
+ # user.errors.messages[:name].clear
+ #
+ # # good
+ # user.errors.delete(:name)
+ #
+ class ActiveModelErrorsDirectManipulation < Base
+ MSG = 'Avoid manipulating ActiveModel errors as hash directly.'
+
+ MANIPULATIVE_METHODS = ':<< :append :clear :collect! :compact! :concat '\
+ ':delete :delete_at :delete_if :drop :drop_while :fill :filter! :keep_if '\
+ ':flatten! :insert :map! :pop :prepend :push :reject! :replace :reverse! '\
+ ':rotate! :select! :shift :shuffle! :slice! :sort! :sort_by! :uniq! :unshift'
+
+ BASE_NODE = {
+ general: '{send ivar lvar}',
+ model_file: '{nil? send ivar lvar}'
+ }.freeze
+
+ PATTERN = {
+ root_manipulation: <<~PATTERN,
+ (send
+ (send
+ (send %<base_node>s :errors)
+ :[]
+ ...)
+ {#{MANIPULATIVE_METHODS}}
+ ...)
+ PATTERN
+ root_assignment: <<~PATTERN,
+ (send
+ (send %<base_node>s :errors)
+ :[]=
+ ...)
+ PATTERN
+ messages_details_manipulation: <<~PATTERN,
+ (send
+ (send
+ (send
+ (send %<base_node>s :errors)
+ {:messages :details})
+ :[]
+ ...)
+ {#{MANIPULATIVE_METHODS}}
+ ...)
+ PATTERN
+ messages_details_assignment: <<~PATTERN
+ (send
+ (send
+ (send %<base_node>s :errors)
+ {:messages :details})
+ :[]=
+ ...)
+ PATTERN
+ }.freeze
+
+ BASE_NODE.each do |file_type, base_node_pattern|
+ PATTERN.each do |pattern_type, pattern|
+ node_matcher_name = "#{file_type}_on_#{pattern_type}?"
+
+ def_node_matcher(
+ node_matcher_name,
+ format(pattern, base_node: base_node_pattern)
+ )
+ end
+ end
+
+ def on_send(node)
+ file_type = file_type()
+
+ PATTERN.each do |pattern_type, _pattern|
+ add_offense(node) if send("#{file_type}_on_#{pattern_type}?", node)
+ end
+ end
+
+ private
+
+ def file_type
+ filename = File.expand_path(processed_source.buffer.name)
+ filename.include?('/models/') ? :model_file : :general
+ end
+ end
+ end
+ end
+end

lib/rubocop/cop/rails_cops.rb

@@ -6,6 +6,7 @@
 require_relative 'mixin/target_rails_version'
 
 require_relative 'rails/action_filter'
+require_relative 'rails/active_model_errors_direct_manipulation'
 require_relative 'rails/active_record_aliases'
 require_relative 'rails/active_record_callbacks_order'
 require_relative 'rails/active_record_override'

spec/rubocop/cop/rails/active_model_errors_direct_manipulation_spec.rb

@@ -0,0 +1,138 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::ActiveModelErrorsDirectManipulation, :config do
+ shared_examples 'errors call with explicit receiver' do
+ context 'when modifying errors' do
+ it 'registers an offense' do
+ expect_offense(<<~RUBY, file_path)
+ user.errors[:name] << 'msg'
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+
+ context 'when assigning' do
+ it 'registers an offense' do
+ expect_offense(<<~RUBY, file_path)
+ user.errors[:name] = []
+ ^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+ end
+ end
+
+ context 'when modifying errors.messages' do
+ it 'registers an offense' do
+ expect_offense(<<~RUBY, file_path)
+ user.errors.messages[:name] << 'msg'
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+
+ context 'when assigning' do
+ it 'registers an offense' do
+ expect_offense(<<~RUBY, file_path)
+ user.errors.messages[:name] = []
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+ end
+ end
+
+ context 'when modifying errors.details' do
+ it 'registers an offense' do
+ expect_offense(<<~RUBY, file_path)
+ user.errors.details[:name] << {}
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+
+ context 'when assigning' do
+ it 'registers an offense' do
+ expect_offense(<<~RUBY, file_path)
+ user.errors.details[:name] = []
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+ end
+ end
+ end
+
+ shared_examples 'errors call without explicit receiver' do
+ def expect_offense_if_model_file(code, file_path)
+ if file_path.include?('/models/')
+ expect_offense(code, file_path)
+ else
+ code = code.gsub(/^\^+ .+$/, '')
+ expect_no_offenses(code, file_path)
+ end
+ end
+
+ context 'when modifying errors' do
+ it 'registers an offense for model file' do
+ expect_offense_if_model_file(<<~RUBY, file_path)
+ errors[:name] << 'msg'
+ ^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+
+ context 'when assigning' do
+ it 'registers an offense' do
+ expect_offense_if_model_file(<<~RUBY, file_path)
+ errors[:name] = []
+ ^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+ end
+ end
+
+ context 'when modifying errors.messages' do
+ it 'registers an offense' do
+ expect_offense_if_model_file(<<~RUBY, file_path)
+ errors.messages[:name] << 'msg'
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+
+ context 'when assigning' do
+ it 'registers an offense' do
+ expect_offense_if_model_file(<<~RUBY, file_path)
+ errors.messages[:name] = []
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+ end
+ end
+
+ context 'when modifying errors.details' do
+ it 'registers an offense' do
+ expect_offense_if_model_file(<<~RUBY, file_path)
+ errors.details[:name] << {}
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+
+ context 'when assigning' do
+ it 'registers an offense' do
+ expect_offense_if_model_file(<<~RUBY, file_path)
+ errors.details[:name] = []
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating ActiveModel errors as hash directly.
+ RUBY
+ end
+ end
+ end
+ end
+
+ context 'when file is model file' do
+ let(:file_path) { '/foo/app/models/bar.rb' }
+
+ it_behaves_like 'errors call with explicit receiver'
+ it_behaves_like 'errors call without explicit receiver'
+ end
+
+ context 'when file is generic' do
+ let(:file_path) { '/foo/app/lib/bar.rb' }
+
+ it_behaves_like 'errors call with explicit receiver'
+ it_behaves_like 'errors call without explicit receiver'
+ end
+end