Skip to content

Commit 91ef090

Browse files
committed
Add Rails/FindByOrAssignmentMemoization cop
1 parent b8c4126 commit 91ef090

File tree

5 files changed

+106
-0
lines changed

5 files changed

+106
-0
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#x](https://github.com/rubocop/rubocop-rails/pull/x): Add `Rails/FindByOrAssignmentMemoization` cop. ([@r7kamura][])

config/default.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -502,6 +502,12 @@ Rails/FindById:
502502
Enabled: 'pending'
503503
VersionAdded: '2.7'
504504

505+
Rails/FindByOrAssignmentMemoization:
506+
Description: 'Avoid memoization by `find_by` with `||=`.'
507+
Enabled: pending
508+
Safe: false
509+
VersionAdded: '<<next>>'
510+
505511
Rails/FindEach:
506512
Description: 'Prefer all.find_each over all.each.'
507513
StyleGuide: 'https://rails.rubystyle.guide#find-each'
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# Avoid memoization by `find_by` with `||=`.
7+
#
8+
# It is common to see code that attempts to memoize `find_by` result by `||=`,
9+
# but `find_by` may return `nil`, in which case it is not memoized as intended.
10+
#
11+
# @safety
12+
# This cop is unsafe because detected `find_by` may not be activerecord's method,
13+
# or the code may have a different purpose than memoization.
14+
#
15+
# @example
16+
# # bad
17+
# @current_user ||= User.find_by(id: session[:user_id])
18+
#
19+
# # good
20+
# @current_user =
21+
# if instance_variable_defined?(:@current_user)
22+
# @current_user
23+
# else
24+
# User.find_by(id: session[:user_id])
25+
# end
26+
class FindByOrAssignmentMemoization < Base
27+
extend AutoCorrector
28+
29+
MSG = 'Avoid memoization by `find_by` with `||=`.'
30+
31+
RESTRICT_ON_SEND = %i[find_by].freeze
32+
33+
def_node_matcher :find_by_or_assignment_memoization, <<~PATTERN
34+
(or_asgn
35+
(ivasgn $_)
36+
$(send _ :find_by ...)
37+
)
38+
PATTERN
39+
40+
def on_send(node)
41+
assignment_node = node.parent
42+
find_by_or_assignment_memoization(assignment_node) do |varible_name, find_by|
43+
add_offense(assignment_node) do |corrector|
44+
corrector.replace(
45+
assignment_node,
46+
<<~RUBY.rstrip
47+
#{varible_name} =
48+
if instance_variable_defined?(:#{varible_name})
49+
#{varible_name}
50+
else
51+
#{find_by.source}
52+
end
53+
RUBY
54+
)
55+
end
56+
end
57+
end
58+
end
59+
end
60+
end
61+
end

lib/rubocop/cop/rails_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
require_relative 'rails/file_path'
5656
require_relative 'rails/find_by'
5757
require_relative 'rails/find_by_id'
58+
require_relative 'rails/find_by_or_assignment_memoization'
5859
require_relative 'rails/find_each'
5960
require_relative 'rails/freeze_time'
6061
require_relative 'rails/has_and_belongs_to_many'
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Rails::FindByOrAssignmentMemoization, :config do
4+
context 'when using `find_by` with `||=`' do
5+
it 'registers an offense' do
6+
expect_offense(<<~RUBY)
7+
@current_user ||= User.find_by(id: session[:user_id])
8+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoization by `find_by` with `||=`.
9+
RUBY
10+
11+
expect_correction(<<~RUBY)
12+
@current_user =
13+
if instance_variable_defined?(:@current_user)
14+
@current_user
15+
else
16+
User.find_by(id: session[:user_id])
17+
end
18+
RUBY
19+
end
20+
end
21+
22+
context 'with `find_by!`' do
23+
it 'does not register an offense' do
24+
expect_no_offenses(<<~RUBY)
25+
@current_user ||= User.find_by!(id: session[:user_id])
26+
RUBY
27+
end
28+
end
29+
30+
context 'with local variable' do
31+
it 'does not register an offense' do
32+
expect_no_offenses(<<~RUBY)
33+
current_user ||= User.find_by(id: session[:user_id])
34+
RUBY
35+
end
36+
end
37+
end

0 commit comments

Comments
 (0)