False positive for Style/EachForSimpleLoop

[marocchino]

�I discovered an issue where a parameter was determined not to be used under certain conditions.

(0..9).each { |i| "123" % (i + 1) }
(0..9).each { |i| SomeObject.new(i:) }

Expected behavior

no offence.

Actual behavior

$ echo '(0..9).each { |i| "123" % (i + 1) }' | bundle exec rubocop --stdin test.rb --only Style/EachForSimpleLoop
Inspecting 1 file
C

Offenses:

test.rb:1:1: C: [Correctable] Style/EachForSimpleLoop: Use Integer#times for a simple loop which iterates a fixed number of times.
(0..9).each { |i| "123" % (i + 1) }
^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense autocorrectable
$ echo '(0..9).each { |i| SomeObject.new(i:) }' | bundle exec rubocop --stdin test.rb --only Style/EachForSimpleLoop
Inspecting 1 file
C

Offenses:

test.rb:1:1: C: [Correctable] Style/EachForSimpleLoop: Use Integer#times for a simple loop which iterates a fixed number of times.
(0..9).each { |i| SomeObject.new(i:) }
^^^^^^^^^^^

Steps to reproduce the problem

see Actual behavior

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler.
If you see extension cop versions (e.g. rubocop-performance, rubocop-rspec, and others)
output by rubocop -V, include them as well. Here's an example:

$ [bundle exec] rubocop -V
1.59.0 (using Parser 3.2.2.4, rubocop-ast 1.30.0, running on ruby 3.1.3) [arm64-darwin22]
  - rubocop-capybara 2.19.0
  - rubocop-factory_bot 2.24.0
  - rubocop-performance 1.19.1
  - rubocop-rails 2.22.2
  - rubocop-rspec 2.25.0

[koic]

It appears to behave as expected, so can you provide more information on what issues you are encountering?

[marocchino]

https://docs.rubocop.org/rubocop/cops_style.html#styleeachforsimpleloop
says that:

This check only applies if the block takes no parameters.

My understanding is that even though this uses each, it shouldn't violate Lint if parameters are used. Is my understanding wrong?

[koic]

It seems to have been added in #3182, but I didn't know what "This check only applies if the block takes no parameters." meant. It seems like something is wrong with either the documentation or the behavior.
IMO, the presence or absence of parameters is irrelevant.

[marocchino]

If there are no parameters, only the number of repetitions is important, so times may be easier to read, but if there are parameters, it may be easier to read if you write them in range.
So I think it's better to leave the explanation as is and change the implementation.

[koic]

In #10928, it seems that the behavior changes when the start of the range is zero, even when a block parameter is present. About this cop, it appears that the documentation needs to be updated.

[marocchino]

I still think changing range to times when there are arguments has readability issues.

Consistent with the rule that random numbers should be written as ranges, I would have preferred not to revert the above PR outright, but to make an exception like (0..10).each { |i| }.

[koic]

OK, TBH, I don't have a strong opinion for this. Let's respect the documentation, I've opened #12609.