Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style/AccessModifierDeclarations false positives #5953

Closed
dgollahon opened this issue Jun 6, 2018 · 34 comments
Closed

Style/AccessModifierDeclarations false positives #5953

dgollahon opened this issue Jun 6, 2018 · 34 comments

Comments

@dgollahon
Copy link
Contributor

@dgollahon dgollahon commented Jun 6, 2018

I just upgraded to the latest rubocop and received some peculiar offenses from Style/AccessModifierDeclarations offenses. I use a number of different tools that generate methods automatically, including anima, for example. But sometimes I apply a different access modifier to the generated method(s). Style/AccessModifierDeclarations flags this as "inlined", when it is not anything I would consider inline.


Expected behavior

I expect the following example (and various similar cases) not to raise an offense with the default rubocop configuration.

class Foo
  include Anima.new(:foo) # Defines constructor which takes the single keyword argument :foo and generates a public reader.

  private :foo
end

If, for some reason, this is the intended behavior and I'm missing something here, I think the error message should be made clearer. I think this cop should ignore access modifier adjustments to methods it can't statically detect.

Actual behavior

test.rb:4:3: C: Style/AccessModifierDeclarations: private should not be inlined in method definitions.
  private :foo
  ^^^^^^^

Steps to reproduce the problem

Run rubocop with Style/AccessModifierDeclarations's style set to "group" (the default).

RuboCop version

$ rubocop -V
0.57.0 (using Parser 2.5.1.0, running on ruby 2.5.1 x86_64-darwin17)
@jodosha
Copy link

@jodosha jodosha commented Jun 6, 2018

Same problem here:

def _run_before_callbacks(params)
end

private :_run_before_callbacks
@phillycheeze
Copy link

@phillycheeze phillycheeze commented Jun 6, 2018

Same is happening for me using module_function

Weird thing is if I try to disable the cop within source code around the method, I get a Lint/UnneededCopDisableDirective error!

@dgollahon
Copy link
Contributor Author

@dgollahon dgollahon commented Jun 6, 2018

I wonder if what @jodosha is doing should be a style option. It's logically similar to doing

private def _run_before_callbacks(params)
end

as if he were doing the inline style, but just choosing to put the private annotation elsewhere. It is different from the typical private region, anyway.

In my case I simply can't use the normal stateful / region modifier and I also cannot place it at the method definition site because there isn't one that's accessible to me.

@jodosha
Copy link

@jodosha jodosha commented Jun 7, 2018

@dgollahon There is a new cop released with 0.57.0 that discourage private def style: see #5444

@dgollahon
Copy link
Contributor Author

@dgollahon dgollahon commented Jun 8, 2018

@jodosha I think perhaps we're talking past each other. I'm aware of #5444, as it's is the cop I'm opening the issue about. My understanding is the intent is to differentiate between having one private keyword per method v.s. one per class / group of methods.

I was suggesting that within what that cop is trying to support you might expect to find configuration for both

private def foo
end

as currently enforced and

def foo
end
private :foo

which are similar in intent (put the modifier right next to the method). I'm not sure anybody actually religiously does the latter though.

My main point in opening this issue is that my problem is none of the above--I simply don't have a method to put the modifier next to nor do I have a way to have a no-argument region style modifier apply to the generated method.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 8, 2018

@brandonweiss
Copy link
Contributor

@brandonweiss brandonweiss commented Jun 8, 2018

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.

@roberts1000
Copy link
Contributor

@roberts1000 roberts1000 commented Jun 12, 2018

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
cupakromer added a commit to RadiusNetworks/radius-spec that referenced this issue Jun 14, 2018
This Rubocop version adds several new cops:

  - `Rails/BulkChangeTable`

    I wasn't aware of the bulk change table being a more ideal choice
    due to it's bundling of the operation so nice to see that cop added.
  - `Style/AccessModifierDeclarations`

    This is a controversial style cop. The vast majority of Ruby code
    I've seen, and written, using the grouping style. While this cop
    defaults to that style it was written with the intention of forcing
    the alternative inline style. Additionally, the cop has a [🐛
    which flags instances which are impossible to write using the inline
    style](see rubocop/rubocop#5953).

    The author of the cop has two arguments against the group style:

      - in big methods someone has to scroll to see the modifier and
        even then it can be lost (as the modifier is indented the same
        level as the methods in their examples)

      - class methods listed under non-public modifiers are still public

    Let me attempt to address the first point. We have set our style to
    outdent access modifiers to better highlight the groupings. However,
    I still admit that large classes can cause such scrolling issues;
    esp. classes which have lots of documentation.

    Conversely, many IDEs have built-in features or plugins available
    which can fold code, shortening the file, and show a sidebar with
    the class method overview including modifier annotations. I
    personally use vim so I don't have that benefit but it does exist.
    Additionally, I would argue that the grouping style better forces
    grouping methods by access which the inline style would not. IMO the
    grouping makes it easier see at a glance how much of an API is
    public; which can affect how quickly I can find something usable
    within the class.

    I also worry, that Ruby programmers, which are used to the grouping
    style (which is sort of the Ruby default) will accidentally make
    things public without realizing it. Since public is the default
    access and this cop does not force an explicit use of `public` in
    the inline style it is a potential risk. Given I haven't used this
    style maybe this isn't really an issue.

    Regarding the second point, there is already a cop which protects
    against that exact issue: `Lint/IneffectiveAccessModifier`. Reading
    the feature discussion there was a bit of talk around this. The
    argument in-favor of the inline style helping to avoid it was
    largely framed as:

      > If `Lint/IneffectiveAccessModifier` is disabled this cop does
      > both. So you just need this cop.

    I am generally in-favor of styles which double as error protection
    and/or contribute additional semantic meaning. However, I'm not
    ready to get behind the inline style yet. Part of me feels the main
    complaint point, regarding scrolling in large files, is more an
    attempt to ease a symptom of violating SRP / composition.

    Also, after reviewing several of our projects there are virtually
    zero violations of the grouping style. Changing to force the inline
    style would be a massive undertaking (as the cop does not support
    autocorrect).

    NOTE: We have one project that mixes styles. As I am the main author
    of the code mixing styles I am leaving this cop enabled with the
    default grouping for now until I can better audit the impact.

  - `Style/UnneededCondition`
  - `Layout/ClosingHeredocIndentation`
  - `Layout/LeadingBlankLines`

This version also includes a fix for `Rails/HttpPositionalArguments`
which supports `headers` and `env` in the kwargs.
@Nondv
Copy link

@Nondv Nondv commented Jun 20, 2018

@brandonweiss What if I have something like

def foo
end

whatever
something_else

def bar
end

private :foo

Will you think that this is inline too?
I don't know for sure but I think that people don't use your second "inline" style. But it's pretty common (I guess) to face issue from first post when method is defined elsewhere.

@brandonweiss
Copy link
Contributor

@brandonweiss brandonweiss commented Jun 20, 2018

@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…

@colszowka
Copy link
Contributor

@colszowka colszowka commented Jun 25, 2018

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 #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)

@pboling
Copy link

@pboling pboling commented Jun 29, 2018

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
wata727 added a commit to wata727/rubocop that referenced this issue Jul 1, 2018
Follow up of rubocop#5953, rubocop#6032

I think it is too difficult to enforce `group` (default) style
by default because it doesn't work for some methods.

For example, `def_delegator` doesn't work when using `group` style:

```ruby
require 'forwardable'

class Cat
  def meow
    puts "Meow!"
  end
end

class CatCage
  extend Forwardable

  def initialize(cat)
    @cat = cat
  end

  private

  def_delegator :@cat, :meow
end
```

```
[8] pry(main)> CatCage.new(Cat.new).meow
Meow!
=> nil
```

This is because `def_delegator` defines a method by `self.module_eval`.
Forcing group styles by default may be misleading for the above problems.

Maybe it is needed to add such methods to the whitelist and exclude
them from inspection, but it seems to be difficult for me.

First of all, I think it should make this cop disabled by default
and help users confused by this cop. WDYT? @brandonweiss
wata727 added a commit to wata727/rubocop that referenced this issue Jul 1, 2018
Follow up of rubocop#5953, rubocop#6032

I think it is too difficult to enforce `group` (default) style
by default because it doesn't work for some methods.

For example, `def_delegator` doesn't work when using `group` style:

```ruby
require 'forwardable'

class Cat
  def meow
    puts "Meow!"
  end
end

class CatCage
  extend Forwardable

  def initialize(cat)
    @cat = cat
  end

  private

  def_delegator :@cat, :meow
end
```

```
[8] pry(main)> CatCage.new(Cat.new).meow
Meow!
=> nil
```

This is because `def_delegator` defines a method by `self.module_eval`.
Forcing group styles by default may be misleading for the above problems.

Maybe it is needed to add such methods to the whitelist and exclude
them from inspection, but it seems to be difficult for me.

First of all, I think it should make this cop disabled by default
and help users confused by this cop. WDYT? @brandonweiss
@brandonweiss
Copy link
Contributor

@brandonweiss brandonweiss commented Jul 2, 2018

@pboling I posted a comment on the other issue that should hopefully explain the reason!

@stefan-kolb
Copy link

@stefan-kolb stefan-kolb commented Jul 25, 2018

Does happen for me, too although it is a really unfortunate case.

FactoryBot.define do
  factory :hosting, class: Profiles::Hosting do
    # mandatory properties
    public true
    private false
  end
end
floehopper added a commit to freerange/mocha that referenced this issue Jul 26, 2018
This cop was only introduced relatively recently and it seems to have
stirred up some controversy [1].

It was being triggered in many of the acceptance tests where anonymous
classes are defined within a test and their methods' visibility is
specified "inline".

[1]: rubocop/rubocop#5953
floehopper added a commit to freerange/mocha that referenced this issue Jul 26, 2018
This cop was only introduced relatively recently and it seems to have
stirred up some controversy [1].

It was being triggered in many of the acceptance tests where anonymous
classes are defined within a test and their methods' visibility is
specified "inline".

[1]: rubocop/rubocop#5953
Greyschale added a commit to gocardless/gc_ruboconfig that referenced this issue Aug 9, 2018
There is a bug in it logged here:
rubocop/rubocop#5953
@TrevorBramble
Copy link

@TrevorBramble TrevorBramble commented Dec 6, 2018

@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?

@brandonweiss
Copy link
Contributor

@brandonweiss brandonweiss commented Dec 6, 2018

@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.

@kke
Copy link

@kke kke commented Dec 20, 2018

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

@pboling
Copy link

@pboling pboling commented Dec 20, 2018

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?

kyrylo added a commit to pry/pry that referenced this issue Mar 7, 2019
This is a false positive documented in:
rubocop/rubocop#5953
paulfioravanti added a commit to paulfioravanti/survey_tool_ruby that referenced this issue Apr 9, 2019
…positive: rubocop/rubocop#5953
@stale
Copy link

@stale stale bot commented May 8, 2019

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 stale bot added the stale label May 8, 2019
@luke-hill
Copy link

@luke-hill luke-hill commented May 31, 2019

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.

@stale stale bot removed the stale label May 31, 2019
@dgollahon
Copy link
Contributor Author

@dgollahon dgollahon commented Jun 18, 2019

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.

@pboling
Copy link

@pboling pboling commented Jun 19, 2019

I would argue that module_function and attr_reader tripping this is also a false positive, since it is not code that should be flagged as outside of best practice, even if the reasons for the flagging may be different.

@dgollahon
Copy link
Contributor Author

@dgollahon dgollahon commented Jun 19, 2019

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.

@luke-hill
Copy link

@luke-hill luke-hill commented Jul 2, 2019

☝️ does this seem a fair compromise rubocop devs?

@AlexWayfer
Copy link
Contributor

@AlexWayfer AlexWayfer commented Jul 26, 2019

Our case:

def cell(cell_name, &block)
  define_method cell_name, block || instance_method(cell_name)
  private cell_name
end
@stale
Copy link

@stale stale bot commented Jan 22, 2020

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 stale bot added the stale label Jan 22, 2020
@luke-hill
Copy link

@luke-hill luke-hill commented Jan 22, 2020

Could we maybe get the compromise mentioned above worked on?

@stale stale bot removed the stale label Jan 22, 2020
@pboling
Copy link

@pboling pboling commented Feb 23, 2020

If this cop would support an exclusion list I could turn it back on.

@andrewgarner
Copy link

@andrewgarner andrewgarner commented Jun 3, 2020

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
@matthewhively
Copy link

@matthewhively matthewhively commented Aug 20, 2020

Why does this keep getting marked as stale? The cop is wonky and nothing appears to have changed from 0.57 => 0.89 (current)
Bugs don't just fix themselves (usually).
</rant>

I'm going to go ahead and disable the cop in my configuration until someone posts a better solution.

@AlexWayfer
Copy link
Contributor

@AlexWayfer AlexWayfer commented Aug 20, 2020

Why does this keep getting marked as stale?

image

image

Sorry, what? If you're talking about regular stale labels — it's just an automation, it doesn't count something else, like importance, except dates of the last comment.

I'm going to go ahead and disable the cop in my configuration until someone posts a better solution.

It's OK, especially inline, especially with the link to this issue.

@luke-hill
Copy link

@luke-hill luke-hill commented Mar 23, 2021

I'll give this a bump as it's been a while.

koic added a commit to koic/rubocop that referenced this issue Mar 23, 2021
…arations`

Fixes rubocop#5953.

This PR fixes a false positive for `Style/AccessModifierDeclarations`
when using `module_function` with symbol.
koic added a commit to koic/rubocop that referenced this issue Mar 23, 2021
…arations`

Fixes rubocop#5953.

This PR fixes a false positive for `Style/AccessModifierDeclarations`
when using `module_function` with symbol.
@koic koic closed this in #9632 Mar 24, 2021
koic added a commit that referenced this issue Mar 24, 2021
…ess_modifier_declarations

[Fix #5953] Fix a false positive for `Style/AccessModifierDeclarations`
@luke-hill
Copy link

@luke-hill luke-hill commented Mar 24, 2021

@koic Given the solution to this proposed and agreed by a fair few isn't what has happened. Should I create a breakout ticket for adding the new functionality.

Only run this cop on def methods

@pboling
Copy link

@pboling pboling commented Mar 24, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.