Rubocop: Style/SafeNavigation false negatives

[dankohn]

Style/SafeNavigation seems like it should trigger with code in the following pattern:

create_all unless @badges && @badges.length == 103

This should be suggested to be changed to:

create_all unless @badges.?length == 103

And, this code:

if user && user.authenticated?(:remember, cookies[:remember_token])

should be changed to:

if user.?authenticated?(:remember, cookies[:remember_token])

Is there something about this syntax causing the cop to miss these examples?

$ rubocop -v
0.49.1

Please note from the links above that the project is open source and so easily reviewable. Here is the .rubocop.yml.

Thanks in advance.

Cc @rrosenblum @david-a-wheeler

[rrosenblum]

Thanks for the bug report. I just tested both of the examples, and I was able to reproduce the issue. This is a bug. I will start working on a fix for this. This looks it should be easy to fix.

P.S. Thank you for the clear examples, links to source code, and links to configuration.

[dankohn]

Awesome. Thank you for the cop and the quick response.

[rrosenblum]

This might take a little longer to fix than I originally thought. Looking at the code again, we don't appear to be checking for something like @badges && @badges.length even though we say that we are. I think that this may be due to a bad refactor during the bug fix for #3510, PR #3517. I think was overlooked when it was identified that !a || a.empty? and a.nil? || a.empty? are not safe to convert to safe navigation because they could start returning nil instead of true or false. I do not see the same concern with the example that you provided.

[rrosenblum]

Status Update:

I underestimated the difficulty of this fix. As far as I can tell, this can't be fixed with a small update to the node matcher. This is because as methods are chained, they become nested send nodes, and I can't figure out a way to account for an unlimited number of nested methods through def_node_matcher. This should still be possible through traversing the right hand side of the condition.

[1] pry(#<RuboCop::Cop::Style::SafeNavigation>)> node.source
=> "foo && foo.bar"
[2] pry(#<RuboCop::Cop::Style::SafeNavigation>)> node
=> s(:and,
  s(:send, nil, :foo),
  s(:send,
    s(:send, nil, :foo), :bar))
[1] pry(#<RuboCop::Cop::Style::SafeNavigation>)> node.source
=> "foo && foo.bar == 3"
[2] pry(#<RuboCop::Cop::Style::SafeNavigation>)> node
=> s(:and,
  s(:send, nil, :foo),
  s(:send,
    s(:send,
      s(:send, nil, :foo), :bar), :==,
    s(:int, 3)))
[1] pry(#<RuboCop::Cop::Style::SafeNavigation>)> node.source
=> "foo && foo.bar.baz"
[2] pry(#<RuboCop::Cop::Style::SafeNavigation>)> node
=> s(:and,
  s(:send, nil, :foo),
  s(:send,
    s(:send,
      s(:send, nil, :foo), :bar), :baz))

[rrosenblum]

I made some really good progress on this yesterday. I was able to get the cop to register an offense for your example. I need to add a few more test cases and refactor some of the code.

[russelldavis]

Eagerly awaiting this fix. Thanks for your work and the status updates!