Consider disallowing ivars in methods of any module

[mikegee]

This is a recreation of covermymeds/issues/22 opened by @biinari on Jun 22, 2020.

---

As an extension to the suggestion in covermymeds/issues/4, perhaps we should flag up ivars used in methods of any module.

This would unfortunately add some false positives as modules may be included in classes, making instance methods and hence would be using instance variables rather than class instance variables.

They could be individually checked by the user and ignored with a rubocop:disable comment. Or perhaps this should be an option to the InstanceVariableInClassMethod, which may be set depending on how prevalent extending modules is in the user's codebase.

Concerns I would like to catch would be:

module Mod
  def some_method(something)
    @params = something
  end
end

module ExtMod
  extend Mod
end

class ExtClass
  extend Mod
end

ExtMod.some_method(something) # sets a class instance variable on ExtMod
ExtClass.some_method(something) # sets a class instance variable on ExtClass

False positives that would be difficult / impossible to rule out in a static analysis:

module Mod
  def some_method(something)
    @params = something
  end
end

class Includer
  include Mod
end

instance = Includer.new
instance.some_method(something) # sets an instance variable on instance

[SampsonCrowley]

this would cause more false positives than it would correct catches don't you think? people include and prepend modules way more often than they extend them except for the ClassMethods convention, at least from my experience.

is there a way code is capable of being analyzed for if it is being used as an extension?

[viralpraxis]

Hey @mikegee, do you have any thoughts on this after four years? 🙂

[mikegee]

I'm scared of the false positives and don't want to pursue this. I appreciate the intentions and welcome another strategy to achieve that goal.

[viralpraxis]

I agree that there would be too many false positives. Static analysis is quite limited in this case, so I think we should close this. If someone is interested in revisiting it in the future, we could implement it as a disabled-by-default cop.