Add `Rails/FindByOrAssignmentMemoization` cop

[r7kamura]

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.

def current_user
  @current_user ||= User.find_by(id: session[:user_id])
end

In most cases, a query cache is used, but even so, performance degradation due to unnecessary processing and object creation can still occur.

See also:

• https://github.com/rubocop/rails-style-guide/pull/358

---

Before submitting the PR make sure the following are checked:

• [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• [x] Wrote good commit messages.
• [ ] Commit message starts with [Fix #issue-number] (if the related issue exists).
• [x] Feature branch is up-to-date with master (if not - rebase it).
• [x] Squashed related commits together.
• [x] Added tests.
• [x] Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• [x] Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• [x] If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[Earlopain]

I like this, its a common pitfall I've encountered many times myself and is most likely not what the programmer intended.

Let's add this as a test (works fine already, no offense):

@current_user ||= User.find_by(id: session[:user_id]) || User.anon

@current_user ||= session[:user_id] ? User.find_by(id: session[:user_id]) : nil

and this pattern (also fine, correct autocorrect):

@current_user ||= User.find_by(id: session[:user_id]) unless session[:user_id].nil?

---

For what I presume would be the most common case, I'd write the correction as follows:

def current_user
  @current_user ||= User.find_by(id: session[:user_id])
end

# =>

def current_user
  return @current_user if instance_variable_defined?(:@current_user)

  @current_user = User.find_by(id: session[:user_id])
end

I just find it easier to parse than the if/else assignment to a single variable, but that only really works when the memoization is the only line of code, so eh.

---

~~I would exclude controllers from being inspected by this cop. They execute once and don't benefit from changing the code.~~. Well, actions wouldn't but those aren't the only methods a controller can have. I guess my thought here doesn't really work out.

[koic]

If this is a new cop, consider making a corresponding update to the Rails Style Guide.

Can you open to the Rails Style Guide first?

[r7kamura]

Thanks, I just proposed a pull request to rubocop/rails-style-guide:

• https://github.com/rubocop/rails-style-guide/pull/358

[KieranP]

Just updated. The same is true of a lot of belongs_to or has_one methods, e.g. @country ||= @user.country where country fires an SQL query to fetch the record, but could be nil. But without type analysis, I imagine this would be impossible to detect.