Merge pull request #749 from bdewater/active-support-on-load

Add new Rails/ActiveSupportOnLoad cop.

changelog/new_add_railsactivesupportonload_cop.md

@@ -0,0 +1 @@
+* [#749](https://github.com/rubocop/rubocop-rails/pull/749): Add new `Rails/ActiveSupportOnLoad` cop. ([@bdewater][])

config/default.yml

@@ -129,6 +129,15 @@ Rails/ActiveSupportAliases:
   Enabled: true
   VersionAdded: '0.48'
 
+Rails/ActiveSupportOnLoad:
+  Description: 'Use `ActiveSupport.on_load(...)` to patch Rails framework classes.'
+  Enabled: 'pending'
+  Reference:
+    - 'https://api.rubyonrails.org/classes/ActiveSupport/LazyLoadHooks.html'
+    - 'https://guides.rubyonrails.org/engines.html#available-load-hooks'
+  SafeAutoCorrect: false
+  VersionAdded: '<<next>>'
+
 Rails/AddColumnIndex:
   Description: >-
     Rails migrations don't make use of a given `index` key, but also

lib/rubocop/cop/rails/active_support_on_load.rb

@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # Checks for Rails framework classes that are patched directly instead of using Active Support load hooks. Direct
+      # patching forcibly loads the framework referenced, using hooks defers loading until it's actually needed.
+      #
+      # @safety
+      #   While using lazy load hooks is recommended, it changes the order in which is code is loaded and may reveal
+      #   load order dependency bugs.
+      #
+      # @example
+      #
+      #   # bad
+      #   ActiveRecord::Base.include(MyClass)
+      #
+      #   # good
+      #   ActiveSupport.on_load(:active_record) { include MyClass }
+      class ActiveSupportOnLoad < Base
+        extend AutoCorrector
+
+        MSG = 'Use `%<prefer>s` instead of `%<current>s`.'
+        RESTRICT_ON_SEND = %i[prepend include extend].freeze
+        LOAD_HOOKS = {
+          'ActionCable' => 'action_cable',
+          'ActionCable::Channel::Base' => 'action_cable_channel',
+          'ActionCable::Connection::Base' => 'action_cable_connection',
+          'ActionCable::Connection::TestCase' => 'action_cable_connection_test_case',
+          'ActionController::API' => 'action_controller',
+          'ActionController::Base' => 'action_controller',
+          'ActionController::TestCase' => 'action_controller_test_case',
+          'ActionDispatch::IntegrationTest' => 'action_dispatch_integration_test',
+          'ActionDispatch::Request' => 'action_dispatch_request',
+          'ActionDispatch::Response' => 'action_dispatch_response',
+          'ActionDispatch::SystemTestCase' => 'action_dispatch_system_test_case',
+          'ActionMailbox::Base' => 'action_mailbox',
+          'ActionMailbox::InboundEmail' => 'action_mailbox_inbound_email',
+          'ActionMailbox::Record' => 'action_mailbox_record',
+          'ActionMailbox::TestCase' => 'action_mailbox_test_case',
+          'ActionMailer::Base' => 'action_mailer',
+          'ActionMailer::TestCase' => 'action_mailer_test_case',
+          'ActionText::Content' => 'action_text_content',
+          'ActionText::Record' => 'action_text_record',
+          'ActionText::RichText' => 'action_text_rich_text',
+          'ActionView::Base' => 'action_view',
+          'ActionView::TestCase' => 'action_view_test_case',
+          'ActiveJob::Base' => 'active_job',
+          'ActiveJob::TestCase' => 'active_job_test_case',
+          'ActiveRecord::Base' => 'active_record',
+          'ActiveStorage::Attachment' => 'active_storage_attachment',
+          'ActiveStorage::Blob' => 'active_storage_blob',
+          'ActiveStorage::Record' => 'active_storage_record',
+          'ActiveStorage::VariantRecord' => 'active_storage_variant_record',
+          'ActiveSupport::TestCase' => 'active_support_test_case'
+        }.freeze
+
+        def on_send(node)
+          receiver, method, arguments = *node # rubocop:disable InternalAffairs/NodeDestructuring
+          return unless receiver && (hook = LOAD_HOOKS[receiver.const_name])
+
+          preferred = "ActiveSupport.on_load(:#{hook}) { #{method} #{arguments.source} }"
+          add_offense(node, message: format(MSG, prefer: preferred, current: node.source)) do |corrector|
+            corrector.replace(node, preferred)
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -15,6 +15,7 @@
 require_relative 'rails/active_record_callbacks_order'
 require_relative 'rails/active_record_override'
 require_relative 'rails/active_support_aliases'
+require_relative 'rails/active_support_on_load'
 require_relative 'rails/add_column_index'
 require_relative 'rails/after_commit_override'
 require_relative 'rails/application_controller'

spec/rubocop/cop/rails/active_support_on_load_spec.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+RSpec.describe(RuboCop::Cop::Rails::ActiveSupportOnLoad, :config) do
+  it 'adds offense and corrects when trying to extend a framework class with include' do
+    expect_offense(<<~RUBY)
+      ActiveRecord::Base.include(MyClass)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `ActiveSupport.on_load(:active_record) { include MyClass }` instead of `ActiveRecord::Base.include(MyClass)`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      ActiveSupport.on_load(:active_record) { include MyClass }
+    RUBY
+  end
+
+  it 'adds offense and corrects when trying to extend a framework class with prepend' do
+    expect_offense(<<~RUBY)
+      ActiveRecord::Base.prepend(MyClass)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `ActiveSupport.on_load(:active_record) { prepend MyClass }` instead of `ActiveRecord::Base.prepend(MyClass)`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      ActiveSupport.on_load(:active_record) { prepend MyClass }
+    RUBY
+  end
+
+  it 'adds offense and corrects when trying to extend a framework class with extend' do
+    expect_offense(<<~RUBY)
+      ActiveRecord::Base.extend(MyClass)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `ActiveSupport.on_load(:active_record) { extend MyClass }` instead of `ActiveRecord::Base.extend(MyClass)`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      ActiveSupport.on_load(:active_record) { extend MyClass }
+    RUBY
+  end
+
+  it 'adds offense and corrects when trying to extend a framework class with absolute name' do
+    expect_offense(<<~RUBY)
+      ::ActiveRecord::Base.extend(MyClass)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `ActiveSupport.on_load(:active_record) { extend MyClass }` instead of `::ActiveRecord::Base.extend(MyClass)`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      ActiveSupport.on_load(:active_record) { extend MyClass }
+    RUBY
+  end
+
+  it 'adds offense and corrects when trying to extend a framework class with a variable' do
+    expect_offense(<<~RUBY)
+      ActiveRecord::Base.extend(my_class)
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `ActiveSupport.on_load(:active_record) { extend my_class }` instead of `ActiveRecord::Base.extend(my_class)`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      ActiveSupport.on_load(:active_record) { extend my_class }
+    RUBY
+  end
+
+  it 'does not add offense when extending a variable' do
+    expect_no_offenses(<<~RUBY)
+      foo.extend(MyClass)
+    RUBY
+  end
+
+  it 'does not add offense when extending the framework using on_load and include' do
+    expect_no_offenses(<<~RUBY)
+      ActiveSupport.on_load(:active_record) { include MyClass }
+    RUBY
+  end
+
+  it 'does not add offense when extending the framework using on_load and include in a multi-line block' do
+    expect_no_offenses(<<~RUBY)
+      ActiveSupport.on_load(:active_record) do
+        include MyClass
+      end
+    RUBY
+  end
+
+  it 'does not add offense when there is no extension on the supported classes' do
+    expect_no_offenses(<<~RUBY)
+      ActiveRecord::Base.include_root_in_json = false
+    RUBY
+  end
+
+  it 'does not add offense when using include?' do
+    expect_no_offenses(<<~RUBY)
+      name.include?('bob')
+    RUBY
+  end
+
+  it 'does not add offense on unsupported classes' do
+    expect_no_offenses(<<~RUBY)
+      MyClass1.prepend(MyClass)
+    RUBY
+  end
+end