Fix false positives for Style/AccessorGrouping when accessors have …

…different access modifiers
rubocop · Jul 7, 2020 · caef6bb · caef6bb
1 parent 85ff4c3
commit caef6bb
Show file tree

Hide file tree

Showing 6 changed files with 220 additions and 38 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Bug fixes
 
 * [#8252](https://github.com/rubocop-hq/rubocop/issues/8252): Fix a command line option name from `--safe-autocorrect` to `--safe-auto-correct`, which is compatible with RuboCop 0.86 and lower. ([@koic][])
+* [#8253](https://github.com/rubocop-hq/rubocop/issues/8253): Fix false positives for `Style/AccessorGrouping` when accessors have different access modifiers. ([@fatkodima][])
 * [#8257](https://github.com/rubocop-hq/rubocop/issues/8257): Fix an error for `Style/BisectedAttrAccessor` when using `attr_reader` and `attr_writer` with splat arguments. ([@fatkodima][])
 * [#8239](https://github.com/rubocop-hq/rubocop/pull/8239): Don't load `.rubocop.yml` from personal folders to check for exclusions if given a custom configuration file. ([@deivid-rodriguez][])
 * [#8256](https://github.com/rubocop-hq/rubocop/issues/8256): Fix an error for `--auto-gen-config` when running a cop who do not support auto-correction. ([@koic][])

diff --git a/lib/rubocop.rb b/lib/rubocop.rb
@@ -115,6 +115,7 @@
 require_relative 'rubocop/cop/mixin/trailing_comma'
 require_relative 'rubocop/cop/mixin/uncommunicative_name'
 require_relative 'rubocop/cop/mixin/unused_argument'
+require_relative 'rubocop/cop/mixin/visibility_help'
 
 require_relative 'rubocop/cop/utils/format_string'
 

diff --git a/lib/rubocop/cop/layout/class_structure.rb b/lib/rubocop/cop/layout/class_structure.rb
@@ -134,21 +134,18 @@ module Layout
       #
       # @see https://rubystyle.guide#consistent-classes
       class ClassStructure < Cop
+        include VisibilityHelp
+
         HUMANIZED_NODE_TYPE = {
           casgn: :constants,
           defs: :class_methods,
           def: :public_methods,
           sclass: :class_singleton
         }.freeze
 
-        VISIBILITY_SCOPES = %i[private protected public].freeze
         MSG = '`%<category>s` is supposed to appear before ' \
               '`%<previous>s`.'
 
-        def_node_matcher :visibility_block?, <<~PATTERN
-          (send nil? { :private :protected :public })
-        PATTERN
-
         # Validates code style on class declaration.
         # Add offense when find a node out of expected order.
         def on_class(class_node)
@@ -243,38 +240,6 @@ def ignore?(classification)
             expected_order.index(classification).nil?
         end
 
-        def node_visibility(node)
-          scope = find_visibility_start(node)
-          scope&.method_name || :public
-        end
-
-        def find_visibility_start(node)
-          left_siblings_of(node)
-            .reverse
-            .find(&method(:visibility_block?))
-        end
-
-        # Navigate to find the last protected method
-        def find_visibility_end(node)
-          possible_visibilities = VISIBILITY_SCOPES - [node_visibility(node)]
-          right = right_siblings_of(node)
-          right.find do |child_node|
-            possible_visibilities.include?(node_visibility(child_node))
-          end || right.last
-        end
-
-        def siblings_of(node)
-          node.parent.children
-        end
-
-        def right_siblings_of(node)
-          siblings_of(node)[node.sibling_index..-1]
-        end
-
-        def left_siblings_of(node)
-          siblings_of(node)[0, node.sibling_index]
-        end
-
         def humanize_node(node)
           if node.def_type?
             return :initializer if node.method?(:initialize)

diff --git a/lib/rubocop/cop/mixin/visibility_help.rb b/lib/rubocop/cop/mixin/visibility_help.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Help methods for determining node visibility.
+    module VisibilityHelp
+      extend NodePattern::Macros
+
+      VISIBILITY_SCOPES = %i[private protected public].freeze
+
+      private
+
+      def node_visibility(node)
+        scope = find_visibility_start(node)
+        scope&.method_name || :public
+      end
+
+      def find_visibility_start(node)
+        left_siblings_of(node)
+          .reverse
+          .find(&method(:visibility_block?))
+      end
+
+      # Navigate to find the last protected method
+      def find_visibility_end(node)
+        possible_visibilities = VISIBILITY_SCOPES - [node_visibility(node)]
+        right = right_siblings_of(node)
+        right.find do |child_node|
+          possible_visibilities.include?(node_visibility(child_node))
+        end || right.last
+      end
+
+      def left_siblings_of(node)
+        siblings_of(node)[0, node.sibling_index]
+      end
+
+      def right_siblings_of(node)
+        siblings_of(node)[node.sibling_index..-1]
+      end
+
+      def siblings_of(node)
+        node.parent.children
+      end
+
+      def_node_matcher :visibility_block?, <<~PATTERN
+        (send nil? { :private :protected :public })
+      PATTERN
+    end
+  end
+end
diff --git a/lib/rubocop/cop/style/accessor_grouping.rb b/lib/rubocop/cop/style/accessor_grouping.rb
@@ -33,6 +33,7 @@ module Style
       #
       class AccessorGrouping < Cop
         include ConfigurableEnforcedStyle
+        include VisibilityHelp
 
         GROUPED_MSG = 'Group together all `%<accessor>s` attributes.'
         SEPARATED_MSG = 'Use one attribute per `%<accessor>s`.'
@@ -46,6 +47,7 @@ def on_class(node)
             check(macro)
           end
         end
+        alias on_sclass on_class
         alias on_module on_class
 
         def autocorrect(node)
@@ -90,7 +92,9 @@ def separated_style?
 
         def sibling_accessors(send_node)
           send_node.parent.each_child_node(:send).select do |sibling|
-            sibling.macro? && sibling.method?(send_node.method_name)
+            accessor?(sibling) &&
+              sibling.method?(send_node.method_name) &&
+              node_visibility(sibling) == node_visibility(send_node)
           end
         end
 

diff --git a/spec/rubocop/cop/style/accessor_grouping_spec.rb b/spec/rubocop/cop/style/accessor_grouping_spec.rb
@@ -33,6 +33,89 @@ class Foo
       RUBY
     end
 
+    it 'registers an offense and corrects when using separated accessors with different access modifiers' do
+      expect_offense(<<~RUBY)
+        class Foo
+          attr_reader :bar1
+          ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          attr_reader :bar2
+          ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+
+          protected
+          attr_accessor :quux
+
+          private
+          attr_reader :baz1, :baz2
+          ^^^^^^^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          attr_writer :baz3
+          attr_reader :baz4
+          ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+
+          public
+          attr_reader :bar3
+          ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          other_macro :zoo
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Foo
+          attr_reader :bar1, :bar2, :bar3
+          
+
+          protected
+          attr_accessor :quux
+
+          private
+          attr_reader :baz1, :baz2, :baz4
+          attr_writer :baz3
+          
+
+          public
+          
+          other_macro :zoo
+        end
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using separated accessors within eigenclass' do
+      expect_offense(<<~RUBY)
+        class Foo
+          attr_reader :bar
+
+          class << self
+            attr_reader :baz1, :baz2
+            ^^^^^^^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+            attr_reader :baz3
+            ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+
+            private
+
+            attr_reader :quux1
+            ^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+            attr_reader :quux2
+            ^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          end
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Foo
+          attr_reader :bar
+
+          class << self
+            attr_reader :baz1, :baz2, :baz3
+            
+
+            private
+
+            attr_reader :quux1, :quux2
+            
+          end
+        end
+      RUBY
+    end
+
     it 'does not register an offense when using grouped accessors' do
       expect_no_offenses(<<~RUBY)
         class Foo
@@ -67,6 +150,84 @@ class Foo
       RUBY
     end
 
+    it 'registers an offense and corrects when using grouped accessors with different access modifiers' do
+      expect_offense(<<~RUBY)
+        class Foo
+          attr_reader :bar1, :bar2
+          ^^^^^^^^^^^^^^^^^^^^^^^^ Use one attribute per `attr_reader`.
+
+          protected
+          attr_accessor :quux
+
+          private
+          attr_reader :baz1, :baz2
+          ^^^^^^^^^^^^^^^^^^^^^^^^ Use one attribute per `attr_reader`.
+          attr_writer :baz3
+          attr_reader :baz4
+
+          public
+          attr_reader :bar3
+          other_macro :zoo
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Foo
+          attr_reader :bar1
+          attr_reader :bar2
+
+          protected
+          attr_accessor :quux
+
+          private
+          attr_reader :baz1
+          attr_reader :baz2
+          attr_writer :baz3
+          attr_reader :baz4
+
+          public
+          attr_reader :bar3
+          other_macro :zoo
+        end
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using grouped accessors within eigenclass' do
+      expect_offense(<<~RUBY)
+        class Foo
+          attr_reader :bar
+
+          class << self
+            attr_reader :baz1, :baz2
+            ^^^^^^^^^^^^^^^^^^^^^^^^ Use one attribute per `attr_reader`.
+            attr_reader :baz3
+
+            private
+
+            attr_reader :quux1, :quux2
+            ^^^^^^^^^^^^^^^^^^^^^^^^^^ Use one attribute per `attr_reader`.
+          end
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Foo
+          attr_reader :bar
+
+          class << self
+            attr_reader :baz1
+            attr_reader :baz2
+            attr_reader :baz3
+
+            private
+
+            attr_reader :quux1
+            attr_reader :quux2
+          end
+        end
+      RUBY
+    end
+
     it 'does not register an offense when using separated accessors' do
       expect_no_offenses(<<~RUBY)
         class Foo