Lots of false `Style/ReverseFind` warnings in Rails

[robingram]

We've just received a dependabot PR for 1.54.0 and the cop Style/ReverseFind raises warnings for collections that do not implement rfind, in particular in Rails apps for ActiveRecord::AssociationRelation but also Nokogiri::XML::NodeSet, as well as potentially other examples. This means that we have to ignore those errors on either a per line or file basis. It feels like this cop is too much of a blunt instrument at the moment.

[jasonkarns]

Thank you for the report!

Could you share an example of a violation of each of those? Does AssociationRelation implement reverse or is this a bug with the cop? (AFAIK, the cop should only be suggesting rfind in place of reverse.find.)

This might be worth reporting as an issue to rubocop as well, if it is especially prone to false-positives. Perhaps the cop can be improved to be more discerning?

Regardless, we'll re-evaluate whether this cop is worth its cost in standard. It's officially listed as a Style cop, but has a performance improvement, so that weighs into it as well.

[robingram]

Hi Jason, sorry for not getting back to you earlier, and not providing more context originally.

Yes, we are using reverse.find in a few places.

One example is a has_many association which is a Document::ActiveRecord_AssociationRelation.

@documents = tenancy.documents.order(:created_at)
...
@documents.reverse.find(&:bond_refund?)

Switching to rfind gives:

NoMethodError:
       undefined method 'rfind' for an instance of ActiveRecord::AssociationRelation

The nokogiri example is a Nokogiri::XML::NodeSet, which also doesn't implement rfind. In an ActionMailbox where page is a Nokogiri::HTML4::Document:

name_parent = page.at("img[src^='#{profile_image_src}']")&.parent&.parent
name_parent.children.reverse.find { |node| node.name == "td" }.text.strip

A similar error if we use rfind.

Thanks, and I hope that helps.