From 5043655b1be444bcd74f2e6be864606e626cac16 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 2 Sep 2023 04:19:40 +0900 Subject: [PATCH] [Fix #1075] Add new `Rails/SelectMap` cop Resolves #1075. This PR adds new `Rails/SelectMap` cop that checks for uses of `select(:column_name)` with `map(&:column_name)`. These can be replaced with `pluck(:column_name)`. There also should be some performance improvement since it skips instantiating the model class for matches. ```ruby # bad Model.select(:column_name).map(&:column_name) # good Model.pluck(:column_name) ``` --- changelog/new_add_new_rails_select_map_cop | 1 + config/default.yml | 6 ++ lib/rubocop/cop/rails/select_map.rb | 75 ++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/select_map_spec.rb | 89 ++++++++++++++++++++++ 5 files changed, 172 insertions(+) create mode 100644 changelog/new_add_new_rails_select_map_cop create mode 100644 lib/rubocop/cop/rails/select_map.rb create mode 100644 spec/rubocop/cop/rails/select_map_spec.rb diff --git a/changelog/new_add_new_rails_select_map_cop b/changelog/new_add_new_rails_select_map_cop new file mode 100644 index 0000000000..373b5a7294 --- /dev/null +++ b/changelog/new_add_new_rails_select_map_cop @@ -0,0 +1 @@ +* [#1075](https://github.com/rubocop/rubocop-rails/issues/1075): Add new `Rails/SelectMap` cop that checks for uses of `select(:column_name)` with `map(&:column_name)`. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 66e69da650..f255018344 100644 --- a/config/default.yml +++ b/config/default.yml @@ -983,6 +983,12 @@ Rails/ScopeArgs: Include: - app/models/**/*.rb +Rails/SelectMap: + Description: 'Checks for uses of `select(:column_name)` with `map(&:column_name)`.' + Enabled: pending + Safe: false + VersionAdded: '<>' + Rails/ShortI18n: Description: 'Use the short form of the I18n methods: `t` instead of `translate` and `l` instead of `localize`.' StyleGuide: 'https://rails.rubystyle.guide/#short-i18n' diff --git a/lib/rubocop/cop/rails/select_map.rb b/lib/rubocop/cop/rails/select_map.rb new file mode 100644 index 0000000000..54a898385a --- /dev/null +++ b/lib/rubocop/cop/rails/select_map.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Checks for uses of `select(:column_name)` with `map(&:column_name)`. + # These can be replaced with `pluck(:column_name)`. + # + # There also should be some performance improvement since it skips instantiating the model class for matches. + # + # @safety + # This cop is unsafe because the model might override the attribute getter. + # Additionally, the model's `after_initialize` hooks are skipped when using `pluck`. + # + # @example + # # bad + # Model.select(:column_name).map(&:column_name) + # + # # good + # Model.pluck(:column_name) + # + class SelectMap < Base + extend AutoCorrector + + MSG = 'Use `%s` instead of `select` with `%s`.' + + RESTRICT_ON_SEND = %i[map collect].freeze + + def on_send(node) + return unless node.first_argument + + column_name = node.first_argument.source.delete_prefix('&:') + return unless (select_node = find_select_node(node, column_name)) + + offense_range = select_node.loc.selector.begin.join(node.source_range.end) + preferred_method = "pluck(:#{column_name})" + message = format(MSG, preferred_method: preferred_method, map_method: node.method_name) + + add_offense(offense_range, message: message) do |corrector| + autocorrect(corrector, select_node, node, preferred_method) + end + end + + private + + def find_select_node(node, column_name) + node.descendants.detect do |select_candidate| + next if !select_candidate.send_type? || !select_candidate.method?(:select) + + match_column_name?(select_candidate, column_name) + end + end + + def autocorrect(corrector, select_node, node, preferred_method) + corrector.remove(select_node.loc.dot.begin.join(select_node.source_range.end)) + corrector.replace(node.loc.selector.begin.join(node.source_range.end), preferred_method) + end + + def match_column_name?(select_candidate, column_name) + return false unless select_candidate.arguments.one? + return false unless (first_argument = select_candidate.first_argument) + + argument = case select_candidate.first_argument.type + when :sym + first_argument.source.delete_prefix(':') + when :str + first_argument.value if first_argument.respond_to?(:value) + end + + argument == column_name + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 3ffdb8f311..7da46a78bc 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -112,6 +112,7 @@ require_relative 'rails/save_bang' require_relative 'rails/schema_comment' require_relative 'rails/scope_args' +require_relative 'rails/select_map' require_relative 'rails/short_i18n' require_relative 'rails/skips_model_validations' require_relative 'rails/squished_sql_heredocs' diff --git a/spec/rubocop/cop/rails/select_map_spec.rb b/spec/rubocop/cop/rails/select_map_spec.rb new file mode 100644 index 0000000000..0ea548620e --- /dev/null +++ b/spec/rubocop/cop/rails/select_map_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::SelectMap, :config do + it 'registers an offense when using `select(:column_name).map(&:column_name)`' do + expect_offense(<<~RUBY) + Model.select(:column_name).map(&:column_name) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `pluck(:column_name)` instead of `select` with `map`. + RUBY + + expect_correction(<<~RUBY) + Model.pluck(:column_name) + RUBY + end + + it "registers an offense when using `select('column_name').map(&:column_name)`" do + expect_offense(<<~RUBY) + Model.select('column_name').map(&:column_name) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `pluck(:column_name)` instead of `select` with `map`. + RUBY + + expect_correction(<<~RUBY) + Model.pluck(:column_name) + RUBY + end + + it 'registers an offense when using `select(:column_name).collect(&:column_name)`' do + expect_offense(<<~RUBY) + Model.select(:column_name).collect(&:column_name) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `pluck(:column_name)` instead of `select` with `collect`. + RUBY + + expect_correction(<<~RUBY) + Model.pluck(:column_name) + RUBY + end + + it 'registers an offense when using `select(:column_name).where(conditions).map(&:column_name)`' do + expect_offense(<<~RUBY) + Model.select(:column_name).where(conditions).map(&:column_name) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `pluck(:column_name)` instead of `select` with `map`. + RUBY + + expect_correction(<<~RUBY) + Model.where(conditions).pluck(:column_name) + RUBY + end + + it 'does not register an offense when using `select(:mismatch_column_name).map(&:column_name)`' do + expect_no_offenses(<<~RUBY) + Model.select(:mismatch_column_name).map(&:column_name) + RUBY + end + + it 'does not register an offense when using `select(:column_name, :other_column_name).map(&:column_name)`' do + expect_no_offenses(<<~RUBY) + Model.select(:column_name, :other_column_name).map(&:column_name) + RUBY + end + + it 'does not register an offense when using `select(column_names).map(&:column_name)`' do + expect_no_offenses(<<~RUBY) + Model.select(column_names).map(&:column_name) + RUBY + end + + it 'does not register an offense when using `select(:column_name).do_something(&:column_name)`' do + expect_no_offenses(<<~RUBY) + Model.select(:column_name).do_something(&:column_name) + RUBY + end + + it 'does not register an offense when using `select { |item| item.column_name }.map(&:column_name)`' do + expect_no_offenses(<<~RUBY) + Model.select { |item| item.column_name }.map(&:column_name) + RUBY + end + + it 'does not register an offense when using `select(:column_name).map { |item| do_something(item) }`' do + expect_no_offenses(<<~RUBY) + Model.select(:column_name).map { |item| do_something(item) } + RUBY + end + + it 'does not register an offense when using `pluck(:column_name)`' do + expect_no_offenses(<<~RUBY) + Model.pluck(:column_name) + RUBY + end +end