-
-
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
Compiler: evaluate instance var initializers at the metaclass level #6414
Compiler: evaluate instance var initializers at the metaclass level #6414
Conversation
9ae5664
to
c350121
Compare
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 want to say no, the current behavior makes total sense, but that's just me doing too much Java and ruby behaves in the proposed way, so 👍
In my opinion it seems logical that an ivar assignment behaves the same whether it is inside an On the other hand, it's probably not that important and having a consistent rule is fine either way. I just don't see an issue with |
Yeah, maybe it should run at the instance level. I'm not sure yet, so for now let's not merge this. |
ivar assignments are done at the class level so why should they be resolved differently? (see Ruby) |
@Sija @straight-shoota What do you mean? There's no such thing in Ruby. When you do |
@Sija Even if Ruby was different... In Crystal |
@asterite @straight-shoota Sorry, my bad! I already forgot that in Ruby |
And you can always reach class scope with |
Actually, I tried to see if I can fix a few issues regarding initializers and instance vars... but it's all a big mess, in addition to how generics work or are implemented. I don't feel like fixing this by adding more patches and hacks. As usual, I believe it's better to have a good portion of time to rewrite the compiler from scratch, in a readable, efficient, well thought-out way (though at least I won't do that, ever, but if Crystal ever gets enough money to do it, I'd advise to do it). |
@asterite I'm sure rewriting or refactoring portions of the compiler will be far more productive then throwing out the entire thing. If you're working on fixing semantics, then you probably dont want to be testing that with freshly written buggy codegen code. |
Foo.new | ||
), | ||
"undefined local variable or method 'bar'" |
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.
can we detect this and emit a specialized "did you mean" error message? I suspect it wouldn't be easy but worth asking in case it is.
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 think it's possible, if a lookup fails, see if there's a method with that name in the class scope... but I don't feel like implement that right now (someone else can try it, though)
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 think this change make sense, at least, today.
It would be great to eventually allow the declaration-initializers lines to be evaluated in context of the instance, but only when that is made in a sound way. Rules for typing, order, dependencies of values need to be considered.
With this change, initializers of constants values (literals, loggers, etc) still work.
So 👍 on my side. And maybe in the future if needed we can relax this a bit.
@asterite any final though given your comment
I don't think there's a big pressure to merge this, or do something regarding this. It can be tackled in the future. |
Fixes #5876
This currently compiles:
However, it makes very little sense to run an instance var initializer at the instance level, because that's what we are initializing.
Java allows that behavior, but C# rejects it, demanding that
bar
has to be a static method. That makes much more sense to me.In any case, you can initialize it at the instance level if you want to, in an
initialize
method:That makes it much more clear that this is run at the instance level, and additionally the compiler will check that the instance var isn't used before it's assigned a value, etc.
It also makes sense from a scope level. If we do:
So now it's uniform:
This is a breaking change, but I doubt there's a lot of code out there that uses this kind of initializers.