Add new Lint/RedundantWithIndex cop

[koic]

Feature

This cop checks for unneeded with_index.

% cat /tmp/example.rb
# frozen_string_literal: true

ary.each_with_index do |v|
  v
end

ary.each.with_index do |v|
  v
end

% bundle exec rubocop /tmp/example.rb
Inspecting 1 file
W

Offenses:

/tmp/example.rb:3:5: W: Use each instead of each_with_index.
ary.each_with_index do |v|
    ^^^^^^^^^^^^^^^
/tmp/example.rb:7:10: W: Remove unneeded with_index.
ary.each.with_index do |v|
         ^^^^^^^^^^

1 file inspected, 2 offenses detected

Target Problem

The main aim is to remove unneeded with_index and clear the code. This will reduce unneeded code, thus improving performance.

The following is a benchmark script.

% cat /tmp/bench.rb
require 'benchmark/ips'
require 'uri'

Benchmark.ips do |x|
  x.report('each')            { %i[a b].each { |v| v } }
  x.report('each_with_index') { %i[a b].each_with_index { |v| v } }
  x.report('each.with_index') { %i[a b].each.with_index { |v| v } }
  x.compare!
end

It is the result of running the above benchmark script.

% ruby /tmp/bench.rb
Warming up --------------------------------------
                each   207.982k i/100ms
     each_with_index   147.822k i/100ms
     each.with_index   102.341k i/100ms
Calculating -------------------------------------
                each      4.123M (± 3.6%) i/s -     20.590M in   5.000595s
     each_with_index      2.380M (± 3.6%) i/s -     11.974M in   5.038652s
     each.with_index      1.374M (± 5.0%) i/s -      6.857M in   5.005021s

Comparison:
                each:  4123458.5 i/s
     each_with_index:  2379638.7 i/s - 1.73x  slower
     each.with_index:  1373928.0 i/s - 3.00x  slower

The benchmark is for reference, the main aim is to make the code correct. That is why I chose this cop as Lint department.

Other Information

This topic is about JRuby. JRuby has the following Issue related to with_index. And this Issue seems to regress in the latest JRuby 9.1.12.0.

jruby/jruby#4610

RuboCop had one place using unneeded with_index. Perhaps it is the cause of a build error of JRuby 9.1.12.0 at #4703.

https://travis-ci.org/bbatsov/rubocop/jobs/272483544

This PR removes unneeded with_index, so I think that it will solve the above build error.

---

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).
• Used the same coding conventions as the rest of the project.
• 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.
• All tests(rake spec) are passing.
• The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

[rrosenblum]

I think there could make this cop more generic and apply the same concept to each_with_object.

This isn't entirely a safe assumption to make about the code based on the number of arguments that are passed to the block. You can still access the index from the single argument.

[3] pry(main)> [1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }
1: 0
2: 1
3: 2

The opposite of this is used to give meaning to the key and value when iterating over a hash

[6] pry(main)> {a: 1, b: 2, c: 3}.each { |hash| puts "#{hash[0]}: #{hash[1]}" }
a: 1
b: 2
c: 3
[7] pry(main)> {a: 1, b: 2, c: 3}.each { |key, value| puts "#{key}: #{value}" }
a: 1
b: 2
c: 3

[koic]

Thanks for your comment. I also thought about integrating each_with_object. However, since we have not encountered a concrete use case that wrong block argument with with_object, this PR focused on with_index.

[3] pry(main)> [1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }
1: 0
2: 1
3: 2

This behavior is JRuby's regression reported in jruby/jruby#4610. The original behavior of each_with_index is as follows, and index is not handed over.

% ruby -e '[1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }'
1: 0
0: 1
1: 1

And this regression was fixed in JRuby 9.1.13.0 released several hours ago (!) .
http://jruby.org/2017/09/06/jruby-9-1-13-0

% rbenv shell jruby-9.1.12.0 && ruby -e '[1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }'
1: 0
2: 1
3: 2
% rbenv shell jruby-9.1.13.0 && ruby -e '[1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }'
1: 0
0: 1
1: 1

If it depends on the regression behavior, it will break when its regression is fixed. So every code should not depends on the regression behavior.

And I thought that the original each_with_index behavior can be autocorrected.

[rrosenblum]

The example that I provided was using Ruby 2.4.1. This isn't a difference between Ruby and jRuby.

I do see that jRuby was not handling each_with_index with a single block argument properly.

% rbenv shell jruby-9.1.12.0 && ruby -e '[1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }'
1: 0
2: 1
3: 2
% rbenv shell jruby-9.1.13.0 && ruby -e '[1, 2, 3].each_with_index { |foo| puts "#{foo[0]}: #{foo[1]}" }'
1: 0
0: 1
1: 1

Based on this example, it looks like there is still a bug in jRuby with how a single block argument is handled. I assume that jRuby is working to fix this so jRuby and Ruby function the same way. Assuming they do function in the same way, a single block argument to each_with_index is technically valid. Simply checking for a single block argument is not enough to determine if each or each_with_index was intended.

What would you think about changing this slightly to recommend passing multiple arguments to block methods that yield more than one item. each_with_index should have 2 arguments. This could be a little tricky because of the chained methods such as with_index.

[].each_with_object([]).with_index { |(element, object), index| something }

[koic]

I see!
I'm wrong. I seem to have misunderstood this problem 💦

[].each_with_object([]).with_index { |(element, object), index| something }

I think about this use case, please wait for a while. Thank you for pointing out.

[bbatsov]

Normally you'd use redundant instead of unneeded in English.

[koic]

I changed it all together. This changes the name of this cop from Lint/UnneededWithIndex to Lint/RedundantWithIndex. 712bce3

[bbatsov]

Why isn't this done with pattern matching?

[koic]

This was solved with method_name method. 44fa554

[bbatsov]

What's the status here?

[koic]

I'm thinking about redesigning this cop considering the pointed out. I will update it soon. Please just a wait.

[koic]

I examined the case of using only one block argument with ary.each_with_object.with_index.
In conclusion, in cases where only one block argument is used, index seems not to be passed.
So, there is no problem with the current method of checking by the number of block arguments.

In some cases I confirmed. It is exemplified by an Array object, but a Hash object is similar.

Receive index argument

This is a case that does not offense in this cop.

> %i[foo bar].each_with_object([]).with_index { |(v, ret), index| puts "v = #{v}, index = #{index}" }
v = foo, index = 0
v = bar, index = 1
=> []

This also applies to the following cases pointed out. And in this case is no offense.

[].each_with_object([]).with_index { |(element, object), index| something }

The following case is also no offense.

> %i[foo bar].each_with_object([]).with_index { |v, index| puts "v = #{v}, index = #{index}" }
v = [:foo, []], index = 0
v = [:bar, []], index = 1
=> []

with_index is used even though the block argument index is not used

It is a problem case with this cop. It used with_index, but there is no index in the block argument.

> %i[foo bar].each_with_object([]).with_index { |v| puts "v = #{v}" }
v = [:foo, []]
v = [:bar, []]
=> []

Below is also a offense case.

> %i[foo bar].each_with_object([]).with_index { |(v, ret)| puts "v = #{v}, ret = #{ret}" }
v = foo, ret = []
v = bar, ret = []
=> []

From these, we can judge whether it is offense by looking at the number of block arguments.
I thought that redesign was necessary, but it was not necessary from the above confirmation.

In addition, I added some of these use cases to test cases. 71959d3

[koic]

📝 I changed this cop name from Lint/UnneededWithIndex to Lint/RedundantWithIndex. I'd like to change the commit message to the following when do squash.

Add new `Lint/RedundantWithIndex` cop

[rrosenblum]

This looks good to me. I think that a Lint/RedundantWithObject would be valuable as well.