Skip to content

Commit

Permalink
Detect require method as arg in Lint/NonDeterministicRequireOrder
Browse files Browse the repository at this point in the history
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)
)
```
  • Loading branch information
biinari committed Jul 8, 2020
1 parent 698a17f commit 80813b5
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 8 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -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][])
Expand Down
18 changes: 18 additions & 0 deletions docs/modules/ROOT/pages/cops_lint.adoc
Expand Up @@ -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

|===
Expand Down
67 changes: 67 additions & 0 deletions lib/rubocop/cop/lint/non_deterministic_require_order.rb
Expand Up @@ -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.'

Expand All @@ -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")
Expand All @@ -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
Expand All @@ -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
Expand Down
51 changes: 43 additions & 8 deletions spec/rubocop/cop/lint/non_deterministic_require_order_spec.rb
Expand Up @@ -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|
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 80813b5

Please sign in to comment.