From 32b98390a2b471aaf7b81c56b6ae59e3d3c064f2 Mon Sep 17 00:00:00 2001 From: Bart de Water Date: Fri, 22 Jul 2022 07:48:59 -0400 Subject: [PATCH] Add new Rails/ActiveSupportOnLoad cop. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This cop is extracted from Shopify's internal Rubocop repository. Many thanks to the original authors for their work. Co-authored-by: Julian Nadeau Co-authored-by: Rafael Mendonça França Co-authored-by: Francois Chagnon Co-authored-by: Jean Boussier --- .../new_add_railsactivesupportonload_cop.md | 1 + config/default.yml | 9 ++ .../cop/rails/active_support_on_load.rb | 70 ++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + .../cop/rails/active_support_on_load_spec.rb | 96 +++++++++++++++++++ 5 files changed, 177 insertions(+) create mode 100644 changelog/new_add_railsactivesupportonload_cop.md create mode 100644 lib/rubocop/cop/rails/active_support_on_load.rb create mode 100644 spec/rubocop/cop/rails/active_support_on_load_spec.rb diff --git a/changelog/new_add_railsactivesupportonload_cop.md b/changelog/new_add_railsactivesupportonload_cop.md new file mode 100644 index 0000000000..1d2b94afc0 --- /dev/null +++ b/changelog/new_add_railsactivesupportonload_cop.md @@ -0,0 +1 @@ +* [#749](https://github.com/rubocop/rubocop-rails/pull/749): Add new `Rails/ActiveSupportOnLoad` cop. ([@bdewater][]) diff --git a/config/default.yml b/config/default.yml index 286029a31a..e413158507 100644 --- a/config/default.yml +++ b/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: '<>' + Rails/AddColumnIndex: Description: >- Rails migrations don't make use of a given `index` key, but also diff --git a/lib/rubocop/cop/rails/active_support_on_load.rb b/lib/rubocop/cop/rails/active_support_on_load.rb new file mode 100644 index 0000000000..1bb9f50d2b --- /dev/null +++ b/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 `%s` instead of `%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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index b8c7612879..8e0e555cc4 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rails/active_support_on_load_spec.rb b/spec/rubocop/cop/rails/active_support_on_load_spec.rb new file mode 100644 index 0000000000..0d4adeb01a --- /dev/null +++ b/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