[Fix #336] Expand the safety warning on Performance/Detect

[ekohl]

In addition to the external library activerecord, the built in Hash class also has compatibility issues.

I'm unsure if this deserves a changelog entry, given it doesn't change the code there shouldn't be an impact (which is why I didn't add tests either).

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• 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.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[koic]

Yeah, this change does not require a changelog entry and tests. Thank you for the improvement!

[r7kamura]

I understand that this cop's autocorrection can fail depending on the receiver. However, I think that the autocorrection will succeed correctly both when the receiver is a Hash and a ActiveRecord::Relation. Can you give me an example of in which case it would fail?

[ekohl]

In the original issue I added an example: #336.

[r7kamura]

@ekohl I had missed it, thanks!

So the description says:

Hash and ActiveRecord do not implement a detect method and find has its own meaning. Correcting Hash and ActiveRecord methods with this cop should be considered unsafe.

but the actual reason of the unsafety is that there is no Hash#reverse.

[ekohl]

I think you're right that it could be phrased better. Should this be considered sufficient:

This cop is unsafe because is assumes the class implements the Enumerable interface, but can't reliably detect this.

Then drop this part with specific method names?

Hash and ActiveRecord do not implement a detect method and find has its own meaning.

So it becomes:

This cop is unsafe because is assumes the class implements the Enumerable interface, but can't reliably detect this. This creates known compatibility issues with Hash, ActiveRecord and other frameworks. Correcting Hash and ActiveRecord methods with this cop should be considered unsafe.

[r7kamura]

hmm, since this cop depends on Array#reverse, it seems more appropriate to write the following:

This cop is unsafe because it assumes that the receiver is an Array or equivalent, but can't reliably detect it.

On another point, I feel it would be better to remove the explanation about ActiveRecord. Because in fact #find in ActiveRecord::Relation delegates to Enumerable#find when a block is passed, and ActiveRecord::Relation can also respond to #reverse, so it shouldn't be a problem.

https://github.com/rails/rails/blob/v7.0.4.2/activerecord/lib/active_record/relation/finder_methods.rb#L68

[ekohl]

Archeology time! ActiveRecord 5 started to include Enumerable: rails/rails@b644964. Then 5010157 added a note on ActiveRecord compatibility note. The ActiveRecord 5.0.0 was released in June 2016, while ActiveRecord note was authored in 2016 (and committed in 2018). By now the whole ActiveRecord note may be obsolete, while my issues with Hash still are relevant.

So I think you're right that the unsafe part is limited to Hash missing a reverse method.