-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix crystal_type_id for interpreter #16433
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
base: master
Are you sure you want to change the base?
Fix crystal_type_id for interpreter #16433
Conversation
5906499 to
d8d5400
Compare
|
Removing The segfault error comes from below test: crystal/spec/compiler/interpreter/types_spec.cr Lines 100 to 102 in 2f90e2b
And it doesn't segfault if you run that code with interpreter directly. Gemini AI's analysis shows that it's caused by different prelude setup for the test environment: The !owner.nil_type? check is needed because of the test environment. In a standard Crystal program, src/nil.cr is loaded, which defines Nil#crystal_type_id. Calls on nil dispatch to this method and never reach the primitive. However, spec/compiler/interpreter/types_spec.cr runs with a minimal prelude (prelude: "primitives") that does not load src/nil.cr. In this environment, Nil inherits Object#crystal_type_id (the primitive). I think it's better to remove or refactor the segfaulting test instead of adding |
|
|
||
| it "interprets crystal_type_id for nil" do | ||
| interpret("nil.crystal_type_id").should eq(0) | ||
| interpret("nil.crystal_type_id", prelude: "prelude").should eq("0") |
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.
Issue: why does this spec now requires the prelude, and why is the value now a string when it should be an i32? 🤨
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.
The crystal_type_id is defined in Nil struct:
Lines 53 to 55 in 35dce72
| def crystal_type_id | |
| 0 | |
| end |
The prelude is required to load src/nil.cr
I have no idea why the value is now a string when prelude is included.
@ysbaddaden
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.
Gemini AI's analysis of cause of "0" instead of 0:
The expected value is a string because of how the interpret helper method works when the prelude: "prelude" argument is passed.
In spec/compiler/interpreter/spec_helper.cr, the interpret method has two modes:
- Default (prelude: "primitives"): It runs the code in the same process and returns the actual result value (e.g., Int32).
- Separate Process (prelude: "prelude" or others): It runs the code in a separate process using interpret_in_separate_process. This method executes the code, prints the result to STDOUT, and returns that output as a String.
Since the test case at line 101 uses prelude: "prelude", the interpreter runs nil.crystal_type_id, gets the result 0 (Int32), prints it as "0", and returns that string. This is why the expectation is .should eq("0").
Fixes #14967
Generated by Gemini AI:
Root Cause
The interpreter implementation of crystal_type_id was incorrectly using the compile-time type of the variable to determine the Type ID. In the case of Bar.new.as(Foo).crystal_type_id, the variable's compile-time type is Foo, so the interpreter returned Foo's ID. However, the actual object in memory is an instance of Bar.
The Fix
I modified the interpreter (src/compiler/crystal/interpreter/primitives.cr) to handle reference types dynamically. Instead of using the static type ID, the interpreter now:
Puts the object pointer on the stack.
Reads the first 4 bytes from that pointer (pointer_get(4)).
This retrieves the runtime Type ID stored in the object's header, which correctly identifies it as Bar.
Compiler vs. Interpreter Implementation
Compiler (Codegen): Generates LLVM instructions to load the type ID from the object instance in memory at runtime (effectively object->type_id).
Interpreter (Old): "Optimized" this by simply pushing the constant integer ID of the static type known at compile time (put_i32 type_id(static_type)). This optimization was invalid for polymorphic types cast to a base class.
Interpreter (New): Now mimics the compiler's behavior for reference types by reading the actual Type ID from the object's memory.
Why this is Reasonable
This fix ensures parity between interpreted and compiled execution. In Crystal, objects store their Type ID at offset 0 of their memory layout. Reading these 4 bytes is the correct, low-level mechanism to identify an object's concrete type at runtime, which is the specific purpose of crystal_type_id.