From 80813b53be4a223781669b556984a38060ba4fa0 Mon Sep 17 00:00:00 2001 From: Bill Ruddock Date: Wed, 8 Jul 2020 05:20:01 +0100 Subject: [PATCH 1/2] Detect require method as arg in Lint/NonDeterministicRequireOrder Detect `&method(:require)` passed as a block argument. For example: ```ruby Dir[Rails.root.join('spec/support/**/*.rb')].each(&method(:require)) Dir.glob( Rails.root.join('spec', 'support', '**', '*.rb'), &method(:require) ) ``` --- CHANGELOG.md | 4 ++ docs/modules/ROOT/pages/cops_lint.adoc | 18 +++++ .../lint/non_deterministic_require_order.rb | 67 +++++++++++++++++++ .../non_deterministic_require_order_spec.rb | 51 +++++++++++--- 4 files changed, 132 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d9fc362c7d..54ae4962dad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### New features + +* [#8279](https://github.com/rubocop-hq/rubocop/pull/8279): Recognise require method passed as argument in `Lint/NonDeterministicRequireOrder` cop. ([@biinari][]) + ### Bug fixes * [#8252](https://github.com/rubocop-hq/rubocop/issues/8252): Fix a command line option name from `--safe-autocorrect` to `--safe-auto-correct`, which is compatible with RuboCop 0.86 and lower. ([@koic][]) diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index d97306b859d..7244cc24f70 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -1910,6 +1910,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..f337d8c9a38 100644 --- a/spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb +++ b/spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb @@ -29,14 +29,6 @@ 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| @@ -50,6 +42,18 @@ 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) @@ -90,6 +94,18 @@ 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) @@ -130,6 +146,25 @@ 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) From 739afb60898375d996e64ee3e2ece0f107fdf117 Mon Sep 17 00:00:00 2001 From: Bill Ruddock Date: Wed, 8 Jul 2020 21:41:34 +0100 Subject: [PATCH 2/2] Use expect_correction in Lint/NonDeterministicRequireOrder spec --- .../non_deterministic_require_order_spec.rb | 54 ++++++++----------- 1 file changed, 22 insertions(+), 32 deletions(-) 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 f337d8c9a38..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,37 +7,38 @@ 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 '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 @@ -72,22 +73,14 @@ 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 @@ -124,22 +117,14 @@ 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 @@ -173,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