Nonsense warnings about using next to skip iteration

[yorickpeterse]

Consider the following snippet of Ruby code:

foos.each do
  unless foo
    yield bar
  end
end

When running Rubocop on this file the output is as following:

Inspecting 1 file
C

Offenses:

/tmp/test.rb:1:6: C: Use next to skip iteration.
foos.each do
     ^^^^
/tmp/test.rb:2:3: C: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
  unless foo
  ^^^^^^

1 file inspected, 2 offenses detected

The second warning makes sense but the first one is absolute nonsense. That is, the following code is no better:

foos.each do
  next if foo
  yield bar
end

To make things more confusing, the following doesn't produce the above warning:

foos.each do
  yield bar unless foo
end

As a more real-world example, there's the following snippet of code that we have:

review_nodes.each do |review|
  unless review.attr('class').include?('separator')
    yield review
  end
end

Rewriting this as following brings absolutely no benefits:

review_nodes.each do |review|
  next if review.attr('class').include?('separator')

  yield review
end

It might save you a single level of indentation but that's about it. It might make sense if you have multiple conditionals that determine whether or not to skip the loop. However, in simple cases such as those shown above the warning is utter nonsense.

Another snippet, which also contains a confusing line about where the error occurs:

ReviewIterator.new(page).each do |review|
  parsed = parse_review(review)

  unless review_exists?(config, parsed[:review_id])
    reviews << parsed
  end
end

This result in:

Inspecting 1 file
C

Offenses:

/tmp/test.rb:1:26: C: Use next to skip iteration.
ReviewIterator.new(page).each do |review|
                         ^^^^
/tmp/test.rb:4:3: C: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
  unless review_exists?(config, parsed[:review_id])
  ^^^^^^

1 file inspected, 2 offenses detected

Again rewriting this gives no benefits, unless more complex code were to be used as the conditional:

ReviewIterator.new(page).each do |review|
  parsed = parse_review(review)

  next if review_exists?(config, parsed[:review_id])

  reviews << parsed
end

I realize that it's rather difficult to figure out the complexity of code following a next and when to throw warnings or not, but throwing this warning always is a massive overkill.

[yorickpeterse]

I realize this cop can be disabled (and I've done so). However, considering the questionable warnings it produces I don't think it's wise to enable this by default (at least not in its current state).

[bbatsov]

That's a thorough ticket. :-)

Both this cop and the GuardClause cop suffer from the same problem - they are overzealous as they are not mindful of their context. Obviously they were conceived with larger blocks in mind and perhaps we should set some length thresholds beneath which they shouldn't be triggered.

However, considering the questionable warnings it produces I don't think it's wise to enable this by default (at least not in its current state).

Yes, I agree. This has been in my todo list for a while now. If I don't find time to refine those cops I'll disable them by default.

@yujinakayama @jonas054 You input is most welcome!

[jonas054]

@bbatsov Your plan sounds good to me! I agree with @yorickpeterse that the changes suggested by these cops aren't always improvements. Sometimes a simple if or unless expresses your intent more clearly, and I feel silly now for not speaking up sooner, because I've had this nagging doubt about these cops for quite some time. Maybe we have overused next and return in the RuboCop source code as a result.

[geniou]

The GuardClause cop has a configuration option MinBodyLength. Wouldn't it be a solution to add this to the Next cop too and set it to an higher value by default?

[yujinakayama]

@yorickpeterse @bbatsov @jonas054 I agree. Though I usually tend to prefer next over if/unless when there are the options, I've experienced some Next offenses that feel strange and disabled the cop in my personal projects. I think new cops should be less zealous when in doubt. Otherwise people immediately disable them and won't enable them in the future since not all people carefully check RuboCop's changelog.

To refine the cops, I think we need to make a list of example code (though there're already some in the specs) and figure out what preferences there are.

By default, the refined cops should register offenses only for definitely bad structure that almost all people would agree (this would be less strict than the current), and add some configuration options to fit each preference.

[bbatsov]

The GuardClause cop has a configuration option MinBodyLength. Wouldn't it be a solution to add this to the Next cop too and set it to an higher value by default?

@geniou I'm starting to forget things we've already done. Yes, doing the same for Next would offset the problems described to some extent.

In the long run, however, we should do something similar to what @yujinakayama suggested - making those two cops more configurable to suit more personal preferences.

[geniou]

I'll try to add the mentioned configuration to the next cop.

Should we then increase the default value for both cops by default? What would be good, 3?

[bbatsov]

@geniou I've already added the config option, but I've yet to decide on its default. I'm still thinking whether to only update the default or to disable the cop by default.

[geniou]

Since the next cop is from me I would love that it stays enabled. ;-) Do we have use cases where we agree that we need to adapt the cop?

[bbatsov]

Of course - you can disable it just like any other cop in .rubocop.yml.