crystal-ameba / ameba

A static code analysis tool for Crystal
https://crystal-ameba.github.io
MIT License
518 stars 38 forks source link

[Feature Request] Lint against accessing instance variables outside of the instance of a class #463

Open DanielGilchrist opened 3 months ago

DanielGilchrist commented 3 months ago

It's possible to access instance variables on an instance of a class similarly to instance_variable_get in Ruby

class MyKlass
  def initialize(@my_instance_var : String); end
end

puts MyKlass.new("hello").@my_instance_var
# => "hello"

Crystal Playground

This can be handy for debugging purposes but I think generally should be discouraged as I think it's commonly expected that instance variables are private to the instance of a class.

straight-shoota commented 3 months ago

What about this though?

class MyKlass
  def initialize(@my_instance_var : String); end

  def foo(other : self)
    other.@my_instance_var
  end
end

MyKlass.new.foo(MyKlass.new)

I think from the class scope it should be perfectly fine to access instance variables of any instance. I'm also not sure if you'd ever want to prohibit this, even from outside the class scope. It also makes much sense when several types work together, and are integrated in their implementation details. But that's hard to gauge.

Sija commented 2 months ago

I'd agree that accessing internal variables should be considered as unsafe and a potential issue.

straight-shoota commented 2 months ago

Accesssing ivars is never unsafe, at least not in the way this term is used for other language features which can lead to undefined behaviour or failure. The compiler ensures ivar usage is completely safe.

The only potential issue is that they are typically not considered part of the public API, so they might break in future versions of a dependency. But this only affects code across library boundaries. Within a shared codebase, accessing instance variables should be totally fine.

Instance variables are pretty similar to private methods in this regard. And I don't suppose anyone would consider those unsafe or a potential issue.

Sija commented 2 months ago

That's what I meant. Within the same class it should be totally safe, yet using undocumented APIs in general is IMO sth to watch for.