diff --git a/changelog/new_add_rails_find_by_or_assignment_memoization_cop.md b/changelog/new_add_rails_find_by_or_assignment_memoization_cop.md new file mode 100644 index 0000000000..60bab72b8b --- /dev/null +++ b/changelog/new_add_rails_find_by_or_assignment_memoization_cop.md @@ -0,0 +1 @@ +* [#1324](https://github.com/rubocop/rubocop-rails/pull/1324): Add `Rails/FindByOrAssignmentMemoization` cop. ([@r7kamura][]) diff --git a/config/default.yml b/config/default.yml index 561b36484a..1be1eae089 100644 --- a/config/default.yml +++ b/config/default.yml @@ -502,6 +502,13 @@ Rails/FindById: Enabled: 'pending' VersionAdded: '2.7' +Rails/FindByOrAssignmentMemoization: + Description: 'Avoid memoizing `find_by` results with `||=`.' + StyleGuide: 'https://rails.rubystyle.guide/#find-by-memoization' + Enabled: pending + Safe: false + VersionAdded: '<>' + Rails/FindEach: Description: 'Prefer all.find_each over all.each.' StyleGuide: 'https://rails.rubystyle.guide#find-each' diff --git a/lib/rubocop/cop/rails/find_by_or_assignment_memoization.rb b/lib/rubocop/cop/rails/find_by_or_assignment_memoization.rb new file mode 100644 index 0000000000..107a3e5cc7 --- /dev/null +++ b/lib/rubocop/cop/rails/find_by_or_assignment_memoization.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Avoid memoizing `find_by` results with `||=`. + # + # It is common to see code that attempts to memoize `find_by` result by `||=`, + # but `find_by` may return `nil`, in which case it is not memoized as intended. + # + # @safety + # This cop is unsafe because detected `find_by` may not be activerecord's method, + # or the code may have a different purpose than memoization. + # + # @example + # # bad + # def current_user + # @current_user ||= User.find_by(id: session[:user_id]) + # end + # + # # good + # def current_user + # if instance_variable_defined?(:@current_user) + # @current_user + # else + # @current_user = User.find_by(id: session[:user_id]) + # end + # end + class FindByOrAssignmentMemoization < Base + extend AutoCorrector + + MSG = 'Avoid memoizing `find_by` results with `||=`.' + + RESTRICT_ON_SEND = %i[find_by].freeze + + def_node_matcher :find_by_or_assignment_memoization, <<~PATTERN + (or_asgn + (ivasgn $_) + $(send _ :find_by ...) + ) + PATTERN + + def on_send(node) + assignment_node = node.parent + find_by_or_assignment_memoization(assignment_node) do |varible_name, find_by| + next if in_condition?(assignment_node) + + add_offense(assignment_node) do |corrector| + corrector.replace( + assignment_node, + <<~RUBY.rstrip + if instance_variable_defined?(:#{varible_name}) + #{varible_name} + else + #{varible_name} = #{find_by.source} + end + RUBY + ) + end + end + end + + private + + def in_condition?(node) + node.each_ancestor(:if, :unless).any? + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index e5173baeb0..8e35c24b55 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -55,6 +55,7 @@ require_relative 'rails/file_path' require_relative 'rails/find_by' require_relative 'rails/find_by_id' +require_relative 'rails/find_by_or_assignment_memoization' require_relative 'rails/find_each' require_relative 'rails/freeze_time' require_relative 'rails/has_and_belongs_to_many' diff --git a/spec/rubocop/cop/rails/find_by_or_assignment_memoization_spec.rb b/spec/rubocop/cop/rails/find_by_or_assignment_memoization_spec.rb new file mode 100644 index 0000000000..f0e994f717 --- /dev/null +++ b/spec/rubocop/cop/rails/find_by_or_assignment_memoization_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::FindByOrAssignmentMemoization, :config do + context 'when using `find_by` with `||=`' do + it 'registers an offense' do + expect_offense(<<~RUBY) + @current_user ||= User.find_by(id: session[:user_id]) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoizing `find_by` results with `||=`. + RUBY + + expect_correction(<<~RUBY) + if instance_variable_defined?(:@current_user) + @current_user + else + @current_user = User.find_by(id: session[:user_id]) + end + RUBY + end + end + + context 'with `find_by!`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + @current_user ||= User.find_by!(id: session[:user_id]) + RUBY + end + end + + context 'with local variable' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + current_user ||= User.find_by(id: session[:user_id]) + RUBY + end + end + + context 'with `||`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + @current_user ||= User.find_by(id: session[:user_id]) || User.anonymous + RUBY + end + end + + context 'with ternary operator' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + @current_user ||= session[:user_id] ? User.find_by(id: session[:user_id]) : nil + RUBY + end + end + + context 'with `if`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + @current_user ||= User.find_by(id: session[:user_id]) if session[:user_id] + RUBY + end + end + + context 'with `unless`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + @current_user ||= User.find_by(id: session[:user_id]) unless session[:user_id].nil? + RUBY + end + end +end