Lint/UselessAccessModifier doesn't respect active support's concerning since 0.40.0

[maxjacobson]

Hello again after a while!

I'm upgrading a Rails app from RuboCop v0.39.0 to v0.40.0 and noticed some new Lint/UselessAccessModifier offenses appeared in code which looks like this:

# gem install active_support
require 'active_support/core_ext/module/concerning'

class Dog
  concerning :Barking do
    def woof
      puts woof_message
    end

    private

    def woof_message
      "woof!"
    end
  end

  concerning :BeingAGoodBoy do
    def good_boy?
      ate? && !peed_on_floor?
    end

    private

    def ate?
      [true, false].sample
    end

    def peed_on_floor?
      [true, false].sample
    end
  end
end

In this case, RuboCop thinks the second private isn't doing anything, but it actually does make the subsequent methods in the concerning block private.

[amilligan]

We're having this same problem with a vanilla ActiveSupport::Concern. e.g.:

module Foo
  extend ActiveSupport::Concern

  class_methods do
    private
    ...
  end

  private

  def bar
  ...
  end
end

It appears the new implementation is unable to see that the first `private` only applies inside the block.

[savef]

I think this has already been fixed in master here.

[jonas054]

@savef It seems that fix was only for blocks given to new. I still get a false positive from Lint/UselessAccessModifier when checking @maxjacobson's example code. The problem started with #3060.

[savef]

Ah! Sorry. I only tested it on my own false positive (which was a Struct.new one).

[maxjacobson]

Thanks for double checking that.

I'm happy to try and make a fix here, although maybe I should loop in @owst and see if he's aware and already doing so.

[owst]

I wasn't aware of this, and I'm not already looking so feel free to go ahead @maxjacobson!

I would guess that adding a new matcher for concerning and class_methods to (a suitable renaming of) instance_eval_or_new_call? would do the trick (since we want to treat the blocks passed to those methods as new scopes).

However, I'm not sure if we should care that this would be ActiveSupport-specific code in a non-Rails cop, hopefully not!

[maxjacobson]

Fixed in #3186