Restrict exp.@var to types in the same hierarchy
#6358
Conversation
|
Is this technically a breaking change? |
|
Would this prevent access from a sample like this? require "http/client"
class SampleConnection
def self.collect_metrics
client = HTTP::Client.new "www.example.com"
pp! client.@socket
end
end
SampleConnection.collect_metrics |
|
Yes, of course. |
|
Should there be a test case for not in the top level? it "can read from any non top-level" do
assert_error %(
class Foo
@x = 1
end
class Bar
def baz
Foo.new.@x
end
end
Bar.new.baz
),
"can't access Foo.@x from the top-level"
endAlso is there a reason why top level is excluded? |
|
There's a test for that. And your test is accessing the instance var from Bar, not the top level. |
|
I just wanted to check. It seems a lot of the tests are passing in params. It is good to know that will be there. |
| "can't access Bar.@x from unrelated type Foo" | ||
| end | ||
|
|
||
| it "can't read from top-level" do |
There was a problem hiding this comment.
Is there a reason why the top-level is excluded from accessing ivars?
There was a problem hiding this comment.
It's the main thing this PR changes.
There was a problem hiding this comment.
I am sorry I missed that from the issue. I didnt realize it was restricted like that.
|
I was under the impression that the tenor in #6066 was not to change accessing instance variables. |
|
|
Following up from earlier. I am curious why was this merged? |
|
It doesn’t follow from discussion that this should have been merged. Let’s revert this PR before the release :-) |
|
Better to try it and see what it breaks in my opinion. We can always revert it in a point release if people actually need this feature. This PR makes the code cleaner at least, so please avoid doing a flat-out |
|
@RX14 Breaking it to see if users' code breaks while we actually don't want it to break (and break again in a point release) doesn't seem like a good idea at all. |
|
@straight-shoota we change things to see if they break all the time, look at #6024, which was reverted. Let's do some science. |
|
Yeah, but that change was intended. It would have stayed if it didn't break. This one is, as far as I am concerned, not intended. So why bother breaking if we don't want it in the end anyways? |
Fixes #6066
With this PR, doing
exp.@varonly works if:exp, orexp, orexpinherits the current type, orthe current type and the type ofexplive in the same namespaceSo, freely accessing instance vars is disallowed. Of course, one can always reopen a class and add an accessor (which is why I didn't see the current behaviour as a big problem, and why probably Ruby has
instance_variable_get, because one can always access an instance var if you want to). But, at least we are not encouraging its use.I was hesitant about point 4. All 4 points mean (Crystal, not Ruby)
protected. Without 4, it's almostprotected. At first I implemented just 1, 2 and 3, but then noticed it's used inHash::BaseIterator:crystal/src/hash.cr
Line 958 in 4771dcb
So I added rule 4. However, later I noticed it's the only place where that "in the same namespace" rule is used for instance vars, at least in the standard library.
But, I think accessing instance vars like that should be mainly used for struct comparison, or accessing instance vars from types in the same hierarchy. If you really need to access an instance var from an unrelated type, even in the same namespace, you can always define a
protectedmethod, which gives you that access (like I did in this PR).I was also hesitant to remove point 4, because now you can't say "
exp.@varhas protected access", but I expect a doc page explaining those three points without mentioningprotectedaccess, and we are done. Plus, it's a super rare thing to need, so understanding this rule and how it's different fromprotectedwon't be needed for most of users.Separate notes:
IO#has_non_utf8_encoding?as:nodoc:becauseChar#to_suses it. The other alternative is to doio.encoding == "UTF-8"but that's probably much slower._varto test the value of some instance vars. These are necessary because the tests check implementation details.