diff --git a/changelog/new_add_new_style_map_into_array_cop.md b/changelog/new_add_new_style_map_into_array_cop.md new file mode 100644 index 000000000000..d65f7612bf37 --- /dev/null +++ b/changelog/new_add_new_style_map_into_array_cop.md @@ -0,0 +1 @@ +* [#11878](https://github.com/rubocop/rubocop/issues/11878): Add new `Style/MapIntoArray` cop. ([@ymap][]) diff --git a/config/default.yml b/config/default.yml index e0154ea32b68..df3d3bad05f1 100644 --- a/config/default.yml +++ b/config/default.yml @@ -4289,6 +4289,13 @@ Style/MapCompactWithConditionalBlock: Enabled: pending VersionAdded: '1.30' +Style/MapIntoArray: + Description: 'Checks for usages of `each` with `<<`, `push`, or `append` which can be replaced by `map`.' + StyleGuide: '#functional-code' + Enabled: pending + VersionAdded: '<>' + Safe: false + Style/MapToHash: Description: 'Prefer `to_h` with a block over `map.to_h`.' Enabled: pending diff --git a/lib/rubocop.rb b/lib/rubocop.rb index b5f498d713e1..713569adf382 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -557,6 +557,7 @@ require_relative 'rubocop/cop/style/lambda_call' require_relative 'rubocop/cop/style/line_end_concatenation' require_relative 'rubocop/cop/style/magic_comment_format' +require_relative 'rubocop/cop/style/map_into_array' require_relative 'rubocop/cop/style/map_to_hash' require_relative 'rubocop/cop/style/map_to_set' require_relative 'rubocop/cop/style/method_call_without_args_parentheses' diff --git a/lib/rubocop/cop/style/map_into_array.rb b/lib/rubocop/cop/style/map_into_array.rb new file mode 100644 index 000000000000..54fe16a69750 --- /dev/null +++ b/lib/rubocop/cop/style/map_into_array.rb @@ -0,0 +1,175 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # Checks for usages of `each` with `<<`, `push`, or `append` which + # can be replaced by `map`. + # + # If `PreferredMethods` is configured for `map` in `Style/CollectionMethods`, + # this cop uses the specified method for replacement. + # + # NOTE: The return value of `Enumerable#each` is `self`, whereas the + # return value of `Enumerable#map` is an `Array`. They are not autocorrected + # when a return value could be used because these types differ. + # + # NOTE: It only detects when the mapping destination is a local variable + # initialized as an empty array and referred to only by the pushing operation. + # This is because, if not, it's challenging to statically guarantee that the + # mapping destination variable remains an empty array: + # + # [source,ruby] + # ---- + # @dest = [] + # src.each { |e| @dest << e * 2 } # `src` method may mutate `@dest` + # + # dest = [] + # src.each { |e| dest << transform(e, dest) } # `transform` method may mutate `dest` + # ---- + # + # @safety + # This cop is unsafe because not all objects that have an `each` + # method also have a `map` method (e.g. `ENV`). Additionally, for calls + # with a block, not all objects that have a `map` method return an array + # (e.g. `Enumerator::Lazy`). + # + # @example + # # bad + # dest = [] + # src.each { |e| dest << e * 2 } + # dest + # + # # good + # dest = src.map { |e| e * 2 } + # + # # good - contains another operation + # dest = [] + # src.each { |e| dest << e * 2; puts e } + # dest + # + class MapIntoArray < Base + include RangeHelp + extend AutoCorrector + + MSG = 'Use `%s` instead of `each` to map elements into an array.' + + # @!method each_block_with_push?(node) + def_node_matcher :each_block_with_push?, <<-PATTERN + [ + ^({begin kwbegin} ...) + ({block numblock} (send _ :each) _ + (send (lvar _) {:<< :push :append} _)) + ] + PATTERN + + # @!method empty_array_asgn?(node) + def_node_matcher :empty_array_asgn?, '(lvasgn _ (array))' + + # @!method lvar_ref?(node, name) + def_node_matcher :lvar_ref?, '(lvar %1)' + + def self.joining_forces + VariableForce + end + + def after_leaving_scope(scope, _variable_table) + (@scopes ||= []) << scope + end + + def on_block(node) + return unless each_block_with_push?(node) + + dest_var = find_dest_var(node) + return unless (asgn = find_closest_assignment(node, dest_var)) + return unless empty_array_asgn?(asgn) + return unless dest_used_only_for_mapping?(node, dest_var, asgn) + + register_offense(node, dest_var, asgn) + end + + alias on_numblock on_block + + private + + def find_dest_var(block) + node = block.body.receiver + name = node.children.first + + candidates = @scopes.lazy.filter_map { |s| s.variables[name] } + candidates.find { |v| v.references.any? { |n| n.node.equal?(node) } } + end + + def find_closest_assignment(block, dest_var) + dest_var.assignments.reverse_each.lazy.map(&:node).find do |node| + node.source_range.end_pos < block.source_range.begin_pos + end + end + + def dest_used_only_for_mapping?(block, dest_var, asgn) + range = asgn.source_range.join(block.source_range) + + asgn.parent.equal?(block.parent) && + dest_var.references.one? { |r| range.contains?(r.node.source_range) } && + dest_var.assignments.one? { |a| range.contains?(a.node.source_range) } + end + + def register_offense(block, dest_var, asgn) + add_offense(block, message: format(MSG, new_method_name: new_method_name)) do |corrector| + next if return_value_used?(block) + + corrector.replace(block.send_node.selector, new_method_name) + remove_assignment(corrector, asgn) + correct_push_node(corrector, block.body) + correct_return_value_handling(corrector, block, dest_var) + end + end + + def new_method_name + default = 'map' + alternative = config.for_cop('Style/CollectionMethods').dig('PreferredMethods', default) + alternative || default + end + + def return_value_used?(node) + parent = node.parent + + case parent&.type + when nil + false + when :begin, :kwbegin + !node.right_sibling && return_value_used?(parent) + when :block, :numblock + !parent.void_context? + else + true + end + end + + def remove_assignment(corrector, asgn) + range = range_with_surrounding_space(asgn.source_range, side: :right) + range = range_with_surrounding_space(range, side: :right, newlines: false) + + corrector.remove(range) + end + + def correct_push_node(corrector, push_node) + range = push_node.source_range + arg_range = push_node.first_argument.source_range + + corrector.remove(range_between(range.begin_pos, arg_range.begin_pos)) + corrector.remove(range_between(arg_range.end_pos, range.end_pos)) + end + + def correct_return_value_handling(corrector, block, dest_var) + next_node = block.right_sibling + + if lvar_ref?(next_node, dest_var.name) + corrector.remove(range_with_surrounding_space(next_node.source_range, side: :left)) + end + + corrector.insert_before(block, "#{dest_var.name} = ") + end + end + end + end +end diff --git a/spec/rubocop/cop/style/map_into_array_spec.rb b/spec/rubocop/cop/style/map_into_array_spec.rb new file mode 100644 index 000000000000..dcb0e1d96be2 --- /dev/null +++ b/spec/rubocop/cop/style/map_into_array_spec.rb @@ -0,0 +1,352 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::MapIntoArray, :config do + it 'registers an offense and corrects when using `each` with `<<` to build an array' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| e * 2 } + RUBY + end + + it 'registers an offense and corrects when using `each` with `push` to build an array' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest.push(e * 2) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| e * 2 } + RUBY + end + + it 'registers an offense and corrects when using `each` with `append` to build an array' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest.append(e * 2) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| e * 2 } + RUBY + end + + it 'registers an offense and corrects when a non-related operation precedes an `each` call' do + expect_offense(<<~RUBY) + dest = [] + do_something + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + do_something + dest = src.map { |e| e * 2 } + RUBY + end + + it 'registers an offense and corrects when a non-related operation follows an `each` call' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + do_something + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| e * 2 } + do_something + RUBY + end + + it 'registers an offense and corrects when using a numblock' do + expect_offense(<<~RUBY) + dest = [] + src.each { dest << _1 * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { _1 * 2 } + RUBY + end + + it 'registers an offense and corrects when the destination initialized multiple times' do + expect_offense(<<~RUBY) + dest = [] + do_something(dest) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = [] + do_something(dest) + dest = src.map { |e| e * 2 } + RUBY + end + + it 'registers an offense and corrects removing a destination reference follows an `each` call' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + dest + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| e * 2 } + RUBY + end + + it 'registers an offense and corrects when nested autocorrections required' do + expect_offense(<<~RUBY) + dest = [] + src.each do |e| + ^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + dest << ( + dest2 = [] + src.each do |e| + ^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + dest2 << e + end + dest2 + ) + end + RUBY + + expect_correction(<<~RUBY) + dest = src.map do |e| + ( + dest2 = src.map do |e| + e + end + ) + end + RUBY + end + + it 'does not register an offense when the destination is not a local variable' do + expect_no_offenses(<<~RUBY) + @dest = [] + src.each { |e| @dest << e } + RUBY + end + + it 'does not register an offense when `each` is called with non-block arguments' do + expect_no_offenses(<<~RUBY) + dest = [] + StringIO.new('foo:bar').each(':') { |e| dest << e } + RUBY + end + + it 'does not register an offense when the parent node of an `each` call is not a begin node' do + expect_no_offenses(<<~RUBY) + [ + dest = [], + src.each { |e| dest << e * 2 }, + ] + RUBY + end + + it 'does not register an offense when the destination initialization is not a sibling of an `each` call' do + expect_no_offenses(<<~RUBY) + dest = [] + if cond + src.each { |e| dest << e * 2 } + end + RUBY + end + + it 'does not register an offense when the destination is used before an `each` call' do + expect_no_offenses(<<~RUBY) + dest = [] + dest << 0 + src.each { |e| dest << e * 2 } + RUBY + end + + it 'does not register an offense when the destination is used in the receiver expression' do + expect_no_offenses(<<~RUBY) + dest = [] + (dest << 1).each { |e| dest << e * 2 } + RUBY + end + + it 'does not register an offense when the destination is shadowed by a block argument' do + expect_no_offenses(<<~RUBY) + dest = [] + src.each { |dest| dest << 1 } + RUBY + end + + it 'does not register an offense when the destination is used in the transformation' do + expect_no_offenses(<<~RUBY) + dest = [] + src.each { |e| dest << dest.size } + RUBY + end + + context 'new method name for replacement' do + context 'when `Style/CollectionMethods` is configured for `map`' do + let(:other_cops) do + { + 'Style/CollectionMethods' => { + 'PreferredMethods' => { + 'map' => 'collect' + } + } + } + end + + it 'registers an offense and corrects using the method specified in `PreferredMethods`' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `collect` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.collect { |e| e * 2 } + RUBY + end + end + + context 'when `Style/CollectionMethods` is configured except for `map`' do + let(:other_cops) do + { + 'Style/CollectionMethods' => { + 'PreferredMethods' => { + 'reject' => 'filter' + } + } + } + end + + it 'registers an offense and corrects using `map` method' do + expect_offense(<<~RUBY) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(<<~RUBY) + dest = src.map { |e| e * 2 } + RUBY + end + end + end + + context 'autocorrection skipping' do + shared_examples 'corrects' do |template:| + it 'registers an offense and corrects' do + expect_offense(format(template, <<~RUBY)) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_correction(format(template, <<~RUBY)) + dest = src.map { |e| e * 2 } + RUBY + end + end + + shared_examples 'skip correcting' do |template:| + it 'registers an offense but does not autocorrect it' do + expect_offense(format(template, <<~RUBY)) + dest = [] + src.each { |e| dest << e * 2 } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `map` instead of `each` to map elements into an array. + RUBY + + expect_no_corrections + end + end + + context 'at the top level' do + it_behaves_like 'corrects', template: '%s' + end + + context 'in parentheses' do + context 'not at the end' do + context 'parent is used' do + it_behaves_like 'corrects', template: 'a = (%s; do_someting)' + end + + context 'parent is not used' do + it_behaves_like 'corrects', template: '(%s; do_someting)' + end + end + + context 'at the end' do + context 'parent is used' do + it_behaves_like 'skip correcting', template: 'a = (%s)' + end + + context 'parent is not used' do + it_behaves_like 'corrects', template: '(%s)' + end + end + end + + context 'in a begin block' do + context 'not at the end' do + context 'parent is used' do + it_behaves_like 'corrects', template: 'a = begin; %s; do_something end' + end + + context 'parent is not used' do + it_behaves_like 'corrects', template: 'begin; %s; do_something end' + end + end + + context 'at the end' do + context 'parent is used' do + it_behaves_like 'skip correcting', template: 'a = begin; %s end' + end + + context 'parent is not used' do + it_behaves_like 'corrects', template: 'begin; %s end' + end + end + end + + context 'in a block' do + context 'in a void context' do + it_behaves_like 'corrects', template: 'each { %s }' + end + + context 'in a non-void context' do + it_behaves_like 'skip correcting', template: 'block { %s }' + end + end + + context 'in a numblock' do + context 'in a void context' do + it_behaves_like 'corrects', template: 'each { _1; %s }' + end + + context 'in a non-void context' do + it_behaves_like 'skip correcting', template: 'block { _1; %s }' + end + end + + context 'in a method' do + context 'not at the end' do + it_behaves_like 'corrects', template: 'def foo; %s; do_something end' + end + + context 'at the end' do + it_behaves_like 'skip correcting', template: 'def foo; %s end' + end + end + end +end