Add iterator? to deprecated methods and prefer block_given?

[tejasbubane]

Closes #6688

• Wrote [good commit messages][1].
• 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.

[rrosenblum]

The code looks good to me. I think we should add autocorrect support to the cop. I'm not a big fan of the name KernelIteratorPredicate. I don't think most people realize that iterator? even exists.

[Drenmi]

The code looks good to me. I think we should add autocorrect support to the cop.

Would you be open to adding auto-correct @tejasbubane? It involves defining an #autocorrect method on the cop. Something like this should do the trick:

def autocorrect(node)
  lambda do |corrector|
    corrector.replace(node.source_range, 'block_given?')
  end
end

I haven't tested it though. 🙂

[tejasbubane]

@Drenmi yes I will add autocorrect and make all the changes today. Thanks!

[bbatsov]

I don’t think this warrants a dedicated cop. Probably it should be part of some generic cop about preferred methods. I know we have some offenders in the current codebase, but we shouldn’t grow that list.

[tejasbubane]

I don’t think this warrants a dedicated cop. Probably it should be part of some generic cop about preferred methods. I know we have some offenders in the current codebase, but we shouldn’t grow that list.

Which cop do you think this should go in? Create a new one called PreferredMethods?

[bquorning]

We have an existing cop called DeprecatedClassMethods where this code could go.

[rrosenblum]

DeprecatedClassMethods will need to be refactored for this to work. That cop was specifically designed to handle class methods so it looks for both the receiver and the method. iterator? has a nil receiver.

[tejasbubane]

I have made the changes to use DeprecatedClassMethods instead of adding a new cop.

[bquorning]

Thank you 🙏

[Drenmi]

Looks good @tejasbubane! I'm ready to accept the code as is, but it would be cool, since it's a Kernel method, to only register an offense when there is no receiver. I.e. foo.iterator? shouldn't be an offense.

Maybe the code already does this, but the unit test seems to be missing. 🙂

[tejasbubane]

Maybe the code already does this, but the unit test seems to be missing. 🙂

Yes the code already checked for nil receiver. I added the test case.

[dgollahon]

This is less about this specific change and might require further discussion outside of this PR, but I noticed that this cop is marked as safe. Any time a method selector is changed this is not safe (as I mean it: known to be semantically equivalent). On something like File.exists? it's still not technically safe, but there the odds of a semantic change are extremely low so that concerns me a bit less. In this particular case it seems rather unlikely that someone would define #iterator?, but it's definitely more likely than redefining File.exists? and certainly not provably safe.

Maybe it's good to define safe as "safeish" but I, personally, would prefer to be able to trust that safe is truly safe under all circumstances. Perhaps we need a distinction between cops with (in theory) provably safe autocorrects (maybe even assert the AST is unchanged?) v.s. things that almost always do the right thing, even if there are edge cases that rubocop can't reasonably detect via static analysis (for example, the #iterator? -> #block_given?).

Has someone defined a specific policy for what counts as safe anywhere?

[Drenmi]

Has someone defined a specific policy for what counts as safe anywhere?

You're asking good questions, @dgollahon. We don't have a formal definition of this at the moment. It was also relatively recently we started marking cops as unsafe, so I haven't given this a lot of thought. On first thought, I also prefer that "safe" is truly safe, although it could turn out (I don't have a good feel of this) that it would disqualify a very large subset of cops, which might be the wrong trade-off. 🤔