Fix Style/EachForSimpleLoop handling for blocks with a parameter

[j1wilmot]

Problem:
The Style/EachForSimpleLoop cop documents that "This check only applies if the block takes no parameters", however that is not how it behaves. Not running for blocks with parameters was also the original intent when this cop was introduced. The issue is that the each_range_with_zero_origin matcher matches against a block that takes arguments when it should not.

Solution:

• Add tests to ensure an offense is not generated for blocks with params
• Remove the each_range_with_zero_origin matcher

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• 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.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[bbatsov]

I'm a bit puzzled by the proposed change, as we could have just as easily updated the docs. As times actually passes a parameter to the block it seems to me that use-case is just as valid as the one without passing a param to the block. Am I missing something here?

[j1wilmot]

As times actually passes a parameter to the block it seems to me that use-case is just as valid as the one without passing a param to the block.

There are a couple of reasons why times is less ideal than Range#each when a block is passed:

1. times is more explicit when there is a known number of iterations to run, but if a specific value needs to be iterated then using Range#each is more explicit.

# Example:

# Using a range makes it explicit and easy to visualize which values will be passed to the block.
(0..4).each { |n| p n }

# Using times makes it implicit which values will be passed to the block. 
# We can reason that this will pass 0, 1, 2, 3 to the block, but it is less visually clear.
4.times { |n| p n }

2. Using times as a replacement for calling each with a block on a range will only work for ranges that start at 0. So it is more consistent to use Range#each when the value being iterated is needed.

# Example:
# These are not equivalent.
# Only ranges starting at 0 will have the same block value as `times`.

(1..4).each { |n| p n } # => "1\n2\n3\n4\n" 
4.times { |n| p n } # => "0\n1\n2\n3\n" 

[diesl]

Using times as a replacement for calling each with a block on a range will only work for ranges that start at 0. So it is more consistent to use Range#each when the value being iterated is needed.

Yes, this is exactly why I came here!

[bbatsov]

@j1wilmot The changes should also be reflected in the cop's documentation - description & examples.

[j1wilmot]

The changes should also be reflected in the cop's documentation - description & examples.

@bbatsov I think the documentation already reflects the desired behavior, but the implementation previously diverged from the documentation:

"This check only applies if the block takes no parameters."
https://github.com/rubocop/rubocop/pull/11681/files#diff-4ef0af7d2699cabc4a94f5e625da92a87d8ec254c054d94ba8c0b695c9d58980L10

[j1wilmot]

Looks like this was fixed in #12601. Closing.