Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naming/MemoizedInstanceVariableName false positive #9487

Closed
jdufresne opened this issue Feb 2, 2021 · 18 comments
Closed

Naming/MemoizedInstanceVariableName false positive #9487

jdufresne opened this issue Feb 2, 2021 · 18 comments
Labels

Comments

@jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Feb 2, 2021

$ 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.

@koic
Copy link
Member

@koic koic commented Feb 2, 2021

This MemoizedInstanceVariableName cop is in the Naming department and is an expected behavior.
This cop checks that method name and memoized instance variable name are the same. A solution suggested by the cop is to make the instance variable name the same as the method name. (Tip: Be careful not to conflict with existing instance variable names.) Thank you.

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

https://docs.rubocop.org/rubocop/1.9/cops_naming.html#namingmemoizedinstancevariablename

@koic koic closed this as completed Feb 2, 2021
@jdufresne
Copy link
Contributor Author

@jdufresne jdufresne commented 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
Copy link
Contributor Author

@jdufresne jdufresne commented Feb 2, 2021

... and memoized instance variable

I would also like to reiterate that the provided examples are not memoizing the function. The instance variables are intentionally recalculated every call. That is why I consider this a false positive.

Suggestion: If the method has at least one unconditional assignment, then this offense should not occur. (For example @myvar = other_method1 or @favorite_color = current_user.favorite_color)

@dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Feb 2, 2021

I agree. This example isn’t memorization, even though it uses ||=. The ivar will be recalculated every time the method is called, even if it’s previously set.

@dvandersluis dvandersluis reopened this Feb 2, 2021
@koic
Copy link
Member

@koic koic commented Feb 2, 2021

Ah, I get it! It seems that my understanding was not enough 💦

@marcandre
Copy link
Contributor

@marcandre marcandre commented Feb 2, 2021

What happens if you explicitly end your method with nil?

Currently you are returning @favorite_color, which is probably harmless but is confusing the cop.

@jdufresne
Copy link
Contributor Author

@jdufresne jdufresne commented Feb 2, 2021

I agree that adding nil as the final expression also works as a workaround to silence the offense. I offered a workaround in the original post and I'm sure there are others too.

But I still think the unconditional assignment should be enough to silence the offense (if possible). While adding an extra nil expression isn't a huge ask, it is a small annoyance when the code is otherwise correct and follows the Ruby style guide.

@tenpaiyomi
Copy link

@tenpaiyomi tenpaiyomi commented Feb 2, 2021

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.

@marcandre
Copy link
Contributor

@marcandre marcandre commented Feb 2, 2021

@tenpaiyomi Indeed. Returning nil works though. May I point out that it seems incorrect to set @label and that @html_opts in before_render and not in initialize (or via methods that return @label || @name.titleize if @name is subject to change). Maybe a list of ignored methods (which have their return values typically ignored) could help.

@tenpaiyomi
Copy link

@tenpaiyomi tenpaiyomi commented Feb 2, 2021

@marcandre given how the view components work, it is necessitated that I set the options in before_render as opposed to within the initialize method, given that initialize does not have a value for @variant yet, nor does it have access to helper methods.

@marcandre
Copy link
Contributor

@marcandre marcandre commented Feb 2, 2021

Oh, I see. Isn't it the @label ||= ... that is causing the error? Anyways, it could depend on something else I guess.

@tenpaiyomi
Copy link

@tenpaiyomi tenpaiyomi commented Feb 2, 2021

@marcandre yes, in this case it appears that rubocop is incorrectly assuming the @label ||= is supposed to be a return value assignment for the before_render method and believes it should be @before_render.

I was able to shift the @label ||= up to the top of the method and the error went away, but it could be a point of confusion for others (or not a viable option, outside of placing nil at the end of the method which feels janky).

@marcandre
Copy link
Contributor

@marcandre marcandre commented Feb 2, 2021

Maybe we can reduce the number of false positives by restricting this offense to def foo; @x ||= ...; end, with no other code before or after the @x ||=

@tenpaiyomi
Copy link

@tenpaiyomi tenpaiyomi commented Feb 2, 2021

I think the biggest issue in this case is that ||= is not explicitly just memoization. In my case, it's being utilizing it as an assign on blank fallback in case the user does not pass in a value for label, which is not an uncommon usage scenario at all. It's not a resource intensive process or anything of the sort, it's just the most concise way of doing the operation, IMO.

A few alternatives that would work and don't throw any issues are

@label = @name.titleize if @label.blank?

or

@label = set_label_value

...

def set_label_value
  @label || @name.titleize
end

but neither of those (personally) feel as clean and straightforward as the current solution.

The issue you present would definitely help, but in a case of my original code where I was not modifying @html_opts, then I would still be getting a false positive.

I'll mull over potentials to try and present, as this is certainly an odd case.

@AndreiEres
Copy link
Contributor

@AndreiEres AndreiEres commented Mar 15, 2021

So guys, what a result of this discussion? Is it still a bug and we have to fix it? Or we can leave it as is?

@marcandre
Copy link
Contributor

@marcandre marcandre commented Mar 16, 2021

There's no clear concensus. Maybe Rails app will simply disable this cop.

@jdufresne
Copy link
Contributor Author

@jdufresne jdufresne commented Mar 16, 2021

IMO, so long as this cop reports false positives, this should continue to be considered an issue. That is still the case today.

If the false positives can't be resolved, then perhaps the next best thing is to mark this cop as unsafe. IIUC, complying with a safe offense should not change outward behavior.

@marcandre
Copy link
Contributor

@marcandre marcandre commented Mar 16, 2021

Oh, right, I thought it was unsafe already. I opened #9487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants