diff --git a/changelog/fix_fixed_stylebisectedattraccessor.md b/changelog/fix_fixed_stylebisectedattraccessor.md new file mode 100644 index 00000000000..b625dd23610 --- /dev/null +++ b/changelog/fix_fixed_stylebisectedattraccessor.md @@ -0,0 +1 @@ +* [#9622](https://github.com/rubocop/rubocop/issues/9622): Fixed `Style/BisectedAttrAccessor` autocorrection to handle multiple bisected attrs in the same macro. ([@dvandersluis][]) diff --git a/lib/rubocop/cop/style/bisected_attr_accessor.rb b/lib/rubocop/cop/style/bisected_attr_accessor.rb index e19d4e70d87..c08d5a61d48 100644 --- a/lib/rubocop/cop/style/bisected_attr_accessor.rb +++ b/lib/rubocop/cop/style/bisected_attr_accessor.rb @@ -19,47 +19,61 @@ module Style # end # class BisectedAttrAccessor < Base - include VisibilityHelp + require_relative 'bisected_attr_accessor/macro' + + include RangeHelp extend AutoCorrector MSG = 'Combine both accessors into `attr_accessor %s`.' + def on_new_investigation + @macros_to_rewrite = {} + end + def on_class(class_node) - VISIBILITY_SCOPES.each do |visibility| - reader_names, writer_names = accessor_names(class_node, visibility) - next unless reader_names && writer_names + @macros_to_rewrite[class_node] = Set.new + + find_macros(class_node.body).each do |_visibility, macros| + bisected = find_bisection(macros) + next unless bisected.any? - accessor_macroses(class_node, visibility).each do |macro| - check(macro, reader_names, writer_names) + macros.each do |macro| + attrs = macro.bisect(*bisected) + next if attrs.none? + + @macros_to_rewrite[class_node] << macro + attrs.each { |attr| register_offense(attr) } end end end alias on_sclass on_class alias on_module on_class - private - - def accessor_names(class_node, visibility) - reader_names = nil - writer_names = nil - - accessor_macroses(class_node, visibility).each do |macro| - names = macro.arguments.map(&:source) - - names.each do |name| - if attr_reader?(macro) - (reader_names ||= Set.new).add(name) + # Each offending macro is captured and registered in `on_class` but correction + # happens in `after_class` because a macro might have multiple attributes + # rewritten from it + def after_class(class_node) + @macros_to_rewrite[class_node].each do |macro| + node = macro.node + range = range_by_whole_lines(node.loc.expression, include_final_newline: true) + + correct(range) do |corrector| + if macro.writer? + correct_writer(corrector, macro, node, range) else - (writer_names ||= Set.new).add(name) + correct_reader(corrector, macro, node, range) end end end - - [reader_names, writer_names] end + alias after_sclass after_class + alias after_module after_class - def accessor_macroses(class_node, visibility) - class_def = class_node.body + private + + def find_macros(class_def) + # Find all the macros (`attr_reader`, `attr_writer`, etc.) in the class body + # and turn them into `Macro` objects so that they can be processed. return [] if !class_def || class_def.def_type? send_nodes = @@ -69,66 +83,40 @@ def accessor_macroses(class_node, visibility) class_def.each_child_node(:send) end - send_nodes.select { |node| attr_within_visibility_scope?(node, visibility) } + send_nodes.each_with_object([]) do |node, macros| + macros << Macro.new(node) if Macro.macro?(node) + end.group_by(&:visibility) end - def attr_within_visibility_scope?(node, visibility) - node.macro? && - (attr_reader?(node) || attr_writer?(node)) && - node_visibility(node) == visibility + def find_bisection(macros) + # Find which attributes are defined in both readers and writers so that they + # can be replaced with accessors. + readers, writers = macros.partition(&:reader?) + readers.flat_map(&:attr_names) & writers.flat_map(&:attr_names) end - def attr_reader?(send_node) - send_node.method?(:attr_reader) || send_node.method?(:attr) + def register_offense(attr) + add_offense(attr, message: format(MSG, name: attr.source)) end - def attr_writer?(send_node) - send_node.method?(:attr_writer) - end + def correct_reader(corrector, macro, node, range) + attr_accessor = "attr_accessor #{macro.bisected_names.join(', ')}\n" - def check(macro, reader_names, writer_names) - macro.arguments.each do |arg_node| - name = arg_node.source - - next unless (attr_reader?(macro) && writer_names.include?(name)) || - (attr_writer?(macro) && reader_names.include?(name)) - - add_offense(arg_node, message: format(MSG, name: name)) do |corrector| - macro = arg_node.parent - - corrector.replace(macro, replacement(macro, arg_node)) - end - end - end - - def replacement(macro, node) - class_node = macro.each_ancestor(:class, :sclass, :module).first - reader_names, writer_names = accessor_names(class_node, node_visibility(macro)) - - rest_args = rest_args(macro.arguments, reader_names, writer_names) - - if attr_reader?(macro) - attr_reader_replacement(macro, node, rest_args) - elsif rest_args.empty? - '' + if macro.all_bisected? + corrector.replace(range, "#{indent(node)}#{attr_accessor}") else - "#{macro.method_name} #{rest_args.map(&:source).join(', ')}" - end - end - - def rest_args(args, reader_names, writer_names) - args.reject do |arg| - name = arg.source - reader_names.include?(name) && writer_names.include?(name) + correction = "#{indent(node)}attr_reader #{macro.rest.join(', ')}" + corrector.insert_before(node, attr_accessor) + corrector.replace(node, correction) end end - def attr_reader_replacement(macro, node, rest_args) - if rest_args.empty? - "attr_accessor #{node.source}" + def correct_writer(corrector, macro, node, range) + if macro.all_bisected? + corrector.remove(range) else - "attr_accessor #{node.source}\n"\ - "#{indent(macro)}#{macro.method_name} #{rest_args.map(&:source).join(', ')}" + correction = "attr_writer #{macro.rest.join(', ')}" + corrector.replace(node, correction) end end end diff --git a/lib/rubocop/cop/style/bisected_attr_accessor/macro.rb b/lib/rubocop/cop/style/bisected_attr_accessor/macro.rb new file mode 100644 index 00000000000..628a53c24bd --- /dev/null +++ b/lib/rubocop/cop/style/bisected_attr_accessor/macro.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + class BisectedAttrAccessor + # Representation of an `attr_reader`, `attr_writer` or `attr` macro + # for use by `Style/BisectedAttrAccessor`. + # @api private + class Macro + include VisibilityHelp + + attr_reader :node, :attrs, :bisection + + def self.macro?(node) + node.method?(:attr_reader) || node.method?(:attr_writer) || node.method?(:attr) + end + + def initialize(node) + @node = node + @attrs = node.arguments.map do |attr| + [attr.source, attr] + end.to_h + @bisection = [] + end + + def bisect(*names) + @bisection = attrs.slice(*names).values + end + + def attr_names + @attr_names ||= attrs.keys + end + + def bisected_names + bisection.map(&:source) + end + + def visibility + @visibility ||= node_visibility(node) + end + + def reader? + node.method?(:attr_reader) || node.method?(:attr) + end + + def writer? + node.method?(:attr_writer) + end + + def all_bisected? + rest.none? + end + + def rest + @rest ||= attr_names - bisected_names + end + end + end + end + end +end diff --git a/spec/rubocop/cop/style/bisected_attr_accessor_spec.rb b/spec/rubocop/cop/style/bisected_attr_accessor_spec.rb index 3c9a61dd40f..4f550712182 100644 --- a/spec/rubocop/cop/style/bisected_attr_accessor_spec.rb +++ b/spec/rubocop/cop/style/bisected_attr_accessor_spec.rb @@ -15,7 +15,6 @@ class Foo expect_correction(<<~RUBY) class Foo attr_accessor :bar - #{trailing_whitespace} other_macro :something end RUBY @@ -35,7 +34,6 @@ class Foo expect_correction(<<~RUBY) class Foo attr_accessor :bar - #{trailing_whitespace} other_macro :something end RUBY @@ -57,7 +55,6 @@ class Foo class Foo ATTRIBUTES = %i[foo bar] attr_accessor *ATTRIBUTES - #{trailing_whitespace} other_macro :something end RUBY @@ -84,6 +81,25 @@ class Foo RUBY end + it 'registers an offense and corrects properly when attr_writer is before attr_reader' do + expect_offense(<<~RUBY) + class Foo + attr_writer :foo + ^^^^ Combine both accessors into `attr_accessor :foo`. + attr_reader :foo + ^^^^ Combine both accessors into `attr_accessor :foo`. + other_macro :something + end + RUBY + + expect_correction(<<~RUBY) + class Foo + attr_accessor :foo + other_macro :something + end + RUBY + end + it 'registers an offense and corrects when both accessors are in the same visibility scope' do expect_offense(<<~RUBY) class Foo @@ -104,11 +120,9 @@ class Foo expect_correction(<<~RUBY) class Foo attr_accessor :bar - #{trailing_whitespace} private - #{trailing_whitespace} attr_accessor :baz end RUBY @@ -138,7 +152,6 @@ class Foo class << self attr_accessor :baz - #{trailing_whitespace} private @@ -148,6 +161,53 @@ class << self RUBY end + context 'multiple bisected accessors' do + context 'when all attr names are bisected' do + it 'registers and replaces with attr_accessor' do + expect_offense(<<~RUBY) + class Foo + attr_reader :foo, :bar, :baz + ^^^^ Combine both accessors into `attr_accessor :foo`. + ^^^^ Combine both accessors into `attr_accessor :bar`. + ^^^^ Combine both accessors into `attr_accessor :baz`. + attr_writer :foo, :bar, :baz + ^^^^ Combine both accessors into `attr_accessor :foo`. + ^^^^ Combine both accessors into `attr_accessor :bar`. + ^^^^ Combine both accessors into `attr_accessor :baz`. + end + RUBY + + expect_correction(<<~RUBY) + class Foo + attr_accessor :foo, :bar, :baz + end + RUBY + end + end + + context 'when some attr names are bisected' do + it 'registers and retains non-bisected attrs' do + expect_offense(<<~RUBY) + class Foo + attr_reader :foo, :bar, :baz + ^^^^ Combine both accessors into `attr_accessor :foo`. + ^^^^ Combine both accessors into `attr_accessor :baz`. + attr_writer :foo, :baz + ^^^^ Combine both accessors into `attr_accessor :foo`. + ^^^^ Combine both accessors into `attr_accessor :baz`. + end + RUBY + + expect_correction(<<~RUBY) + class Foo + attr_accessor :foo, :baz + attr_reader :bar + end + RUBY + end + end + end + it 'does not register an offense when only one accessor of the name exists' do expect_no_offenses(<<~RUBY) class Foo @@ -163,7 +223,64 @@ class Foo attr_reader :bar private + attr_writer :bar + end + RUBY + end + + it 'registers an offense for accessors with the same visibility in different scopes' do + expect_offense(<<~RUBY) + class Foo + attr_reader :foo + ^^^^ Combine both accessors into `attr_accessor :foo`. + + private + attr_writer :bar + + public + attr_writer :foo + ^^^^ Combine both accessors into `attr_accessor :foo`. + end + RUBY + + expect_correction(<<~RUBY) + class Foo + attr_accessor :foo + + private + attr_writer :bar + + public + end + RUBY + end + + it 'registers and corrects in a module' do + expect_offense(<<~RUBY) + module Foo + attr_reader :foo + ^^^^ Combine both accessors into `attr_accessor :foo`. + attr_writer :foo, :bar + ^^^^ Combine both accessors into `attr_accessor :foo`. + + private + + attr_reader :bar, :baz + ^^^^ Combine both accessors into `attr_accessor :baz`. attr_writer :baz + ^^^^ Combine both accessors into `attr_accessor :baz`. + end + RUBY + + expect_correction(<<~RUBY) + module Foo + attr_accessor :foo + attr_writer :bar + + private + + attr_accessor :baz + attr_reader :bar end RUBY end