Add new Style/AccessorGrouping cop (#8241)

Co-authored-by: Bozhidar Batsov <bozhidar@batsov.com>

.rubocop.yml

@@ -24,6 +24,11 @@ Naming/PredicateName:
     - def_node_matcher
     - def_node_search
 
+Style/AccessorGrouping:
+  Exclude:
+    - lib/rubocop/formatter/base_formatter.rb
+    - lib/rubocop/cop/offense.rb
+
 Style/FormatStringToken:
   # Because we parse a lot of source codes from strings. Percent arrays
   # look like unannotated format string tokens to this cop.

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### New features
 
 * [#7868](https://github.com/rubocop-hq/rubocop/pull/7868): `Cop::Base` is the new recommended base class for cops. ([@marcandre][])
+* [#3983](https://github.com/rubocop-hq/rubocop/issues/3983): Add new `Style/AccessorGrouping` cop. ([@fatkodima][])
 * [#8244](https://github.com/rubocop-hq/rubocop/pull/8244): Add new `Style/BisectedAttrAccessor` cop. ([@fatkodima][])
 * [#7458](https://github.com/rubocop-hq/rubocop/issues/7458): Add new `AsciiConstants` option for `Naming/AsciiIdentifiers`. ([@fatkodima][])
 * [#7373](https://github.com/rubocop-hq/rubocop/issues/7373): Add new `Style/RedundantAssignment` cop. ([@fatkodima][])

config/default.yml

@@ -2299,6 +2299,17 @@ Style/AccessModifierDeclarations:
     - group
   AllowModifiersOnSymbols: true
 
+Style/AccessorGrouping:
+  Description: 'Checks for grouping of accessors in `class` and `module` bodies.'
+  Enabled: 'pending'
+  VersionAdded: '0.87'
+  EnforcedStyle: grouped
+  SupportedStyles:
+    # separated: each accessor goes in a separate statement.
+    # grouped: accessors are grouped into a single statement.
+    - separated
+    - grouped
+
 Style/Alias:
   Description: 'Use alias instead of alias_method.'
   StyleGuide: '#alias-method-lexically'

docs/modules/ROOT/pages/cops.adoc

@@ -313,6 +313,7 @@ In the following section you find all available cops:
 === Department xref:cops_style.adoc[Style]
 
 * xref:cops_style.adoc#styleaccessmodifierdeclarations[Style/AccessModifierDeclarations]
+* xref:cops_style.adoc#styleaccessorgrouping[Style/AccessorGrouping]
 * xref:cops_style.adoc#stylealias[Style/Alias]
 * xref:cops_style.adoc#styleandor[Style/AndOr]
 * xref:cops_style.adoc#stylearrayjoin[Style/ArrayJoin]

docs/modules/ROOT/pages/cops_style.adoc

@@ -104,6 +104,66 @@ end
 | Boolean
 |===
 
+== Style/AccessorGrouping
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 0.87
+| -
+|===
+
+This cop checks for grouping of accessors in `class` and `module` bodies.
+By default it enforces accessors to be placed in grouped declarations,
+but it can be configured to enforce separating them in multiple declarations.
+
+=== Examples
+
+==== EnforcedStyle: grouped (default)
+
+[source,ruby]
+----
+# bad
+class Foo
+  attr_reader :bar
+  attr_reader :baz
+end
+
+# good
+class Foo
+  attr_reader :bar, :baz
+end
+----
+
+==== EnforcedStyle: separated
+
+[source,ruby]
+----
+# bad
+class Foo
+  attr_reader :bar, :baz
+end
+
+# good
+class Foo
+  attr_reader :bar
+  attr_reader :baz
+end
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| EnforcedStyle
+| `grouped`
+| `separated`, `grouped`
+|===
+
 == Style/Alias
 
 |===

lib/rubocop.rb

@@ -356,6 +356,7 @@
 require_relative 'rubocop/cop/naming/variable_number'
 
 require_relative 'rubocop/cop/style/access_modifier_declarations'
+require_relative 'rubocop/cop/style/accessor_grouping'
 require_relative 'rubocop/cop/style/alias'
 require_relative 'rubocop/cop/style/and_or'
 require_relative 'rubocop/cop/style/array_join'

lib/rubocop/cop/layout/hash_alignment.rb

@@ -219,8 +219,7 @@ def autocorrect(node)
           correct_node(node, delta)
         end
 
-        attr_accessor :offences_by
-        attr_accessor :column_deltas
+        attr_accessor :offences_by, :column_deltas
 
         private
 

lib/rubocop/cop/style/accessor_grouping.rb

@@ -0,0 +1,136 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Style
+      # This cop checks for grouping of accessors in `class` and `module` bodies.
+      # By default it enforces accessors to be placed in grouped declarations,
+      # but it can be configured to enforce separating them in multiple declarations.
+      #
+      # @example EnforcedStyle: grouped (default)
+      #   # bad
+      #   class Foo
+      #     attr_reader :bar
+      #     attr_reader :baz
+      #   end
+      #
+      #   # good
+      #   class Foo
+      #     attr_reader :bar, :baz
+      #   end
+      #
+      # @example EnforcedStyle: separated
+      #   # bad
+      #   class Foo
+      #     attr_reader :bar, :baz
+      #   end
+      #
+      #   # good
+      #   class Foo
+      #     attr_reader :bar
+      #     attr_reader :baz
+      #   end
+      #
+      class AccessorGrouping < Cop
+        include ConfigurableEnforcedStyle
+
+        GROUPED_MSG = 'Group together all `%<accessor>s` attributes.'
+        SEPARATED_MSG = 'Use one attribute per `%<accessor>s`.'
+
+        ACCESSOR_METHODS = %i[attr_reader attr_writer attr_accessor attr].freeze
+
+        def on_class(node)
+          class_send_elements(node).each do |macro|
+            next unless accessor?(macro)
+
+            check(macro)
+          end
+        end
+        alias on_module on_class
+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(node, correction(node))
+          end
+        end
+
+        private
+
+        def class_send_elements(class_node)
+          class_def = class_node.body
+
+          if !class_def || class_def.def_type?
+            []
+          elsif class_def.send_type?
+            [class_def]
+          else
+            class_def.each_child_node(:send).to_a
+          end
+        end
+
+        def accessor?(send_node)
+          send_node.macro? && ACCESSOR_METHODS.include?(send_node.method_name)
+        end
+
+        def check(send_node)
+          if grouped_style? && sibling_accessors(send_node).size > 1
+            add_offense(send_node)
+          elsif separated_style? && send_node.arguments.size > 1
+            add_offense(send_node)
+          end
+        end
+
+        def grouped_style?
+          style == :grouped
+        end
+
+        def separated_style?
+          style == :separated
+        end
+
+        def sibling_accessors(send_node)
+          send_node.parent.each_child_node(:send).select do |sibling|
+            sibling.macro? && sibling.method?(send_node.method_name)
+          end
+        end
+
+        def message(send_node)
+          msg = grouped_style? ? GROUPED_MSG : SEPARATED_MSG
+          format(msg, accessor: send_node.method_name)
+        end
+
+        def correction(node)
+          if grouped_style?
+            accessors = sibling_accessors(node)
+            if node == accessors.first
+              group_accessors(node, accessors)
+            else
+              ''
+            end
+          else
+            separate_accessors(node)
+          end
+        end
+
+        def group_accessors(node, accessors)
+          accessor_names = accessors.flat_map do |accessor|
+            accessor.arguments.map(&:source)
+          end
+
+          "#{node.method_name} #{accessor_names.join(', ')}"
+        end
+
+        def separate_accessors(node)
+          node.arguments.map do |arg|
+            if arg == node.arguments.first
+              "#{node.method_name} #{arg.source}"
+            else
+              indent = ' ' * node.loc.column
+              "#{indent}#{node.method_name} #{arg.source}"
+            end
+          end.join("\n")
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/utils/format_string.rb

@@ -41,8 +41,7 @@ class FormatString
         #
         # @see https://ruby-doc.org/core-2.6.3/Kernel.html#method-i-format
         class FormatSequence
-          attr_reader :begin_pos, :end_pos
-          attr_reader :flags, :width, :precision, :name, :type
+          attr_reader :begin_pos, :end_pos, :flags, :width, :precision, :name, :type
 
           def initialize(match)
             @source = match[0]

lib/rubocop/rake_task.rb

@@ -12,13 +12,7 @@ module RuboCop
   # Use global Rake namespace here to avoid namespace issues with custom
   # rubocop-rake tasks
   class RakeTask < ::Rake::TaskLib
-    attr_accessor :name
-    attr_accessor :verbose
-    attr_accessor :fail_on_error
-    attr_accessor :patterns
-    attr_accessor :formatters
-    attr_accessor :requires
-    attr_accessor :options
+    attr_accessor :name, :verbose, :fail_on_error, :patterns, :formatters, :requires, :options
 
     def initialize(name = :rubocop, *args, &task_block)
       setup_ivars(name)

spec/rubocop/cop/style/accessor_grouping_spec.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Style::AccessorGrouping, :config do
+  subject(:cop) { described_class.new(config) }
+
+  context 'when EnforcedStyle is grouped' do
+    let(:cop_config) do
+      { 'EnforcedStyle' => 'grouped' }
+    end
+
+    it 'registers an offense and corrects when using separated accessors' do
+      expect_offense(<<~RUBY)
+        class Foo
+          attr_reader :bar1
+          ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          attr_reader :bar2
+          ^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          attr_accessor :quux
+          attr_reader :bar3, :bar4
+          ^^^^^^^^^^^^^^^^^^^^^^^^ Group together all `attr_reader` attributes.
+          other_macro :zoo
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Foo
+          attr_reader :bar1, :bar2, :bar3, :bar4
+          
+          attr_accessor :quux
+          
+          other_macro :zoo
+        end
+      RUBY
+    end
+
+    it 'does not register an offense when using grouped accessors' do
+      expect_no_offenses(<<~RUBY)
+        class Foo
+          attr_reader :bar, :baz
+        end
+      RUBY
+    end
+  end
+
+  context 'when EnforcedStyle is separated' do
+    let(:cop_config) do
+      { 'EnforcedStyle' => 'separated' }
+    end
+
+    it 'registers an offense and corrects when using grouped accessors' do
+      expect_offense(<<~RUBY)
+        class Foo
+          attr_reader :bar, :baz
+          ^^^^^^^^^^^^^^^^^^^^^^ Use one attribute per `attr_reader`.
+          attr_accessor :quux
+          other_macro :zoo, :woo
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class Foo
+          attr_reader :bar
+          attr_reader :baz
+          attr_accessor :quux
+          other_macro :zoo, :woo
+        end
+      RUBY
+    end
+
+    it 'does not register an offense when using separated accessors' do
+      expect_no_offenses(<<~RUBY)
+        class Foo
+          attr_reader :bar
+          attr_reader :baz
+        end
+      RUBY
+    end
+  end
+end