Add new Lint/NonDeterministicRequireOrder cop

[Mangara]

What is this PR trying to do?

Dir[...] and Dir.glob(...) make no guarantees about the order files are returned in:

Case sensitivity depends on your system [...], as does the order in which the results are returned.

This becomes a problem when they are used for applications in which the order can matter, such as requiring files. At worst, this can lead to bugs that only happen intermittently in production and can't be reproduced in development.

This cop suggests adding a .sort:

# bad
Dir["./lib/middleware/*.rb"].each do |file|
  require file
end

# good
Dir["./lib/middleware/*.rb"].sort.each do |file|
  require file
end

Questions for reviewers

• Should it test for more patterns?

The current implementation very specifically looks for these three patterns: Dir[_].each do |_|, Dir.glob(_).each do |_|, and Dir.glob(_) do |_|. Are there other common patterns that have the same effect? Should we allow for intermediate operations between the Dir[...] and .each?

• Should it apply regardless of whether the file is required?

There are other use cases where the order matters, although this cannot in general be detected statically, and you may want to avoid the performance penalty associated with sorting. We could add a configuration option: Always, OnlyForRequires?

• Is there a better way to match the pattern?

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[Mangara]

@buehmann What needs to happen before we can merge this?

[buehmann]

@Mangara I just did a full review and left some comments. In the end we need someone from the core team to visit this.

[buehmann]

Regarding your question

Should it test for more patterns?

I think it might make sense to support this case as well from the beginning:

Dir.glob(Rails.root.join(__dir__, 'test', '*.rb'), File::FNM_DOTMATCH) do |file|
  require file
end

(that is glob taking a block without each)

What do you think? (Adding this should be rather easy with a {(send …) (send …)} alternative in unsorted_dir_loop?.)

[Mangara]

Yes! Good catch. We should definitely support that.

[Mangara]

Is this the best way to write the new pattern?

{ (send (const nil? :Dir) :glob ...) (send (send (const nil? :Dir) {:[] :glob} ...) :each) }

[Mangara]

(Only the .glob variant supports the direct block syntax)

[Mangara]

I ended up splitting it into two patterns to help with autocorrection.

[Drenmi]

The generated methods also take a block, which is yielded to in the case of a match. 🙂

loop_variable(node.arguments) do |var_name|
  return unless var_is_required?(node.body, var_name)

  add_offense(node.send_node)
end

Looks like you could also potentially combine the patterns into a single matcher.

[Mangara]

Ah, nice! I like the block syntax.

As for the single matcher, I like that all of the building blocks are fairly simple patterns. Merging them into one would make it harder to understand, I think.

[Drenmi]

Great job! 🎉

Some minor nits, but overall looks good to me.

[bbatsov]

Can you rebase this on top of master?

[Mangara]

@bbatsov @koic Is this ready to merge?

[koic]

The following commit is due to the impact of this cop addition.
161781d

Can you squash your commits into one?

[koic]

And I think @bbatsov will decide whether the next release will be new feature release or bug fix release. Please wait until the next release is a release that includes new features.