Rubocop: Style/SafeNavigation cop crashes when auto correct

[pocke]

cc: @rrosenblum

Expected behavior

Don't crash

Actual behavior

RuboCop crashes.

Steps to reproduce the problem

Put the following files.

test.rb

if foo
  foo.bar
end

.rubocop.yml

AllCops:
  TargetRubyVersion: 2.3

And run RuboCop with auto correction.

$ rubocop -d -a --only Style/SafeNavigation
For /tmp/tmp.GoBYI5sfic: configuration from /tmp/tmp.GoBYI5sfic/.rubocop.yml
Default configuration from /home/pocke/.gem/ruby/2.3.0/gems/rubocop-0.43.0/config/default.yml
Inheriting configuration from /home/pocke/.gem/ruby/2.3.0/gems/rubocop-0.43.0/config/enabled.yml
Inheriting configuration from /home/pocke/.gem/ruby/2.3.0/gems/rubocop-0.43.0/config/disabled.yml
Inspecting 1 file
Scanning /tmp/tmp.GoBYI5sfic/test.rb
E

Offenses:

test.rb:1:1: C: [Corrected] Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.
if foo ...
^^^^^^
test.rb:3:1: E: unexpected token $end
(Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)

1 file inspected, 2 offenses detected, 1 offense corrected
Finished in 0.0491263820003951 seconds

$ cat test.rb
if foo
  foo&.bar

RuboCop version

$ rubocop -V
0.43.0 (using Parser 2.3.1.3, running on ruby 2.3.1 x86_64-linux)

[dreyks]

Additional examples of incorrect auto-correct (duh!)

• !a || a.empty? and a.nil? || a.empty? are corrected into a&.empty?
• a ? a.b : 'whatever' is corrected to a ? a&.b
• Same with multiline if

if a
  a.b c
else
  'whatever'
end

is corrected into

if a
  a&.b c
# and that's all, rest is simply wiped

First correction results in broken logic, second and third corrections result in syntax errors

[rrosenblum]

Thanks for the bug report. I think that I have an easy fix for the first issue. The other examples will take a little longer to fix, but I don't expect them to be too difficult to fix.

[rrosenblum]

It looks like github auto-linked up my progress on this change since I reused the branch name.

I have a quick solution for @pocke's original request:

if foo
  foo.bar
end

The easiest solution is to not register an offense for non modifier if statements. I would like to spend a little more time on this and register an offense for simple if statement, and not register an offense for more complicated ones.

# register an offense for this
if foo
  foo.bar
end

# allow
if foo
  foo.bar
  baz
end

# allow
if foo
  foo.bar
else
  something
end

@dreyks I think that the examples that you provided !a || a.empty? will warrant a configuration setting. The code in question will work in most cases with safe navigation, but could have some unintended side effects because it will not return the exact same result.

# this is questionable with safe navigation
def something?
  !a || a.empty? # this will always return true or false
end

def something?
  a&.empty? # this will return true, false, or nil
end

# this works fine with safe navigation because nil will be treated as false
something if !a || a.empty?

[dreyks]

The thing is if a variable is nil its empty? is also nil:

before

def do_something(data = nil)
  return if !data || data.empty?

  p 'doing stuff'
end

do_something # => does not output anything

after

def do_something(data = nil)
  return if data&.empty?

  p 'doing stuff'
end

do_something # => outputs "doing stuff"

[rrosenblum]

You are correct. I will remove the checks for || and make a configuration for && for the reasons listed before.

[basex]

Might be better to cut a release since so many people are reporting the same problem.