-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Restrict exp.@var
to types in the same hierarchy
#6358
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"
end Also 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the top-level is excluded from accessing ivars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the main thing this PR changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.@var
only works if:exp
, orexp
, orexp
inherits the current type, orthe current type and the type ofexp
live 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
protected
method, 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.@var
has protected access", but I expect a doc page explaining those three points without mentioningprotected
access, and we are done. Plus, it's a super rare thing to need, so understanding this rule and how it's different fromprotected
won't be needed for most of users.Separate notes:
IO#has_non_utf8_encoding?
as:nodoc:
becauseChar#to_s
uses it. The other alternative is to doio.encoding == "UTF-8"
but that's probably much slower._var
to test the value of some instance vars. These are necessary because the tests check implementation details.