Merge pull request #8279 from Fatsoma/feature/require_order_block_pass

Detect require block passed as argument in Lint/NonDeterministicRequireOrder

Author: bbatsov

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