Merge pull request #1102 from koic/add_new_rails_select_map_cop

[Fix #1075] Add new `Rails/SelectMap` cop

Author: koic

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][])

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: '<<next>>'
+
 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'

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 `%<preferred_method>s` instead of `select` with `%<map_method>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

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'

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