Rubocop: False positive on Style/SafeNavigation when checking hashes

[daveallie]

The Style/SafeNavigation seems to be firing when checking an object exists and then using that object as a Hash.

Given the following file, where a can be nil or a hash:

a = nil
a && a[:my_key].include?('my value')

The cop seems to consider a[:my_key] as a usage of a. To make rubocop happy, a safe operator is needed before the include?... which changes what the line was doing. Previously it would raise if a exists but a[:my_key] didn't (or if the value didn't respond to #include?), with this change it will only raise of a[:my_key] doesn't respond to #include?, but will not longer raise if it's nil.

---

Expected behavior

Given the above test file, I expect the Style/SafeNavigation cop not to warn.

Actual behavior

The Style/SafeNavigation cop warns with:

Inspecting 1 file
C

Offenses:

file.rb:2:1: C: Use safe navigation (&.) instead of checking if an object exists before calling the method.
a && a[:my_key].include?('my value')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Changing the line to a && a[:my_key]&.include?('my value') stops the cop from complaining. But this has a different meaning than the original line.

Steps to reproduce the problem

Run rubocop against the test file above.

RuboCop version

Include the output of rubocop -V. Here's an example:

$ rubocop -V
0.50.0 (using Parser 2.4.0.0, running on ruby 2.3.1 x86_64-darwin15)

[mrbongiolo]

I'm having a similar issue on this line: resource_logo_tag if resource && resource[:logo_url].
It does stop complaining if I change it to resource_logo_tag if resource&.[](:logo_url), but TBH the usage of &.[](:key) to safe navigate hashes is just ugly.
I changed this method to use a safeguard instead, but anyway maybe this cop shouldn't warn about this kind of usage on hashes, or at least this should be configurable, since I don't want to completely deactivate this cop, but I don't want it complaining about hashes.

[Drenmi]

I changed this method to use a safeguard instead, but anyway maybe this cop shouldn't warn about this kind of usage on hashes, or at least this should be configurable, since I don't want to completely deactivate this cop, but I don't want it complaining about hashes.

I agree that this cop should probably ignore the #[]method altogether, as foo&.[](:bar). is not an idiom anyone uses. (For good reason, I'd like to say. Just look at this sequence of punctuation: &.[](:) 🙂

[jaredbeck]

I agree that this cop should probably ignore the #[]method altogether, as foo&.. is not an idiom anyone uses. (For good reason

I agree. The []= method is even worse-looking:

foo&.[]=(:bar, "baz")

For indexing, maybe rubocop should suggest Hash#dig instead? For assignment, I think the current recommendation to use &.[]= is not good, but I don't know of an alternative that is as nice as dig is for indexing.

[Drenmi]

For indexing, maybe rubocop should suggest Hash#dig instead?

I've been taking a stab at a Dig cop before, without reaching all the way. I might dig (pun very much intended) it up and see what I can do.

For assignment, I think the current recommendation to use &.[]= is not good [...]

In addition, the semantics are different, I believe. If I recall correctly, foo.[]=(:bar, "baz") returns a value.