-
-
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: correct to handle instance variable assignment inside block on global #4324
Compiler: correct to handle instance variable assignment inside block on global #4324
Conversation
# `InstanceVar` assignment appered in block is not checked | ||
# by `Crystal::InstanceVarsInitializerVisitor` because this block | ||
# may be passed to a macro. So, it checks here. | ||
if current_type.is_a?(Program) || current_type.is_a?(FileModule) |
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.
Shouldn't the check be with!current_type.is_a?(InstanceVarContainer)
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.
Apologize for the late reply. I missed your comment, sorry...
For example, InstanceVarsInitializerVisitor
treats only Program
and FileModule
as global type. (here)
Perhaps your suggestion is correct, but I don't know really.
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 was considering, then I think !current_type.is_a?(InstanceVarContainer)
is not needed because available not InstanceVarContainer
current_type
is one of EnumType
and LibType
as I know, but both type's body cannot contain assignment (and any expressions). Even if enum
or lib
allow to include macro calls in body, assignment is rejected on parse phase, I think.
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.
Ok, i think you are right since FileModule < NonGenericModuleType < InstanceVarContainer
.
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.
And there are other places where Program
and FileModule
are used as a condition.
69ef537
to
29aebd6
Compare
29aebd6
to
f25e897
Compare
Thanks @makenowjust 🙇 |
Fix #4323 (comment)