Cop idea: prevent instance variables in concerns

[ydakuka]

Actual behavior

I have the following code:

# frozen_string_literal: true

module Informable
  extend ActiveSupport::Concern

  def define_user_info
    session[:user_info_id] = @user.id
  end
end

# frozen_string_literal: true

class UsersController
  include Informable
  # include A-able
  # ...
  # include Z-able

  def create
    @user = User.first
    # other code

    define_user_info

    # other code
  end
end

I run rubocop:

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop app/controllers/concerns/informable.rb
Inspecting 1 file
.

1 file inspected, no offenses detected

If I replace the instance variable @user with a local variable in the controller, I will get an error, because the variable user is not explicitly passed to the #define_user_info method.

Rubocop

ydakuka@yauhenid:~/Work/project$ bin/rails_docker rubocop -V
1.54.1 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 2.7.8) [x86_64-linux]
  - rubocop-capybara 2.18.0
  - rubocop-factory_bot 2.23.1
  - rubocop-performance 1.18.0
  - rubocop-rails 2.20.2
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.22.0
  - rubocop-thread_safety 0.5.1

[pirj]

What do you mean by an error? NoMethodError or nil @user?
Do you think it’s specific to concerns?
Afaik, this is a primary argument to not use instance vars in specs. https://github.com/rubocop/rspec-style-guide#instance-variables

[ydakuka]

What do you mean by an error? NoMethodError or nil @user?
No. The default value of the instance variable is nil, so I didn't notice that anything was broken due to the order of the method calls and no tests. The @user var ceased receiving a value prior to the call of the #define_user_info.

This would not have happened if the method had been written like this:

def define_user_info(user_id)
  session[:user_info_id] = user_id
end

Do you think it’s specific to concerns?

I know this case is not specific to concerns. However, I've decided to share my pain about concerns :)

[pirj]

Pretty sure there’s a cop that would disallow passing instance vars as arguments to method calls with implicit receiver. Or there should be such a cop.

It sounds like we’ve shaped this down to “never use instance vars other than assigning them once in the initializer”, but it sounds overly strict.

[ydakuka]

it sounds overly strict.

Agree, I've closed the issue.

#define_user_info could have been defined and called via action filters only in concerns and not called explicitly in controller actions. And it seems that this is a very common case.

I think more tests is my choice.