rubocop: Style/AccessModifierDeclarations false positives

I am getting this and it doesn’t make sense to me why.

# frozen_string_literal: true

module Foo
  def bar
    puts 'hi'
  end
  module_function :bar
end

with the result:

lib/tasks/foo.rake:7:3: C: Style/AccessModifierDeclarations: module_function should not be inlined in method definitions.
  module_function :bar
  ^^^^^^^^^^^^^^^

Latest version of rubocop:

∴ rubocop -v
0.57.2

Without being able to use module_function the cop is not useable for me. Can we use an exclusion list like some other cops do?

You can use module_function like so:

module Foo
  def baz
  end

  module_function

  def bar
    puts 'hi'
  end
end

irb(main):001:0> Foo.bar
hi
=> nil

This to me feels like a bug with the way the cop works, and perhaps should be a cause for auto-disabling the cop??

the following “should” not be raising issues. I know it does, by design; but I would argue it “shouldn’t”

class Foo
  attr_reader :baz
  private :baz

  def initialize(baz)
    @baz = baz
  end

  private

  # ten_other_methods
end

I use private as a keyword as seen above and then all private methods below. But I also put readers/writers at the top. I would argue this is the most common use-case.

Is there currently any way to actually write module_function without disabling the cop?

@Nondv There are three ways of using the private declaration.

# 1

Class.new do

  private

  def method; end

end

# 2

Class.new do

  private def method; end

end

# 3

Class.new do

  def method; end
  private :method

end

#1 is the “group” style. #2 and #3 are the “inline” style. Yes, #2 and #3 are not exactly the same, but they are essentially the same. They are both explicit and require an individual private declaration for each method you want to make private and it’s positioned inline, either before the method definition or before the symbol argument it takes.

I don’t really know what percentage of Rubyists use what style, but #3 is necessary because some methods can be defined in different ways. For example:

private 

attr_reader :method

That would need to be come:

attr_reader :method
private :method

As for using the group style but also needing to declare private inline in order to privatize a method defined elsewhere, the solution depends on how common you think that might be. I’ve been a Rubyist about 12 years and have never needed to do that, so if I did I’d be inclined to just inline-disable it occasionally when I need to do it. However, if this something other people commonly do, then attempting to statically analyze if the method is defined in the class before throwing an offense might make sense, although I imagine doing that could be difficult—there are a lot of ways to define a method in Ruby…

Yeah, wasn’t trying to imply those weren’t issues, just worried the original problem case got lost in the other discussion.

It seems like this cop should maybe just be checking for def defined methods only.

@TrevorBramble The way I would explain it is, the access modifier is being inlined into something (a thing indicating the method being modified). In one scenario it’s being inlined into the method. In another scenario it’s being inlined into a symbol. Some people seem to be confused by it, so perhaps it wasn’t the right name? An alternative was “argument”, but I nixed that because although technically in both 2 and 3 an argument is being passed, a lot of people don’t understand that access modifiers are methods and not special keywords. I also considered “explicit”, but when using the word “explicit” there is an implicit (😂) antonym of “implicit”, and I didn’t think that was a good name for “group”. “inline” was the best I could come up with. 🤷🏼‍♂️

Regarding having three names, sure, it could be possible to come up with another name that distinguishes 2 and 3, but then that implies that there is some reason to choose 2 versus 3, which I don’t think there is. I think if you choose “inline” you want to use 2. However, there are some scenarios in which you might have to use 3.

Here’s another example where the current default settings introduced in 0.57 conflict.

In general, I tend to avoid using instance variables directly and rather create an attr_accessor, followed by making the writer private. This way I can help avoid that direct changes to instance variables pour into the code, and if I need to make adjustments to the setter / getter logic I can just drop the default attr methods and create my custom logic there, keeping i.e. my initializer untouched. See this code:

# frozen_string_literal: true

# Example for rubocop 0.57 complaint for https://github.com/rubocop-hq/rubocop/issues/5953
class Example
  attr_accessor :foo
  private :foo=

  def initialize(foo)
    self.foo = foo
  end
end

With rubocop 0.57 this is getting blamed in the default settings:

rubocop-5953.rb:6:3: C: Style/AccessModifierDeclarations: private should not be inlined in method definitions.
  private :foo=
  ^^^^^^^

I do like the general intention of introducing consistency via https://github.com/rubocop-hq/rubocop/pull/5444, but as it stands I have to disable this new cop on all my projects because it does not fit my needs (group style in the class body, inline style for attr_accessor and friends (i.e. this also applies to ActiveSupport delegate methods)

IMO, a new ticket should be opened which collates all of the other scenarios that have been well documented here, and which have not yet been fixed. CC @koic @luke-hill

As @phillycheeze mentioned, the following code generates a Style/AccessModifierDeclarations: module_function should not be inlined in method definitions. false positive:

module Kernel
  def foo(cmd)
  end
  module_function :foo
end

or

module Kernel
  module_function def foo(cmd)
  end
end

I know a lot of conversation regarding several separate issues has happened since my original complaint and it went stale at some point, but my original problem is still unsolved and I think it could be improved. I’m unable to use the cop because of extensive use of concord and anima and similar tools.

What I think should happen is the cop should determine what was defined directly in that scope and ignore any method definitions it can’t observe (ex: the #foo method in my example).

The module_function and attr_reader cases (possibly others) make sense to me, but I don’t think they’re the same problem I’m getting at which is that the cop has false positives when the method definition cannot be moved below the group.

@brandonweiss Hi, Brandon.

From your earlier comment:

Yes, #2 and #3 are not exactly the same, but they are essentially the same. They are both explicit and require an individual private declaration for each method you want to make private and it’s positioned inline, either before the method definition or before the symbol argument it takes.

I’m very confused by your assertion of “inline” describing both cases. Inserting the privacy modifier into the method declaration is what makes it “inline”. Using a separate privacy declaration, on its own line, is by definition not inline. Yes, both forms have the same target, but that’s beside the point: the method and privacy declarations do not share the same line.

It sounds to me like there’s a missing word here, as conflating #2 and #3 is not helpful. “Matched” maybe? Because you match a privacy (or module_function, etc.) declaration with a separately-declared method?

Hmm, this is getting confusing as everyone seems to be talking about slightly different things, but I’ll try to respond to all of them.

@dgollahon Yeah, that probably shouldn’t be causing an offense as there’s no actual way to fix that since the method is defined elsewhere. I think there are two potential approaches—if you think this is relatively uncommon usage you could just disable the cop for that line, or if you think this is going to happen a lot, as you said, you could alter the cop to try and determine if the method is statically defined when configured with group. This isn’t an issue for me personally as I’ve never needed to control the visibility of methods that I’m not explicitly defining, so feel free to have at it if you feel compelled!

@jodosha @phillycheeze In your example that seems to be a valid offense. If you want to use that style you should configure Style/AccessModifierDeclarations with inline. Or you can fix the offense by using the “group” style.

@dgollahon Yes, the inline style is both of these:

private def foo
end

def foo
end
private :foo

The cop doesn’t discriminate because those are both technically inline, at least as far as I was concerned. You could totally create a third style to differentiate the two, but it didn’t seem useful to me.

Same problem here:

def _run_before_callbacks(params)
end

private :_run_before_callbacks