[Fix #8663] Fix issues with Style/ClassMethodsDefinitions (#8687)

Autocorrection was rewritten, which notably moves the offense for def_self style to the sclass itself, in order to properly handle each method within it without getting clobbering errors.

Autocorrection fixes:
* Handle multiple methods within class << self
* Remove extra whitespace
* Fix indentation
* Remove self << class if empty

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

CHANGELOG.md

@@ -44,6 +44,7 @@
 * [#8617](https://github.com/rubocop-hq/rubocop/issues/8617): Fix `Style/HashAsLastArrayItem` to not register an offense when all items in an array are hashes. ([@dvandersluis][])
 * [#8500](https://github.com/rubocop-hq/rubocop/issues/8500): Add `in?` to AllowedMethods for `Lint/SafeNavigationChain` cop. ([@tejasbubane][])
 * [#8629](https://github.com/rubocop-hq/rubocop/pull/8629): Fix the cache being reusable in CI by using crc32 to calculate file hashes rather than `mtime`, which changes each CI build. ([@dvandersluis][])
+* [#8663](https://github.com/rubocop-hq/rubocop/issues/8663): Fix multiple autocorrection bugs with `Style/ClassMethodsDefinitions`. ([@dvandersluis][])
 * [#8621](https://github.com/rubocop-hq/rubocop/issues/8621): Add helpful Infinite Loop error message. ([@iSarCasm][])
 
 ## 0.90.0 (2020-09-01)

lib/rubocop/cop/mixin/comments_help.rb

@@ -7,15 +7,9 @@ module CommentsHelp
       include VisibilityHelp
 
       def source_range_with_comment(node)
-        begin_pos, end_pos =
-          if node.def_type?
-            start_node = find_visibility_start(node) || node
-            end_node = find_visibility_end(node) || node
-            [begin_pos_with_comment(start_node),
-             end_position_for(end_node) + 1]
-          else
-            [begin_pos_with_comment(node), end_position_for(node)]
-          end
+        begin_pos = begin_pos_with_comment(node)
+        end_pos = end_position_for(node)
+        end_pos += 1 if node.def_type?
 
         Parser::Source::Range.new(buffer, begin_pos, end_pos)
       end

lib/rubocop/cop/style/class_methods_definitions.rb

@@ -62,22 +62,19 @@ class ClassMethodsDefinitions < Base
         include ConfigurableEnforcedStyle
         include CommentsHelp
         include VisibilityHelp
+        include RangeHelp
         extend AutoCorrector
 
-        MSG = 'Use `%<preferred>s` to define class method.'
+        MSG = 'Use `%<preferred>s` to define a class method.'
+        MSG_SCLASS = 'Do not define public methods within class << self.'
 
         def on_sclass(node)
           return unless def_self_style?
           return unless node.identifier.source == 'self'
-          return if contains_non_public_methods?(node)
+          return unless all_methods_public?(node)
 
-          def_nodes(node).each do |def_node|
-            next unless node_visibility(def_node) == :public
-
-            message = format(MSG, preferred: "def self.#{def_node.method_name}")
-            add_offense(def_node, message: message) do |corrector|
-              extract_def_from_sclass(def_node, node, corrector)
-            end
+          add_offense(node, message: MSG_SCLASS) do |corrector|
+            autocorrect_sclass(node, corrector)
           end
         end
 
@@ -94,8 +91,11 @@ def def_self_style?
           style == :def_self
         end
 
-        def contains_non_public_methods?(sclass_node)
-          def_nodes(sclass_node).any? { |def_node| node_visibility(def_node) != :public }
+        def all_methods_public?(sclass_node)
+          def_nodes = def_nodes(sclass_node)
+          return false if def_nodes.empty?
+
+          def_nodes.all? { |def_node| node_visibility(def_node) == :public }
         end
 
         def def_nodes(sclass_node)
@@ -111,19 +111,45 @@ def def_nodes(sclass_node)
           end
         end
 
-        def extract_def_from_sclass(def_node, sclass_node, corrector)
+        def autocorrect_sclass(node, corrector)
+          rewritten_defs = []
+
+          def_nodes(node).each do |def_node|
+            next unless node_visibility(def_node) == :public
+
+            range, source = extract_def_from_sclass(def_node, node)
+
+            corrector.remove(range)
+            rewritten_defs << source
+          end
+
+          if sclass_only_has_methods?(node)
+            corrector.remove(node)
+            rewritten_defs.first&.strip!
+          else
+            corrector.insert_after(node, "\n")
+          end
+
+          corrector.insert_after(node, rewritten_defs.join("\n"))
+        end
+
+        def sclass_only_has_methods?(node)
+          node.body.def_type? || node.body.each_child_node.all?(&:def_type?)
+        end
+
+        def extract_def_from_sclass(def_node, sclass_node)
           range = source_range_with_comment(def_node)
           source = range.source.sub!(
             "def #{def_node.method_name}",
             "def self.#{def_node.method_name}"
           )
 
-          corrector.insert_before(sclass_node, "#{source}\n#{indent(sclass_node)}")
-          corrector.remove(range)
+          source = source.gsub(/^ {#{indentation_diff(def_node, sclass_node)}}/, '')
+          [range, source.chomp]
         end
 
-        def indent(node)
-          ' ' * node.loc.column
+        def indentation_diff(node1, node2)
+          node1.loc.column - node2.loc.column
         end
       end
     end

spec/rubocop/cop/style/class_methods_definitions_spec.rb

@@ -10,24 +10,23 @@
       expect_offense(<<~RUBY)
         class A
           class << self
+          ^^^^^^^^^^^^^ Do not define public methods within class << self.
             attr_reader :two
 
             def three
-            ^^^^^^^^^ Use `def self.three` to define class method.
             end
           end
         end
       RUBY
 
       expect_correction(<<~RUBY)
         class A
-         #{trailing_whitespace}
-            def self.three
-            end
-
           class << self
             attr_reader :two
           end
+
+          def self.three
+          end
         end
       RUBY
     end
@@ -36,27 +35,113 @@ class << self
       expect_offense(<<~RUBY)
         class A
           class << self
+          ^^^^^^^^^^^^^ Do not define public methods within class << self.
             attr_reader :one
 
             # Multiline
             # comment.
             def two
-            ^^^^^^^ Use `def self.two` to define class method.
             end
           end
         end
       RUBY
 
       expect_correction(<<~RUBY)
         class A
-         #{trailing_whitespace}
-            # Multiline
-            # comment.
-            def self.two
+          class << self
+            attr_reader :one
+          end
+
+          # Multiline
+          # comment.
+          def self.two
+          end
+        end
+      RUBY
+    end
+
+    it 'correctly handles class << self containing multiple methods' do
+      expect_offense(<<~RUBY)
+        class A
+          class << self
+          ^^^^^^^^^^^^^ Do not define public methods within class << self.
+            def one
+              :one
             end
 
+            def two
+              :two
+            end
+          end
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class A
+          def self.one
+            :one
+          end
+
+          def self.two
+            :two
+          end
+        end
+      RUBY
+    end
+
+    it 'removes empty class << self when correcting' do
+      expect_offense(<<~RUBY)
+        class A
+          def self.one
+          end
+
           class << self
-            attr_reader :one
+          ^^^^^^^^^^^^^ Do not define public methods within class << self.
+            def two
+            end
+
+            def three
+            end
+          end
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class A
+          def self.one
+          end
+
+          def self.two
+          end
+
+          def self.three
+          end
+        end
+      RUBY
+    end
+
+    it 'correctly handles def self.x within class << self' do
+      expect_offense(<<~RUBY)
+        class A
+          class << self
+          ^^^^^^^^^^^^^ Do not define public methods within class << self.
+            def self.one
+            end
+
+            def two
+            end
+          end
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class A
+          class << self
+            def self.one
+            end
+          end
+
+          def self.two
           end
         end
       RUBY
@@ -71,13 +156,23 @@ def one
 
             private
 
-            def one
+            def two
             end
           end
         end
       RUBY
     end
 
+    it 'does not register an offense when class << self does not contain methods' do
+      expect_no_offenses(<<~RUBY)
+        class A
+          class << self
+            attr_reader :one
+          end
+        end
+      RUBY
+    end
+
     it 'does not register an offense when defining class methods with `def self.method`' do
       expect_no_offenses(<<~RUBY)
         class A
@@ -97,6 +192,17 @@ def one
         end
       RUBY
     end
+
+    it 'does not register an offense when class << self contains only class methods' do
+      expect_no_offenses(<<~RUBY)
+        class A
+          class << self
+            def self.one
+            end
+          end
+        end
+      RUBY
+    end
   end
 
   context 'when EnforcedStyle is self_class' do
@@ -108,7 +214,7 @@ def one
       expect_offense(<<~RUBY)
         class A
           def self.one
-          ^^^^^^^^^^^^ Use `class << self` to define class method.
+          ^^^^^^^^^^^^ Use `class << self` to define a class method.
           end
         end
       RUBY