rubocop: Naming/MemoizedInstanceVariableName false positive

$ bundle exec rubocop -V
1.9.1 (using Parser 3.0.0.0, rubocop-ast 1.4.1, running on ruby 2.7.2 x86_64-linux)

No config

Input file

# frozen_string_literal: true

module MyModule
  # ...

  def f
    @myvar = other_method1
    @myvar ||= other_method2
  end
end

Expected behavior

No error. The method isn’t memoizing a value, it is recalculated every run.

Actual behavior

$ bundle exec rubocop foo.rb 
Inspecting 1 file
C

Offenses:

foo.rb:3:1: C: Style/Documentation: Missing top-level module documentation comment.
module MyModule
^^^^^^
foo.rb:8:5: C: Naming/MemoizedInstanceVariableName: Memoized variable @myvar does not match method name f. Use @f instead.
    @myvar ||= other_method2
    ^^^^^^

1 file inspected, 2 offenses detected

A workaround to avoid the false positive would be to write the code as:

  def f
    @myvar = other_method1 || other_method2
  end

However, depending on the size and complexity of the expression, that may not be preferred. The assignment to the instance variable is the desirable side effect.

Suggestion: If final ||= expression had a previous unconditional assignment, then this offense should not occur.

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 18 (13 by maintainers)

Commits related to this issue

[Fixes #9487] Mark Naming/MemoizedInstanceVariableName as unsafe — committed to marcandre/rubocop by marcandre 3 years ago

Most upvoted comments

Noting that I’m experiencing similar behavior that appears to be a false positive.

def before_render
    @html_opts[:class] ||= ''

    @html_opts[:class] += case @variant
                          when :check_box
                            ' form-check-input'
                          else
                            ' form-control'
                          end

    @label ||= @name.titleize
  end

  def initialize(form:, name:, label: '', select_options: {}, **html_opts)
    @form = form
    @name = name
    @label = label
    @select_options = select_options
    @html_opts = html_opts
  end

 Naming/MemoizedInstanceVariableName: Memoized variable @label does not match method name before_render. Use @before_render instead.
    @label ||= @name.titleize

Obviously in this case, using @before_render would be completely incorrect.

tenpaiyomi on Feb 2, 2021

A solution suggested by the cop is to make the instance variable name the same as the method name

This is not an appropriate change for the example above. In the example above, the assignment to a specific instance variable is the intended side effect. The return value is unused and ignored.

Here is a more real life scenario from a Rails controller:

def show
  @favorite_color = current_user.favorite_color
  @favorite_color ||= default_color
  # implicitly render "show" template  
end

In this example, changing the instance variable to @show would be wrong. It would be less descriptive and require changes in the show template.

jdufresne on Feb 2, 2021