Skip to content
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

Interpreter: set correct scope for class var initializer #12441

Merged
merged 1 commit into from Sep 5, 2022

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 2, 2022

Fixes #12439

@@ -126,5 +126,23 @@ describe Crystal::Repl::Interpreter do
a
CODE
end

it "finds self in class var initializer (#12439)" do
interpret(<<-CODE).should eq(42)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is partly off-topic because it's very general and can't immediately be solved in this PR.

I've been thinking about how interpreter specs are being handled, and something keeps bothering me:

Adding such a spec to just the interpreter seems wrong to me. This just describes behavior of Crystal language; just because the interpreter happened to have it wrong, doesn't mean it should be an interpreter-only spec. It would also be just as valuable for normal compiler. And it would be trivial to just reuse this exact code snippet and expectation and apply it to the general compiler.

Going further, it seems that all specs that look like this (code snippet + expected result) can and should be defined in one place and run for both the compiler and interpreter. When doing that, we will likely see that many of such specs actually fail on the interpreter in the current state. But, isn't that a good thing? What if the compiler actually already has a spec that would show the same failure and so we wouldn't need to wait until some user happens to run into this issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

Maybe an initial way to do this would be having a helper method that runs some code in compiled and interpreted mode? Then we can move more and more specs to be like that, until eventually all or most of them run in that way, and then that could be the default way to run things.

@straight-shoota straight-shoota added this to the 1.6.0 milestone Sep 5, 2022
@asterite asterite merged commit 2894094 into master Sep 5, 2022
@asterite asterite deleted the bug/interpreter-class-var-initializer-scope branch September 5, 2022 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpretor unable to resolve self when in class context
3 participants