-
-
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
Parser: don't use invalid internal names for arguments like @select
or @@select
#6379
Parser: don't use invalid internal names for arguments like @select
or @@select
#6379
Conversation
d38d330
to
e12a1fa
Compare
Let's please always rewrite to internal temporary locals. I would never have considered the access ivar arg as local by the same name more than an implementation detail, we should break this now and force people to do the assignment themselves. |
@jhass I'd rather not to. I tried, but it was a mess We must simply document this expansion, and that's it. There's also this. |
Making that change introduces silent segfaults and memory corruptions. Also, things that used to refer to an instance variable (or, well, the argument behind an instance variable) will now result in a method access. In fact, when I implemented #6007 and ran the compiler, it crashed because of this, and it was very hard to track down where was the problem. Things are fine right now. This PR is an improvement and fixes a bug. Can we merge it and continue the discussion of doing this all the time in a separate place? |
Like, take this line: https://github.com/crystal-lang/crystal/pull/6007/files#diff-0e6efe33c8b047fbf52512a8cb7d2311L1697 def initialize(@name, @args = [] of Arg, @return_type = nil, @varargs = false, @body = nil, @real_name = name) That currently works, Now, I could fix that bug. But I can't, it's difficult and I don't have time. If someone wants to fix that bug and then change the way arguments work, then go ahead. But until then, I prefer to fix existing bugs (that I find easier :-P) |
It would work if it was simply changed to this: def initialize(name, @args = [] of Arg, @return_type = nil, @varargs = false, @body = nil, @real_name = name)
@name = name It's just that the compiler doesn't complain about the existing line? |
I know, that's what I did. But without doing it, it compiles and simply tries to access |
Yeah, I was just checking I understand the problem. But this issue exists regardless of The only special circumstance with ivar arguments is that it currently compiles and changing the language breaks code without raising a compiler error. I'd say, this is totally acceptable before 1.0. |
For me it's not acceptable. Segmentation faults are not fun. But if you (community) want to go that direction, revive my PR, approve it, and merge it (I won't do it because I'm against that change). |
The segfaults were a completely seperate bug that should be fixed regardless of whether this change is made. The change didn't introduce silent segfaults, it just exposed an existing bug. Please, lets fix the bug then merge the original PR. |
My reasoning is this:
So I see this as "No, let's keep the two existing bugs and wait to fix them at the same time instead of fixing one first, for which we already have a fix, and then the other". The problem is that there's no guarantee we'll fix the bug, or when. This PR provides a fix for an existing bug, without changing existing behavior nor introducing segfaults. I can't see the harm of fixing this. Then when that segfault bug is fixed and we can always used external names for |
yeah, you're absolutely right |
Thank you! And note that I'm not against changing how |
Mmh, I wanted to log an issue for the bug as a perquisite for #4332, but it looks like I didn't fully get it yet, why does this correctly error out? https://carc.in/#/r/4ij0 |
@jhass Interesting! I was going to point you to the issue that shows how to reproduce the bug, 3413, changing the instance variable access with a method that accesses it, but it turns out I can't reproduce #3413 anymore in master, it gives a correct error. I also tried reproducing it with a method access and it gives the same error. I don't know when it got fixed... That's good news. I will try to see if I can apply this PR then to all instance/class variable arguments and see if I don't get a segfault anymore and the errors are easy to fix. |
@asterite a git bisect would be interesting |
@RX14 Yeah, I might try that too... I'm implementing this PR for all instance/class vars but I'm not sure I like it (like before). Sometimes there's code like this: @x : Int32?
def foo(@x)
z = x ? x.abs : 0
end Basically, @x : Int32?
def foo(@x)
z = @x ? @x.abs : 0
end and now it doesn't work, because there's no type filtering for instance vars. So we must do: @x : Int32?
def foo(x)
@x = x
z = x ? x.abs : 0
end Essentially, we lost the ability to use I'm not sure what's the benefit of all this... having to write more code? Also, accessing a local variable is faster than accessing an instance variable. Someone care to tell me what are the benefits of this change? We can simply document that |
False alarm, I can still reproduce the bug: class A
property a : Int32
property b : Int32
def initialize(@a, @b = a)
end
end
a = A.new(1)
puts a.b # => 0 |
To me your example above should be written: @x : Int32?
def foo(@x)
z = (x = @x) ? x.abs : 0
end Assigning to a local var to have a smaller type. Right? |
@asterite the point is that the behavior is entirely non-obvious as it's compiler generated code affecting (or I'd say polluting) the local scope. I would go as far as saying it leads to more brittle code as refactoring (renaming things) can easily lead to changed semantics (lvar access -> method call), as you basically experienced it yourself when trying to change this behavior. Sometimes the longer variant (I wouldn't call it redundant) is better because it makes the code easier to read and follow. It's one less thing to keep in the back of your head when reading code, and this is a quite big one in my opinion. |
Okay, I think we're discussing the general issue with ivar arguments in too many different places where it's mostly off-topic. Let's have a proper discussion reflecting the already mentioned arguments and current issues with this. |
Fixes #5997
Right now this:
is expanded to this:
The problem comes when we use a name that's also a keyword:
The above actually works fine, because the parser creates this and it can create a variable name named "class", it's just the user that can't do this. However, if we pass that to a macro, for example:
then it doesn't work because the code gets stringified and then output in the program, and then it's invalid:
My suggestion was to change the compiler to expand it using external named arguments:
The problem with that is that one can no longer do:
However, if we only do this with names that are keywords, then there's no problem:
That's what this PR does.