From 295b9a6d3143be3b66dc3f769bb227753f3543cd Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Tue, 15 Sep 2020 00:37:41 -0400 Subject: [PATCH] [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 --- CHANGELOG.md | 1 + lib/rubocop/cop/mixin/comments_help.rb | 12 +- .../cop/style/class_methods_definitions.rb | 58 +++++--- .../style/class_methods_definitions_spec.rb | 132 ++++++++++++++++-- 4 files changed, 165 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49dc1dff9e1..cf6d796d435 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/lib/rubocop/cop/mixin/comments_help.rb b/lib/rubocop/cop/mixin/comments_help.rb index 76db20f2080..6599f9ebf4d 100644 --- a/lib/rubocop/cop/mixin/comments_help.rb +++ b/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 diff --git a/lib/rubocop/cop/style/class_methods_definitions.rb b/lib/rubocop/cop/style/class_methods_definitions.rb index 8c2589e9695..48b642880d8 100644 --- a/lib/rubocop/cop/style/class_methods_definitions.rb +++ b/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 `%s` to define class method.' + MSG = 'Use `%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 diff --git a/spec/rubocop/cop/style/class_methods_definitions_spec.rb b/spec/rubocop/cop/style/class_methods_definitions_spec.rb index 21e9cc52418..ae7dd5e95cb 100644 --- a/spec/rubocop/cop/style/class_methods_definitions_spec.rb +++ b/spec/rubocop/cop/style/class_methods_definitions_spec.rb @@ -10,10 +10,10 @@ 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 @@ -21,13 +21,12 @@ def three 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,12 +35,12 @@ 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 @@ -49,14 +48,100 @@ def two 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