Skip to content

Commit

Permalink
Add Rails/FindByOrAssignmentMemoization cop
Browse files Browse the repository at this point in the history
  • Loading branch information
r7kamura committed Aug 11, 2024
1 parent b8c4126 commit 7223740
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1324](https://github.com/rubocop/rubocop-rails/pull/1324): Add `Rails/FindByOrAssignmentMemoization` cop. ([@r7kamura][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,12 @@ Rails/FindById:
Enabled: 'pending'
VersionAdded: '2.7'

Rails/FindByOrAssignmentMemoization:
Description: 'Avoid memoization by `find_by` with `||=`.'
Enabled: pending
Safe: false
VersionAdded: '<<next>>'

Rails/FindEach:
Description: 'Prefer all.find_each over all.each.'
StyleGuide: 'https://rails.rubystyle.guide#find-each'
Expand Down
65 changes: 65 additions & 0 deletions lib/rubocop/cop/rails/find_by_or_assignment_memoization.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Avoid memoization by `find_by` 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
# @current_user =
# if instance_variable_defined?(:@current_user)
# @current_user
# else
# User.find_by(id: session[:user_id])
# end
# end
class FindByOrAssignmentMemoization < Base
extend AutoCorrector

MSG = 'Avoid memoization by `find_by` 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|
add_offense(assignment_node) do |corrector|
corrector.replace(
assignment_node,
<<~RUBY.rstrip
#{varible_name} =
if instance_variable_defined?(:#{varible_name})
#{varible_name}
else
#{find_by.source}
end
RUBY
)
end
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
37 changes: 37 additions & 0 deletions spec/rubocop/cop/rails/find_by_or_assignment_memoization_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# 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 memoization by `find_by` with `||=`.
RUBY

expect_correction(<<~RUBY)
@current_user =
if instance_variable_defined?(:@current_user)
@current_user
else
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
end

0 comments on commit 7223740

Please sign in to comment.