Incorrect Style/SafeNavigation transformation

[adfoster-r7]

Given the following input file, after running rubocop it no longer works:

package = false

if package && package.include?('blah')
  puts 'blah package'
else
  puts 'No Package'
end

It currently gets corrected to the following, which breaks the code:

# frozen_string_literal: true

package = false

if package&.include?('blah')
  puts 'blah package'
else
  puts 'No Package'
end

---

Expected behavior

Rubocop runs successfully on this file and produces valid Ruby

# frozen_string_literal: true

package = false

if package && package.include?('blah')
  puts 'blah package'
else
  puts 'No Package'
end

$ ruby foo.rb
No Package

Actual behavior

After running Rubocop, and running the file:

$ ruby foo.rb 
Traceback (most recent call last):
foo.rb:5:in `<main>': undefined method `include?' for false:FalseClass (NoMethodError)

Steps to reproduce the problem

Create a new file:

$ cat foo.rb 
package = false

if package && package.include?('blah')
  puts 'blah package'
else
  puts 'No Package'
end

Confirm it works:

$ ruby foo.rb 
No Package

Run default rubocop:

$ rubocop -a foo.rb
Inspecting 1 file
C

Offenses:

foo.rb:1:1: C: [Corrected] Style/FrozenStringLiteralComment: Missing frozen string literal comment.
package = false
^
foo.rb:2:1: C: [Corrected] Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
package = false
^
foo.rb:3:4: C: [Corrected] Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.
if package && package.include?('blah')

Confirm that the file still works, in this case it doesn't:

$ ruby foo.rb 
Traceback (most recent call last):
foo.rb:5:in `<main>': undefined method `include?' for false:FalseClass (NoMethodError)

RuboCop version

Version:

0.82.0 (using Parser 2.7.1.1, running on ruby 2.6.5 x86_64-darwin18)

I have no custom .rubocop.yml file, so the default configuration appears to be:

  AllowedMethods:
    - present?
    - blank?
    - presence
    - try
    - try!

[tejasbubane]

@adfoster-r7 Since rubocop is a static analyzer, it cannot determine value of the variable. In this case value is right there, but in usual cases it can be defined anywhere. It is only fair of rubocop to see package && package.xyz and mark it as offense.

You can disable the cop on that particular line: https://rubocop.readthedocs.io/en/latest/configuration/#disabling-cops-within-source-code - instead of disabling it entirely (like you did in linked PR).

Question to maintainers is - should we mark this cop as unsafe? I am on the fence for this.

[rrosenblum]

The SafeNavigation cops had a rocky start when we first introduced them. It took a little while to get the rules ironed out. There have been a few issues since then, but nothing too major. There are some risks with this cop, and other cops.

The code example is perfect for showing off the problem, but it is also a signal of a larger problem in the code itself. Narrowly looking at package && package.include?('blah'), there is some implied state behind what package can be. The code by itself is simple check for object existence followed by usage of the object. The expected values for package would be nil, an enumerable, or a string. false is an unexpected value for how the code is intended to work.

I will admit that package && package.include?('blah') safe guards against the false value and package&.include?('blah') does not. This cop is causing an unintended consequence, and this could be an argument to flag the cop as unsafe. Regardless of that, I would encourage you to look through how that code is being used and refactor it to avoid type confusion. This bug was caused by an automated tool, but these kinds of bugs are very easy for a human to introduce as well when the type of an object cannot be easily inferred based on the code as written.

[marcandre]

We should absolutely mark this cop as unsafe (both as detecting false positives and as incorrect autocorrection)

I just got burned by this when working on #7868. RuboCop's current API for autocorrect allows to return either a lambda or nil/false.

I was refactoring the code to:

lambda = autocorrect(node)
#...
lambda.call(node) if lambda

I'll note that I see not good way to write this without disabling this cop in this case.

[vlad-pisanov]

Great rule, but definitely not safe to auto-correct.

[marcandre]

Thanks for the reminder @vlad-pisanov
Marked as unsafe-autocorrection.