Rubocop: Style/AccessModifierDeclarations reporting private def_delegators

[ashmaroli]

Background

Methods created by def_delegator are public even if they are defined after the private keyword on a previous line. So, the opted route was to use the private keyword inlined to the method definitions.

require 'forwardable'

# Dummy class
class Alpha
  extend Forwardable

  def_delegator :@alpha, :foobar, :foo
  private def_delegator :@alpha, :foobar, :alphabeta

  def initialize
    @alpha = 'Alphalpha '
  end

  def foobar
    @alpha + 'foobar'
  end

  private

  def_delegator :@alpha, :foobar, :bar

  def lipsum
    @alpha + 'lipsum'
  end
end

obj = Alpha.new
%i[alphabeta bar lipsum].each do |method|
  print "#{method} - ".rjust(15)
  p obj.private_methods.include?(method)
end

On running above script, the output is:

   alphabeta - true
         bar - false
      lipsum - true

Expected behavior

Expected Rubocop with default configs to be happy with the above script

Actual behavior

C: Style/AccessModifierDeclarations: private should not be inlined in method definitions.
  private def_delegator :@alpha, :foobar, :alphabeta
  ^^^^^^^

Steps to reproduce the problem

Outlined above..

RuboCop version

0.57.2

[wata727]

I confirmed this problem. It seems that the method defined by def_delegator ignores the group style private declaration. Perhaps the same problem will occur with another methods.

@brandonweiss What do you think about this? I guess this problem is too difficult...

[brandonweiss]

Hmm, there might be two problems in one here… I’ll break them apart.

The first problem is whether or not the cop has correctly identified an offense, and I believe it has. The default configuration for Style/AccessModifierDeclarations is group. That means that the offense you’re seeing for private def_delegator :@alpha, :foobar, :alphabeta is correct.

You can do one of two things to “technically” correct the offense.

1. Configure Style/AccessModifierDeclarations to use inline
2. Convert the code to use the “group” style as shown below

private

def_delegator :@alpha, :foobar, :alphabeta

The second problem is perhaps, as @wata727 pointed out, that methods defined via def_delegator below the group private declaration are not actually private. Whether or not that makes sense or it’s a flaw in Ruby’s design is sort of irrelevant, that’s just the way it is. 😕 So “technically” correcting the offense via the 2nd option would result in different behavior than expected.

To make methods defined by def_delegator private, you _have_ to use one of the two inline styles.

Given that, there are a few different perspectives and possible solutions.

One

The “group”-style of access modifiers are pretty dangerous. They do not work how most Rubyists think they work and cause problems as a result. You could switch to the inline configuration and stop using them. That’s what I’ve personally done.

Two

If you really like the “group” style, then you can just inline-disable offenses as needed.

private def_delegator :@alpha, :foobar, :alphabeta # rubocop:disable Style/AccessModifierDeclarations

But, as I mentioned, most Rubyists do not learn until much later the confusing nature of how access modifier declarations work, so with the group configuration, many people are going to incorrectly use the access modifier declaration and get unexpected behavior. To prevent this, there are other cops that look for a “group”-style declaration and throw an offense, if say, a class method is defined afterwards.

You could enhance that cop to also throw offenses for “def_delegator” and perhaps other tokens that are incorrectly found below access modifier declrations.

Three

The offense message for this cop is unfortunately misleading. In this particular example it’s telling you to do something that will not actually work. This could be fixed by making the cop more aware of what the access modifier declarations are inlined before, so that when using the group configuration it skips offenses where the “inline”-style is actually necessary. This should probably be done and it wouldn’t be that technically difficult, but it would require figuring out all of the various method definition methods that don’t work with “group”-style declarations. For example, I know attr_reader, attr_writer, and attr_accessor would need to be exempted. Then there’s also def_delegator, def_delegators, def_instance_delegator, def_instance_delegators. As you can see, there could be a lot of things to exempt. But still, probably worth doing!

I’m pretty busy right now so I’m not sure when I might be able to get to this enhancement myself, but I’d happily help review a PR!

[ashmaroli]

@brandonweiss Thank you very much for your response. I found it very informative.
We had switched to inline configuration just a few releases ago when we found out that certain methods we expected to be private were actually not.
Therefore, we have decided to just disable this particular cop globally instead of altering behavior of various classes.

[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!

[stale[bot]]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.