Merge pull request #1536 from koic/fix_false_positives_for_rails_find_by_or_assignment_memoization

[Fix #1532] Fix false positives for `Rails/FindByOrAssignmentMemoization`

Author: koic

changelog/fix_false_positives_for_rails_find_by_or_assignment_memoization.md

@@ -0,0 +1 @@
+* [#1532](https://github.com/rubocop/rubocop-rails/issues/1532): Fix false positives for `Rails/FindByOrAssignmentMemoization` when assigning a memoization instance variable at `initialize` method. ([@koic][])

lib/rubocop/cop/rails/find_by_or_assignment_memoization.rb

@@ -8,6 +8,9 @@ module Rails
  # 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.
  #
+ # NOTE: Respecting the object shapes introduced in Ruby 3.2, instance variables used
+ # for memoization that are initialized at object creation are ignored.
+ #
  # @safety
  # This cop is unsafe because detected `find_by` may not be Active Record's method,
  # or the code may have a different purpose than memoization.
@@ -56,14 +59,16 @@ class FindByOrAssignmentMemoization < Base
 
  # When a method body contains only memoization, the correction can be more succinct.
  def on_def(node)
- find_by_or_assignment_memoization(node.body) do |varible_name, find_by|
+ find_by_or_assignment_memoization(node.body) do |variable_name, find_by|
+ next if instance_variable_assigned?(variable_name)
+
  add_offense(node.body) do |corrector|
  corrector.replace(
  node.body,
  <<~RUBY.rstrip
- return #{varible_name} if defined?(#{varible_name})
+ return #{variable_name} if defined?(#{variable_name})
 
- #{varible_name} = #{find_by.source}
+ #{variable_name} = #{find_by.source}
  RUBY
  )
 
@@ -74,17 +79,18 @@ def on_def(node)
 
  def on_send(node)
  assignment_node = node.parent
- find_by_or_assignment_memoization(assignment_node) do |varible_name, find_by|
- next if assignment_node.each_ancestor(:if).any?
+
+ find_by_or_assignment_memoization(assignment_node) do |variable_name, find_by|
+ next if assignment_node.each_ancestor(:if).any? || instance_variable_assigned?(variable_name)
 
  add_offense(assignment_node) do |corrector|
  corrector.replace(
  assignment_node,
  <<~RUBY.rstrip
- if defined?(#{varible_name})
- #{varible_name}
+ if defined?(#{variable_name})
+ #{variable_name}
  else
- #{varible_name} = #{find_by.source}
+ #{variable_name} = #{find_by.source}
  end
  RUBY
  )
@@ -94,6 +100,18 @@ def on_send(node)
 
  private
 
+ def instance_variable_assigned?(instance_variable_name)
+ initialize_methods.any? do |def_node|
+ def_node.each_descendant(:ivasgn).any? do |asgn_node|
+ asgn_node.name == instance_variable_name
+ end
+ end
+ end
+
+ def initialize_methods
+ @initialize_methods ||= processed_source.ast.each_descendant(:def).select { |node| node.method?(:initialize) }
+ end
+
  def correct_to_regular_method_definition(corrector, node)
  range = node.loc.assignment.join(node.body.source_range.begin)
 

spec/rubocop/cop/rails/find_by_or_assignment_memoization_spec.rb

@@ -19,26 +19,52 @@
  end
 
  context 'when using `find_by` with `||=` in a method body' do
- it 'registers an offense' do
+ it 'registers an offense when not assigning the instance variable in the `initialize` method' do
  expect_offense(<<~RUBY)
- def foo
- @current_user ||= User.find_by(id: session[:user_id])
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoizing `find_by` results with `||=`.
+ class Foo
+ def initialize
+ @not_current_user = nil
+ end
+
+ def current_user
+ @current_user ||= User.find_by(id: session[:user_id])
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoizing `find_by` results with `||=`.
+ end
  end
  RUBY
 
  expect_correction(<<~RUBY)
- def foo
- return @current_user if defined?(@current_user)
+ class Foo
+ def initialize
+ @not_current_user = nil
+ end
+
+ def current_user
+ return @current_user if defined?(@current_user)
 
  @current_user = User.find_by(id: session[:user_id])
+ end
+ end
+ RUBY
+ end
+
+ it 'does not register an offense when assigning the instance variable in the `initialize` method' do
+ expect_no_offenses(<<~RUBY)
+ class Foo
+ def initialize
+ @current_user = nil
+ end
+
+ def current_user
+ @current_user ||= User.find_by(id: session[:user_id])
+ end
  end
  RUBY
  end
 
- it 'registers registers an offense when the method contains other code' do
+ it 'registers an offense when the method contains other code' do
  expect_offense(<<~RUBY)
- def foo
+ def current_user
  @current_user ||= User.find_by(id: session[:user_id])
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoizing `find_by` results with `||=`.
 
@@ -47,7 +73,7 @@ def foo
  RUBY
 
  expect_correction(<<~RUBY)
- def foo
+ def current_user
  if defined?(@current_user)
  @current_user
  else
@@ -61,12 +87,12 @@ def foo
 
  it 'registers an offense when using endless method definition', :ruby30 do
  expect_offense(<<~RUBY)
- def foo(arg) = @current_user ||= User.find_by(id: session[:user_id])
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoizing `find_by` results with `||=`.
+ def current_user(arg) = @current_user ||= User.find_by(id: session[:user_id])
+  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid memoizing `find_by` results with `||=`.
  RUBY
 
  expect_correction(<<~RUBY)
- def foo(arg)#{' '}
+ def current_user(arg)#{' '}
  return @current_user if defined?(@current_user)
 
  @current_user = User.find_by(id: session[:user_id])