diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bd2cb7f678..6e1794a980c 100644 --- a/CHANGELOG.md +++ b/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][]) diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index 0f75164560f..decf1d39c8a 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/lint/non_deterministic_require_order.rb b/lib/rubocop/cop/lint/non_deterministic_require_order.rb index c676cf8cded..698019effa3 100644 --- a/lib/rubocop/cop/lint/non_deterministic_require_order.rb +++ b/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 diff --git a/spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb b/spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb index 6e89498f4a0..75d1aa8068e 100644 --- a/spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb +++ b/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