Merge pull request #8279 from Fatsoma/feature/require_order_block_pass

Detect require block passed as argument in Lint/NonDeterministicRequireOrder

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### New features
 
+* [#8279](https://github.com/rubocop-hq/rubocop/pull/8279): Recognise require method passed as argument in `Lint/NonDeterministicRequireOrder` cop. ([@biinari][])
 * [#7333](https://github.com/rubocop-hq/rubocop/issues/7333): Add new `Style/RedundantFileExtensionInRequire` cop. ([@fatkodima][])
 * [#8316](https://github.com/rubocop-hq/rubocop/pull/8316): Support autocorrect for `Lint/DisjunctiveAssignmentInConstructor` cop. ([@fatkodima][])
 * [#8242](https://github.com/rubocop-hq/rubocop/pull/8242): Internal profiling available with `bin/rubocop-profile` and rake tasks. ([@marcandre][])

docs/modules/ROOT/pages/cops_lint.adoc

@@ -1943,6 +1943,24 @@ Dir.glob(Rails.root.join(__dir__, 'test', '*.rb')).sort.each do |file|
 end
 ----
 
+[source,ruby]
+----
+# bad
+Dir['./lib/**/*.rb'].each(&method(:require))
+
+# good
+Dir['./lib/**/*.rb'].sort.each(&method(:require))
+----
+
+[source,ruby]
+----
+# bad
+Dir.glob(Rails.root.join('test', '*.rb'), &method(:require))
+
+# good
+Dir.glob(Rails.root.join('test', '*.rb')).sort.each(&method(:require))
+----
+
 == Lint/NonLocalExitFromIterator
 
 |===

lib/rubocop/cop/lint/non_deterministic_require_order.rb

@@ -35,6 +35,22 @@ module Lint
       #     require file
       #   end
       #
+      # @example
+      #
+      #   # bad
+      #   Dir['./lib/**/*.rb'].each(&method(:require))
+      #
+      #   # good
+      #   Dir['./lib/**/*.rb'].sort.each(&method(:require))
+      #
+      # @example
+      #
+      #   # bad
+      #   Dir.glob(Rails.root.join('test', '*.rb'), &method(:require))
+      #
+      #   # good
+      #   Dir.glob(Rails.root.join('test', '*.rb')).sort.each(&method(:require))
+      #
       class NonDeterministicRequireOrder < Cop
         MSG = 'Sort files before requiring them.'
 
@@ -49,7 +65,16 @@ def on_block(node)
           end
         end
 
+        def on_block_pass(node)
+          return unless method_require?(node)
+          return unless unsorted_dir_pass?(node.parent)
+
+          add_offense(node.parent)
+        end
+
         def autocorrect(node)
+          return correct_block_pass(node) if node.arguments.last&.block_pass_type?
+
           if unsorted_dir_block?(node)
             lambda do |corrector|
               corrector.replace(node, "#{node.source}.sort.each")
@@ -64,10 +89,38 @@ def autocorrect(node)
 
         private
 
+        def correct_block_pass(node)
+          if unsorted_dir_glob_pass?(node)
+            lambda do |corrector|
+              block_arg = node.arguments.last
+              corrector.remove(last_arg_range(node))
+              corrector.insert_after(node, ".sort.each(#{block_arg.source})")
+            end
+          else
+            lambda do |corrector|
+              corrector.replace(node.loc.selector, 'sort.each')
+            end
+          end
+        end
+
+        # Returns range of last argument including comma and whitespace.
+        #
+        # @return [Parser::Source::Range]
+        #
+        def last_arg_range(node)
+          node.arguments.last.source_range.with(
+            begin_pos: node.arguments[-2].source_range.end_pos
+          )
+        end
+
         def unsorted_dir_loop?(node)
           unsorted_dir_block?(node) || unsorted_dir_each?(node)
         end
 
+        def unsorted_dir_pass?(node)
+          unsorted_dir_glob_pass?(node) || unsorted_dir_each_pass?(node)
+        end
+
         def_node_matcher :unsorted_dir_block?, <<~PATTERN
           (send (const {nil? cbase} :Dir) :glob ...)
         PATTERN
@@ -76,6 +129,20 @@ def unsorted_dir_loop?(node)
           (send (send (const {nil? cbase} :Dir) {:[] :glob} ...) :each)
         PATTERN
 
+        def_node_matcher :method_require?, <<~PATTERN
+          (block-pass (send nil? :method (sym :require)))
+        PATTERN
+
+        def_node_matcher :unsorted_dir_glob_pass?, <<~PATTERN
+          (send (const {nil? cbase} :Dir) :glob ...
+            (block-pass (send nil? :method (sym :require))))
+        PATTERN
+
+        def_node_matcher :unsorted_dir_each_pass?, <<~PATTERN
+          (send (send (const {nil? cbase} :Dir) {:[] :glob} ...) :each
+            (block-pass (send nil? :method (sym :require))))
+        PATTERN
+
         def_node_matcher :loop_variable, <<~PATTERN
           (args (arg $_))
         PATTERN

spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb

@@ -7,49 +7,54 @@
 
   context 'when requiring files' do
     context 'with unsorted index' do
-      it 'registers an offsense' do
+      it 'registers an offsense and autocorrects to add .sort' do
         expect_offense(<<~RUBY)
           Dir["./lib/**/*.rb"].each do |file|
           ^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
             require file
           end
         RUBY
+        expect_correction(<<~RUBY)
+          Dir["./lib/**/*.rb"].sort.each do |file|
+            require file
+          end
+        RUBY
       end
 
       it 'registers an offsense with extra logic' do
         expect_offense(<<~RUBY)
           Dir["./lib/**/*.rb"].each do |file|
           ^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
-            if file.starts_with('_')
+            if file.start_with?('_')
               puts "Not required."
             else
               require file
             end
           end
         RUBY
-      end
-
-      it 'registers an offense with block passed as parameter' do
-        pending
-        expect_offense(<<~RUBY)
-          Dir["./lib/**/*.rb"].each(&method(:require))
-          ^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
-        RUBY
-      end
-
-      it 'auto-corrects to add .sort' do
-        new_source = autocorrect_source(<<~RUBY)
-          Dir["./lib/**/*.rb"].each do |file|
-            require file
-          end
-        RUBY
-        expect(new_source).to eq(<<~RUBY)
+        expect_correction(<<~RUBY)
           Dir["./lib/**/*.rb"].sort.each do |file|
-            require file
+            if file.start_with?('_')
+              puts "Not required."
+            else
+              require file
+            end
           end
         RUBY
       end
 
+      context 'with require block passed as parameter' do
+        it 'registers an offense an autocorrects to add sort' do
+          expect_offense(<<~RUBY)
+            Dir["./lib/**/*.rb"].each(&method(:require))
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
+          RUBY
+          expect_correction(<<~RUBY)
+            Dir["./lib/**/*.rb"].sort.each(&method(:require))
+          RUBY
+        end
+      end
+
       context 'with top-level ::Dir' do
         it 'registers an offense and corrects to add .sort' do
           expect_offense(<<~RUBY)
@@ -68,28 +73,32 @@
     end
 
     context 'with unsorted glob' do
-      it 'registers an offsense' do
+      it 'registers an offsense and autocorrects to add .sort' do
         expect_offense(<<~RUBY)
           Dir.glob(Rails.root.join(__dir__, 'test', '*.rb'), File::FNM_DOTMATCH).each do |file|
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
             require file
           end
         RUBY
-      end
-
-      it 'auto-corrects to add .sort' do
-        new_source = autocorrect_source(<<~RUBY)
-          Dir.glob(Rails.root.join(__dir__, 'test', '*.rb'), File::FNM_DOTMATCH).each do |file|
-            require file
-          end
-        RUBY
-        expect(new_source).to eq(<<~RUBY)
+        expect_correction(<<~RUBY)
           Dir.glob(Rails.root.join(__dir__, 'test', '*.rb'), File::FNM_DOTMATCH).sort.each do |file|
             require file
           end
         RUBY
       end
 
+      context 'with require block passed as parameter' do
+        it 'registers an offense an autocorrects to add sort' do
+          expect_offense(<<~RUBY)
+            Dir.glob(Rails.root.join('test', '*.rb')).each(&method(:require))
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
+          RUBY
+          expect_correction(<<~RUBY)
+            Dir.glob(Rails.root.join('test', '*.rb')).sort.each(&method(:require))
+          RUBY
+        end
+      end
+
       context 'with top-level ::Dir' do
         it 'registers an offense and corrects to add .sort' do
           expect_offense(<<~RUBY)
@@ -108,28 +117,39 @@
     end
 
     context 'with direct block glob' do
-      it 'registers an offsense' do
+      it 'registers an offsense and autocorrects to add .sort.each' do
         expect_offense(<<~RUBY)
           Dir.glob("./lib/**/*.rb") do |file|
           ^^^^^^^^^^^^^^^^^^^^^^^^^ Sort files before requiring them.
             require file
           end
         RUBY
-      end
-
-      it 'auto-corrects to add .sort.each' do
-        new_source = autocorrect_source(<<~RUBY)
-          Dir.glob("./lib/**/*.rb") do |file|
-            require file
-          end
-        RUBY
-        expect(new_source).to eq(<<~RUBY)
+        expect_correction(<<~RUBY)
           Dir.glob("./lib/**/*.rb").sort.each do |file|
             require file
           end
         RUBY
       end
 
+      context 'with require block passed as parameter' do
+        it 'registers an offense and autocorrects to add sort' do
+          expect_offense(<<~RUBY)
+            Dir.glob(
+            ^^^^^^^^^ Sort files before requiring them.
+              Rails.root.join('./lib/**/*.rb'),
+              File::FNM_DOTMATCH,
+              &method(:require)
+            )
+          RUBY
+          expect_correction(<<~RUBY)
+            Dir.glob(
+              Rails.root.join('./lib/**/*.rb'),
+              File::FNM_DOTMATCH
+            ).sort.each(&method(:require))
+          RUBY
+        end
+      end
+
       context 'with top-level ::Dir' do
         it 'registers an offense and corrects to add .sort.each' do
           expect_offense(<<~RUBY)
@@ -138,6 +158,11 @@
               require file
             end
           RUBY
+          expect_correction(<<~RUBY)
+            ::Dir.glob("./lib/**/*.rb").sort.each do |file|
+              require file
+            end
+          RUBY
         end
       end
     end