Rubocop: Rubocop Style/RedundantSelf changes the semantics of inherited private class method calls when autocorrecting

[preetpalS]

This is probably an edge case but the cop [Style/RedundantSelf] changes the semantics of the following code when using the autocorrecting feature of Rubocop. This bug is somewhat related to https://github.com/bbatsov/rubocop/issues/5577 (in particular see comment https://github.com/bbatsov/rubocop/issues/5577#issuecomment-366887761).

# Sample class containing private class method.
class C
  class << self
    private def pri
      puts 'pri'
    end
  end
end

# Sample class inheriting class containing private class method.
class D < C
  class << self
    # Example method containing usage of self that is not redundant.
    def explicit_receiver_test
      self.pri
    end
  end
end

begin
  D.explicit_receiver_test # Throws: NoMethodError
rescue NoMethodError => e
  puts e.message # Prints: private method `pri' called for D:Class
end

---

Expected behavior

RuboCop auto correction should not change the semantics of the code sample.

Actual behavior

RuboCop auto correction changes the semantics of the code sample.

Steps to reproduce the problem

Sample terminal session:

$ cat sample3.rb 

# Sample class containing private class method.
class C
  class << self
    private def pri
      puts 'pri'
    end
  end
end

# Sample class inheriting class containing private class method.
class D < C
  class << self
    # Example method containing usage of self that is not redundant.
    def explicit_receiver_test
      self.pri
    end
  end
end

begin
  D.explicit_receiver_test # Throws: NoMethodError
rescue NoMethodError => e
  puts e.message # Prints: private method `pri' called for D:Class
end

$ ruby sample3.rb 
private method `pri' called for D:Class

$ rubocop -a sample3.rb 
Inspecting 1 file
C

Offenses:

sample3.rb:16:7: C: [Corrected] Style/RedundantSelf: Redundant self detected.
      self.pri
      ^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

$ cat sample3.rb 

# Sample class containing private class method.
class C
  class << self
    private def pri
      puts 'pri'
    end
  end
end

# Sample class inheriting class containing private class method.
class D < C
  class << self
    # Example method containing usage of self that is not redundant.
    def explicit_receiver_test
      pri
    end
  end
end

begin
  D.explicit_receiver_test # Throws: NoMethodError
rescue NoMethodError => e
  puts e.message # Prints: private method `pri' called for D:Class
end

$ ruby sample3.rb 
pri

RuboCop version

$ rubocop -V
0.52.1 (using Parser 2.4.0.2, running on ruby 2.5.0 x86_64-freebsd11.1)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[preetpalS]

This issue should not be closed since it relates to Rubocop changing the semantics of code.

[Drenmi]

This is a very strange case, but it does indeed change the semantics. Since it deals with inheritance, which is somewhat outside the realm what we can reliably do with static analysis, what we can do is mark this auto-correct as unsafe, requiring the user to manually check the offense.

[jonas054]

I guess technically it's an unsafe cop because it changes the semantics, but in the given example it changes a program that crashes into a program that doesn't crash. Is it possible to come up with a less abnormal example where the current RedundantSelf does some damage? Otherwise I don't think we should mark the cop as unsafe (baby, bathwater, ...).

[preetpalS]

If the definition of safe for a cop means that it does not change code semantics, then it should be marked unsafe but I agree with @jonas054 that this cop should be enabled by default though despite this example. The code in the example is broken by default and is "fixed" by rubocop (nobody should be relying on NoMethodError being thrown in a situation like this).