False positive for Style/RedundantFetchBlock cop with Rails.cache.fetch

[dmarcoux]

I work in a Rails project in which we have a few instances of

Rails.cache.fetch('something') { 'value_for_cache_miss' }

The Style/RedundantFetchBlock cop shouldn't trigger in this case since

Rails.cache.fetch('something', 'value_for_cache_miss')

isn't the same. It's documented here. Anything following the first parameter is considered to be an option. The value for a cache miss has to be passed as a block.

---

Expected behavior

I expect RuboCop to not register offenses for Rails.cache.fetch as explained above.

Actual behavior

An offense for the Style/RedundantFetchBlock cop is registered. This is a false positive.

Steps to reproduce the problem

With Rails.cache.fetch('something') { "value_for_cache_miss" }

RuboCop version

0.86.0 (using Parser 2.7.1.4, rubocop-ast 0.0.3, running on ruby 2.5.7 x86_64-linux-gnu)

[bbatsov]

Fixing this is going to be tricky, due to the dynamic nature of Ruby - we can never be sure about the type of a receiver. The best thing that comes to mind is to have some configuration about allowed receivers or whatever, as I don't want to hardcode some special handling of Rails.cache. //cc @fatkodima

[fatkodima]

This cop is already marked as Safe: false, so false positives are expected.

Something like IgnoredReceivers: [] configuration can be added. But I think just using # rubocop:disable ... is also a good solution in this case.

[bbatsov]

Yeah, that's true.

[dmarcoux]

Adding a pair of disable/enable comments is what I ended up doing before submitting this issue. It was good enough in my case since I didn't have a lot of Rails.cache.fetch calls.

[marcandre]

This cop is already marked as Safe: false, so false positives are expected.

A very personal opinion, but custom objects defining fetch is one thing, and Rails is quite another. It is such a popular use of Ruby that we should not definitely not ignore it; I consider it almost like builtin methods. It is the only gem I can think of that can monkey patch core classes and that will be taken into account by ruby-core when making changes to the core language.

This cop should be aware of Rails.cache.fetch, or at the very least rubocop-rails should patch it / create another cop that replace it, but that seems more complicated for no gain.

Something like IgnoredReceivers: [] configuration can be added. But I think just using # rubocop:disable ... is also a good solution in this case.

If we provide a setting, perfect, otherwise I'd hardcode Rails.cache even if it is ugly.

[fatkodima]

From 3 suggested options, I would also prefer hardcoding Rails.cache - as it will not require any configuration from user (as opposite to IgnoredReceivers setting). And I'm not seeing much value in that setting option with hardcoded receiver names, because the same thing can be named differently in different contexts: Rails.cache vs just cache receiver name vs c receiver name.

I would implement any approach, just let me know.

[marcandre]

Maybe we can start with hardcoding Rails.cache and see if we get reports of other false negatives?