Support :: prefixed constants on some cops

r7kamura · r7kamura · commit 20042f602d02 · 2022-11-15T10:07:55.000+09:00
diff --git a/changelog/fix_support_prefixed_constants_on_some_cops.md b/changelog/fix_support_prefixed_constants_on_some_cops.md
@@ -0,0 +1 @@
+* [#868](https://github.com/rubocop/rubocop-rails/pull/868): Support `::` prefixed constants on `Rails/ActionControllerFlashBeforeRender`, `Rails/ActionControllerTestCase`, `Rails/ApplicationController`, `Rails/ApplicationJob`, `Rails/ApplicationMailer`, `Rails/ApplicationRecord`, `Rails/DotSeparatedKeys`, `Rails/DynamicFindBy`, `Rails/FindEach`, `Rails/FreezeTime`, `Rails/HasManyOrHasOneDependent`, `Rails/HelperInstanceVariable`, `Rails/MailerName`, `Rails/MigrationClassName`, `Rails/Output`, `Rails/ReversibleMigrationMethodDefinition`, `Rails/ReversibleMigration`, `Rails/ShortI18n`, `Rails/SkipsModelValidations`, and `Rails/TimeZoneAssignment`. ([@r7kamura][])
diff --git a/lib/rubocop/cop/mixin/active_record_helper.rb b/lib/rubocop/cop/mixin/active_record_helper.rb
@@ -10,8 +10,8 @@ module ActiveRecordHelper
 
       def_node_matcher :active_record?, <<~PATTERN
         {
-          (const nil? :ApplicationRecord)
-          (const (const nil? :ActiveRecord) :Base)
+          (const {nil? cbase} :ApplicationRecord)
+          (const (const {nil? cbase} :ActiveRecord) :Base)
         }
       PATTERN
 
diff --git a/lib/rubocop/cop/mixin/migrations_helper.rb b/lib/rubocop/cop/mixin/migrations_helper.rb
@@ -8,7 +8,7 @@ module MigrationsHelper
 
       def_node_matcher :migration_class?, <<~PATTERN
         (class
-          (const nil? _)
+          (const {nil? cbase} _)
           (send
             (const (const {nil? cbase} :ActiveRecord) :Migration)
             :[]
diff --git a/lib/rubocop/cop/rails/action_controller_flash_before_render.rb b/lib/rubocop/cop/rails/action_controller_flash_before_render.rb
@@ -43,8 +43,8 @@ class ActionControllerFlashBeforeRender < Base
 
         def_node_search :action_controller?, <<~PATTERN
           {
-            (const nil? :ApplicationController)
-            (const (const nil? :ActionController) :Base)
+            (const {nil? cbase} :ApplicationController)
+            (const (const {nil? cbase} :ActionController) :Base)
           }
         PATTERN
 
diff --git a/lib/rubocop/cop/rails/action_controller_test_case.rb b/lib/rubocop/cop/rails/action_controller_test_case.rb
@@ -30,7 +30,7 @@ class ActionControllerTestCase < Base
 
         def_node_matcher :action_controller_test_case?, <<~PATTERN
           (class
-            (const nil? _)
+            (const {nil? cbase} _)
             (const (const {nil? cbase} :ActionController) :TestCase) _)
         PATTERN
 
diff --git a/lib/rubocop/cop/rails/application_controller.rb b/lib/rubocop/cop/rails/application_controller.rb
@@ -25,7 +25,7 @@ class ApplicationController < Base
 
         MSG = 'Controllers should subclass `ApplicationController`.'
         SUPERCLASS = 'ApplicationController'
-        BASE_PATTERN = '(const (const nil? :ActionController) :Base)'
+        BASE_PATTERN = '(const (const {nil? cbase} :ActionController) :Base)'
 
         # rubocop:disable Layout/ClassStructure
         include RuboCop::Cop::EnforceSuperclass
diff --git a/lib/rubocop/cop/rails/application_job.rb b/lib/rubocop/cop/rails/application_job.rb
@@ -28,7 +28,7 @@ class ApplicationJob < Base
 
         MSG = 'Jobs should subclass `ApplicationJob`.'
         SUPERCLASS = 'ApplicationJob'
-        BASE_PATTERN = '(const (const nil? :ActiveJob) :Base)'
+        BASE_PATTERN = '(const (const {nil? cbase} :ActiveJob) :Base)'
 
         # rubocop:disable Layout/ClassStructure
         include RuboCop::Cop::EnforceSuperclass
diff --git a/lib/rubocop/cop/rails/application_mailer.rb b/lib/rubocop/cop/rails/application_mailer.rb
@@ -28,7 +28,7 @@ class ApplicationMailer < Base
 
         MSG = 'Mailers should subclass `ApplicationMailer`.'
         SUPERCLASS = 'ApplicationMailer'
-        BASE_PATTERN = '(const (const nil? :ActionMailer) :Base)'
+        BASE_PATTERN = '(const (const {nil? cbase} :ActionMailer) :Base)'
 
         # rubocop:disable Layout/ClassStructure
         include RuboCop::Cop::EnforceSuperclass
diff --git a/lib/rubocop/cop/rails/application_record.rb b/lib/rubocop/cop/rails/application_record.rb
@@ -29,7 +29,7 @@ class ApplicationRecord < Base
 
         MSG = 'Models should subclass `ApplicationRecord`.'
         SUPERCLASS = 'ApplicationRecord'
-        BASE_PATTERN = '(const (const nil? :ActiveRecord) :Base)'
+        BASE_PATTERN = '(const (const {nil? cbase} :ActiveRecord) :Base)'
 
         # rubocop:disable Layout/ClassStructure
         include RuboCop::Cop::EnforceSuperclass
diff --git a/lib/rubocop/cop/rails/dot_separated_keys.rb b/lib/rubocop/cop/rails/dot_separated_keys.rb
@@ -24,7 +24,7 @@ class DotSeparatedKeys < Base
         TRANSLATE_METHODS = %i[translate t].freeze
 
         def_node_matcher :translate_with_scope?, <<~PATTERN
-          (send {nil? (const nil? :I18n)} {:translate :t} ${sym_type? str_type?}
+          (send {nil? (const {nil? cbase} :I18n)} {:translate :t} ${sym_type? str_type?}
             (hash <$(pair (sym :scope) ${array_type? sym_type?}) ...>)
           )
         PATTERN
diff --git a/lib/rubocop/cop/rails/freeze_time.rb b/lib/rubocop/cop/rails/freeze_time.rb
@@ -34,12 +34,12 @@ class FreezeTime < Base
 
         # @!method time_now?(node)
         def_node_matcher :time_now?, <<~PATTERN
-          (const nil? {:Time :DateTime})
+          (const {nil? cbase} {:Time :DateTime})
         PATTERN
 
         # @!method zoned_time_now?(node)
         def_node_matcher :zoned_time_now?, <<~PATTERN
-          (send (const nil? :Time) :zone)
+          (send (const {nil? cbase} :Time) :zone)
         PATTERN
 
         def on_send(node)
diff --git a/lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb b/lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb
@@ -37,7 +37,7 @@ class HasManyOrHasOneDependent < Base
         RESTRICT_ON_SEND = %i[has_many has_one].freeze
 
         def_node_search :active_resource_class?, <<~PATTERN
-          (const (const nil? :ActiveResource) :Base)
+          (const (const {nil? cbase} :ActiveResource) :Base)
         PATTERN
 
         def_node_matcher :association_without_options?, <<~PATTERN
diff --git a/lib/rubocop/cop/rails/helper_instance_variable.rb b/lib/rubocop/cop/rails/helper_instance_variable.rb
@@ -37,7 +37,7 @@ class HelperInstanceVariable < Base
         def_node_matcher :form_builder_class?, <<~PATTERN
           (const
             (const
-               (const nil? :ActionView) :Helpers) :FormBuilder)
+               (const {nil? cbase} :ActionView) :Helpers) :FormBuilder)
         PATTERN
 
         def on_ivar(node)
diff --git a/lib/rubocop/cop/rails/mailer_name.rb b/lib/rubocop/cop/rails/mailer_name.rb
@@ -34,8 +34,8 @@ class MailerName < Base
 
         def_node_matcher :mailer_base_class?, <<~PATTERN
           {
-            (const (const nil? :ActionMailer) :Base)
-            (const nil? :ApplicationMailer)
+            (const (const {nil? cbase} :ActionMailer) :Base)
+            (const {nil? cbase} :ApplicationMailer)
           }
         PATTERN
 
@@ -44,7 +44,7 @@ class MailerName < Base
         PATTERN
 
         def_node_matcher :class_new_definition?, <<~PATTERN
-          (send (const nil? :Class) :new #mailer_base_class?)
+          (send (const {nil? cbase} :Class) :new #mailer_base_class?)
         PATTERN
 
         def on_class(node)
diff --git a/lib/rubocop/cop/rails/migration_class_name.rb b/lib/rubocop/cop/rails/migration_class_name.rb
@@ -29,7 +29,7 @@ def on_class(node)
 
           basename = basename_without_timestamp_and_suffix(processed_source.file_path)
 
-          class_identifier = node.identifier
+          class_identifier = node.identifier.location.name
           camelized_basename = camelize(basename)
           return if class_identifier.source.casecmp(camelized_basename).zero?
 
diff --git a/lib/rubocop/cop/rails/output.rb b/lib/rubocop/cop/rails/output.rb
@@ -32,7 +32,7 @@ class Output < Base
           (send
             {
               (gvar #match_gvar?)
-              {(const nil? :STDOUT) (const nil? :STDERR)}
+              (const {nil? cbase} {:STDOUT :STDERR})
             }
             {:binwrite :syswrite :write :write_nonblock}
             ...)
diff --git a/lib/rubocop/cop/rails/short_i18n.rb b/lib/rubocop/cop/rails/short_i18n.rb
@@ -49,7 +49,7 @@ class ShortI18n < Base
         RESTRICT_ON_SEND = PREFERRED_METHODS.keys.freeze
 
         def_node_matcher :long_i18n?, <<~PATTERN
-          (send {nil? (const nil? :I18n)} ${:translate :localize} ...)
+          (send {nil? (const {nil? cbase} :I18n)} ${:translate :localize} ...)
         PATTERN
 
         def on_send(node)
diff --git a/lib/rubocop/cop/rails/skips_model_validations.rb b/lib/rubocop/cop/rails/skips_model_validations.rb
@@ -57,7 +57,7 @@ class SkipsModelValidations < Base
 
         def_node_matcher :good_touch?, <<~PATTERN
           {
-            (send (const nil? :FileUtils) :touch ...)
+            (send (const {nil? cbase} :FileUtils) :touch ...)
             (send _ :touch {true false})
           }
         PATTERN
diff --git a/lib/rubocop/cop/rails/time_zone_assignment.rb b/lib/rubocop/cop/rails/time_zone_assignment.rb
@@ -23,7 +23,7 @@ class TimeZoneAssignment < Base
         RESTRICT_ON_SEND = %i[zone=].freeze
 
         def_node_matcher :time_zone_assignment?, <<~PATTERN
-          (send (const nil? :Time) :zone= ...)
+          (send (const {nil? cbase} :Time) :zone= ...)
         PATTERN
 
         def on_send(node)
diff --git a/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb b/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb
@@ -3,7 +3,12 @@
 RSpec.describe RuboCop::Cop::Rails::ActionControllerFlashBeforeRender, :config do
   context 'when using `flash` before `render`' do
     context 'within an instance method' do
-      %w[ActionController::Base ApplicationController].each do |parent_class|
+      %w[
+        ::ActionController::Base
+        ::ApplicationController
+        ActionController::Base
+        ApplicationController
+      ].each do |parent_class|
         context "within a class inherited from #{parent_class}" do
           it 'registers an offense and corrects when the render call is explicit' do
             expect_offense(<<~RUBY)
diff --git a/spec/rubocop/cop/rails/action_controller_test_case_spec.rb b/spec/rubocop/cop/rails/action_controller_test_case_spec.rb
@@ -36,6 +36,19 @@ class MyControllerTest < ActionDispatch::IntegrationTest
     RUBY
   end
 
+  it 'adds offense when defining `::MyControllerTest`' do
+    expect_offense(<<~RUBY)
+      class ::MyControllerTest < ActionController::TestCase
+                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `ActionDispatch::IntegrationTest` instead.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class ::MyControllerTest < ActionDispatch::IntegrationTest
+      end
+    RUBY
+  end
+
   it 'adds offense when extending `ActionController::TestCase` and having a method definition' do
     expect_offense(<<~RUBY)
       class MyControllerTest < ActionController::TestCase
diff --git a/spec/rubocop/cop/rails/application_controller_spec.rb b/spec/rubocop/cop/rails/application_controller_spec.rb
@@ -24,6 +24,17 @@ class MyController < ApplicationController; end
     RUBY
   end
 
+  it 'corrects controllers that subclass `::ActionController::Base`' do
+    expect_offense(<<~RUBY)
+      class MyController < ::ActionController::Base; end
+                           ^^^^^^^^^^^^^^^^^^^^^^^^ Controllers should subclass `ApplicationController`.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class MyController < ApplicationController; end
+    RUBY
+  end
+
   it 'corrects controllers defined in module namespaces' do
     expect_offense(<<~RUBY)
       module Nested
diff --git a/spec/rubocop/cop/rails/application_job_spec.rb b/spec/rubocop/cop/rails/application_job_spec.rb
@@ -73,6 +73,19 @@ class MyJob < ApplicationJob; end
       end
     end
 
+    context 'when subclassing `::ActiveJob::Base`' do
+      it 'registers an offense' do
+        expect_offense(<<~RUBY)
+          class MyJob < ::ActiveJob::Base; end
+                        ^^^^^^^^^^^^^^^^^ Jobs should subclass `ApplicationJob`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          class MyJob < ApplicationJob; end
+        RUBY
+      end
+    end
+
     context 'when subclassing `ActiveJob::Base` in a module namespace' do
       it 'registers an offense' do
         expect_offense(<<~RUBY)
diff --git a/spec/rubocop/cop/rails/application_mailer_spec.rb b/spec/rubocop/cop/rails/application_mailer_spec.rb
@@ -77,6 +77,17 @@ class MyMailer < ApplicationMailer; end
       RUBY
     end
 
+    it 'corrects mailers that subclass `::ActionMailer::Base`' do
+      expect_offense(<<~RUBY)
+        class MyMailer < ::ActionMailer::Base; end
+                         ^^^^^^^^^^^^^^^^^^^^ Mailers should subclass `ApplicationMailer`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class MyMailer < ApplicationMailer; end
+      RUBY
+    end
+
     it 'corrects mailers defined in module namespaces' do
       expect_offense(<<~RUBY)
         module Nested
diff --git a/spec/rubocop/cop/rails/application_record_spec.rb b/spec/rubocop/cop/rails/application_record_spec.rb
@@ -70,6 +70,19 @@ class MyModel < ApplicationRecord
       RUBY
     end
 
+    it 'corrects models that subclass ::ActiveRecord::Base' do
+      expect_offense(<<~RUBY)
+        class MyModel < ::ActiveRecord::Base
+                        ^^^^^^^^^^^^^^^^^^^^ Models should subclass `ApplicationRecord`.
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class MyModel < ApplicationRecord
+        end
+      RUBY
+    end
+
     it 'corrects single-line class definitions' do
       expect_offense(<<~RUBY)
         class MyModel < ActiveRecord::Base; end
diff --git a/spec/rubocop/cop/rails/dot_separated_keys_spec.rb b/spec/rubocop/cop/rails/dot_separated_keys_spec.rb
@@ -36,6 +36,17 @@
     RUBY
   end
 
+  it 'registers an offense and corrects when translating keys with convertible scopes are used with `::I18n`' do
+    expect_offense(<<~RUBY)
+      ::I18n.t :key, scope: [:one, :two]
+                     ^^^^^^^^^^^^^^^^^^^ Use the dot-separated keys instead of specifying the `:scope` option.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      ::I18n.t 'one.two.key'
+    RUBY
+  end
+
   it 'does not register an offense when key is an array' do
     expect_no_offenses(<<~RUBY)
       t [:key1, :key2], scope: :one
diff --git a/spec/rubocop/cop/rails/dynamic_find_by_spec.rb b/spec/rubocop/cop/rails/dynamic_find_by_spec.rb
@@ -178,6 +178,17 @@ def do_something
       RUBY
     end
 
+    it 'registers an offense when inheriting `::ApplicationRecord`' do
+      expect_offense(<<~RUBY)
+        class C < ::ApplicationRecord
+          def do_something
+            find_by_name(name)
+            ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
+          end
+        end
+      RUBY
+    end
+
     it 'registers an offense when inheriting `ActiveRecord::Base`' do
       expect_offense(<<~RUBY)
         class C < ActiveRecord::Base
@@ -188,6 +199,17 @@ def do_something
         end
       RUBY
     end
+
+    it 'registers an offense when inheriting `::ActiveRecord::Base`' do
+      expect_offense(<<~RUBY)
+        class C < ::ActiveRecord::Base
+          def do_something
+            find_by_name(name)
+            ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
+          end
+        end
+      RUBY
+    end
   end
 
   context 'with allowed receiver name' do
diff --git a/spec/rubocop/cop/rails/find_each_spec.rb b/spec/rubocop/cop/rails/find_each_spec.rb
@@ -108,6 +108,15 @@ class C < ApplicationRecord
       RUBY
     end
 
+    it 'registers an offense when inheriting `::ApplicationRecord`' do
+      expect_offense(<<~RUBY)
+        class C < ::ApplicationRecord
+          all.each { |u| u.x }
+              ^^^^ Use `find_each` instead of `each`.
+        end
+      RUBY
+    end
+
     it 'registers an offense when inheriting `ActiveRecord::Base`' do
       expect_offense(<<~RUBY)
         class C < ActiveRecord::Base
@@ -116,6 +125,15 @@ class C < ActiveRecord::Base
         end
       RUBY
     end
+
+    it 'registers an offense when inheriting `::ActiveRecord::Base`' do
+      expect_offense(<<~RUBY)
+        class C < ::ActiveRecord::Base
+          all.each { |u| u.x }
+              ^^^^ Use `find_each` instead of `each`.
+        end
+      RUBY
+    end
   end
 
   context 'allowed methods' do
diff --git a/spec/rubocop/cop/rails/freeze_time_spec.rb b/spec/rubocop/cop/rails/freeze_time_spec.rb
diff --git a/spec/rubocop/cop/rails/has_many_or_has_one_dependent_spec.rb b/spec/rubocop/cop/rails/has_many_or_has_one_dependent_spec.rb
diff --git a/spec/rubocop/cop/rails/helper_instance_variable_spec.rb b/spec/rubocop/cop/rails/helper_instance_variable_spec.rb
diff --git a/spec/rubocop/cop/rails/mailer_name_spec.rb b/spec/rubocop/cop/rails/mailer_name_spec.rb
diff --git a/spec/rubocop/cop/rails/migration_class_name_spec.rb b/spec/rubocop/cop/rails/migration_class_name_spec.rb
diff --git a/spec/rubocop/cop/rails/output_spec.rb b/spec/rubocop/cop/rails/output_spec.rb
diff --git a/spec/rubocop/cop/rails/reversible_migration_method_definition_spec.rb b/spec/rubocop/cop/rails/reversible_migration_method_definition_spec.rb
diff --git a/spec/rubocop/cop/rails/reversible_migration_spec.rb b/spec/rubocop/cop/rails/reversible_migration_spec.rb
diff --git a/spec/rubocop/cop/rails/short_i18n_spec.rb b/spec/rubocop/cop/rails/short_i18n_spec.rb
diff --git a/spec/rubocop/cop/rails/skips_model_validations_spec.rb b/spec/rubocop/cop/rails/skips_model_validations_spec.rb
diff --git a/spec/rubocop/cop/rails/time_zone_assignment_spec.rb b/spec/rubocop/cop/rails/time_zone_assignment_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+* [#868](https://github.com/rubocop/rubocop-rails/pull/868): Support `::` prefixed constants on `Rails/ActionControllerFlashBeforeRender`, `Rails/ActionControllerTestCase`, `Rails/ApplicationController`, `Rails/ApplicationJob`, `Rails/ApplicationMailer`, `Rails/ApplicationRecord`, `Rails/DotSeparatedKeys`, `Rails/DynamicFindBy`, `Rails/FindEach`, `Rails/FreezeTime`, `Rails/HasManyOrHasOneDependent`, `Rails/HelperInstanceVariable`, `Rails/MailerName`, `Rails/MigrationClassName`, `Rails/Output`, `Rails/ReversibleMigrationMethodDefinition`, `Rails/ReversibleMigration`, `Rails/ShortI18n`, `Rails/SkipsModelValidations`, and `Rails/TimeZoneAssignment`. ([@r7kamura][])
Original file line number	Diff line number	Diff line change
`@@ -10,8 +10,8 @@ module ActiveRecordHelper`
`10`	`10`
`11`	`11`	`def_node_matcher :active_record?, <<~PATTERN`
`12`	`12`	`{`
`13`		`- (const nil? :ApplicationRecord)`
`14`		`- (const (const nil? :ActiveRecord) :Base)`
	`13`	`+ (const {nil? cbase} :ApplicationRecord)`
	`14`	`+ (const (const {nil? cbase} :ActiveRecord) :Base)`
`15`	`15`	`}`
`16`	`16`	`PATTERN`
`17`	`17`
Original file line number	Diff line number	Diff line change
`@@ -43,8 +43,8 @@ class ActionControllerFlashBeforeRender < Base`
`43`	`43`
`44`	`44`	`def_node_search :action_controller?, <<~PATTERN`
`45`	`45`	`{`
`46`		`- (const nil? :ApplicationController)`
`47`		`- (const (const nil? :ActionController) :Base)`
	`46`	`+ (const {nil? cbase} :ApplicationController)`
	`47`	`+ (const (const {nil? cbase} :ActionController) :Base)`
`48`	`48`	`}`
`49`	`49`	`PATTERN`
`50`	`50`
Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ class DotSeparatedKeys < Base`
`24`	`24`	`TRANSLATE_METHODS = %i[translate t].freeze`
`25`	`25`
`26`	`26`	`def_node_matcher :translate_with_scope?, <<~PATTERN`
`27`		`- (send {nil? (const nil? :I18n)} {:translate :t} ${sym_type? str_type?}`
	`27`	`+ (send {nil? (const {nil? cbase} :I18n)} {:translate :t} ${sym_type? str_type?}`
`28`	`28`	`(hash <$(pair (sym :scope) ${array_type? sym_type?}) ...>)`
`29`	`29`	`)`
`30`	`30`	`PATTERN`