From f875d4ee9d88eeee019a5e14ab3a5307e8f42899 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Fri, 13 Jun 2025 05:21:10 +0000 Subject: [PATCH 1/5] Add `Struct.pre_initialize` --- spec/primitives/struct_spec.cr | 75 +++++++++++++++++++ src/compiler/crystal/codegen/primitives.cr | 2 +- .../crystal/interpreter/instructions.cr | 5 +- .../crystal/interpreter/primitives.cr | 13 +++- src/compiler/crystal/semantic/main_visitor.cr | 56 +++++++------- src/primitives.cr | 52 +++++++++++-- 6 files changed, 168 insertions(+), 35 deletions(-) create mode 100644 spec/primitives/struct_spec.cr diff --git a/spec/primitives/struct_spec.cr b/spec/primitives/struct_spec.cr new file mode 100644 index 000000000000..75b2f662ab1f --- /dev/null +++ b/spec/primitives/struct_spec.cr @@ -0,0 +1,75 @@ +require "spec" + +private abstract struct Base +end + +private struct Foo < Base + getter i : Int64 + getter str = "abc" + + def initialize(@i) + end + + def initialize(@str, @i) + end +end + +private struct Bar < Base + getter x : UInt8[128] + + def initialize(@x) + end +end + +private struct Inner +end + +private struct Outer + @x = Inner.new +end + +describe "Primitives: struct" do + describe ".pre_initialize" do + it "doesn't fail on complex ivar initializer if value is discarded (#14325)" do + bar_buffer = Pointer(Outer).malloc(1) + Outer.pre_initialize(bar_buffer) + 1 + end + + it "zeroes the instance data" do + bar = uninitialized Bar + Slice.new(pointerof(bar).as(UInt8*), sizeof(Bar)).fill(0xFF) + Bar.pre_initialize(pointerof(bar)) + bar.x.all?(&.zero?).should be_true + end + + it "runs inline instance initializers" do + foo = uninitialized Foo + Foo.pre_initialize(pointerof(foo)).should be_nil + foo.str.should eq("abc") + end + + it "works when address is on the heap" do + foo_buffer = Pointer(Foo).malloc(1) + Foo.pre_initialize(foo_buffer) + foo_buffer.value.str.should eq("abc") + end + + # see notes in `Struct.pre_initialize` + {% if compare_versions(Crystal::VERSION, "1.2.0") >= 0 %} + it "works with virtual type" do + foo = uninitialized Foo + Foo.as(Base.class).pre_initialize(pointerof(foo)) + foo.str.should eq("abc") + end + {% else %} + pending! "works with virtual type" + {% end %} + + it "raises on abstract virtual type" do + expect_raises(Exception, "Can't pre-initialize abstract struct Base") do + Base.as(Base.class).pre_initialize(Pointer(Void).null) + end + end + end +end diff --git a/src/compiler/crystal/codegen/primitives.cr b/src/compiler/crystal/codegen/primitives.cr index 7eedfdab4adf..ee9729e78397 100644 --- a/src/compiler/crystal/codegen/primitives.cr +++ b/src/compiler/crystal/codegen/primitives.cr @@ -745,7 +745,7 @@ class Crystal::CodeGenVisitor end def codegen_primitive_pre_initialize(node, target_def, call_args) - type = node.type + type = context.type.instance_type base_type = type.is_a?(VirtualType) ? type.base_type : type diff --git a/src/compiler/crystal/interpreter/instructions.cr b/src/compiler/crystal/interpreter/instructions.cr index f8f986f1b44a..43b43ba6fe9d 100644 --- a/src/compiler/crystal/interpreter/instructions.cr +++ b/src/compiler/crystal/interpreter/instructions.cr @@ -1282,7 +1282,10 @@ require "./repl" push: true, code: begin pointer.clear(size) - pointer.as(Int32*).value = type_id + unless type_id == 0 + # 0 stands for any non-reference type + pointer.as(Int32*).value = type_id + end pointer end, }, diff --git a/src/compiler/crystal/interpreter/primitives.cr b/src/compiler/crystal/interpreter/primitives.cr index 619f678ad6bd..44538ea800fb 100644 --- a/src/compiler/crystal/interpreter/primitives.cr +++ b/src/compiler/crystal/interpreter/primitives.cr @@ -187,10 +187,12 @@ class Crystal::Repl::Compiler scope.instance_type end - accept_call_members(node) + accept_call_args(node) - dup sizeof(Pointer(Void)), node: nil - reset_class(aligned_instance_sizeof_type(type), type_id(type), node: node) + # 0 stands for any non-reference type in `reset_class` + # (normally 0 stands for `Nil` so there is no conflict here) + type_id = type.struct? ? 0 : type_id(type) + reset_class(aligned_instance_sizeof_type(type), type_id, node: node) initializer_compiled_defs = @context.type_instance_var_initializers(type) unless initializer_compiled_defs.empty? @@ -202,6 +204,11 @@ class Crystal::Repl::Compiler call compiled_def, node: nil end end + + # `Struct.pre_initialize` does not return a pointer, so always discard it + if !@wants_value || type.struct? + pop(sizeof(Pointer(Void)), node: nil) + end when "tuple_indexer_known_index" unless @wants_value accept_call_members(node) diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index bccbe5d601e1..3895c7e7180e 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2364,17 +2364,22 @@ module Crystal end def visit(node : Primitive) + case node.name + when "pre_initialize" + return visit_pre_initialize node + end + # If the method where this primitive is defined has a return type, use it if return_type = typed_def.return_type node.type = (path_lookup || scope).lookup_type(return_type, free_vars: free_vars) return false end + # TODO: move these into the case expression above and add return types to + # their corresponding methods case node.name when "allocate" visit_allocate node - when "pre_initialize" - visit_pre_initialize node when "pointer_malloc" visit_pointer_malloc node when "pointer_set" @@ -2478,35 +2483,36 @@ module Crystal case instance_type when GenericClassType - node.raise "Can't pre-initialize instance of generic class #{instance_type} without specifying its type vars" + node.raise "Can't pre-initialize instance of #{instance_type.type_desc} #{instance_type} without specifying its type vars" when UnionType node.raise "Can't pre-initialize instance of a union type" - else - if instance_type.abstract? - if instance_type.virtual? - # This is the same as `.initialize` - base_type = instance_type.devirtualize + end - extra = Call.new( - nil, - "raise", - StringLiteral.new("Can't pre-initialize abstract class #{base_type}"), - global: true).at(node) - extra.accept self + if instance_type.abstract? + if instance_type.virtual? + # This is the same as `.initialize` + base_type = instance_type.devirtualize - # This `extra` will replace the Primitive node in CleanupTransformer later on. - node.extra = extra - node.type = @program.no_return - return - else - # If the type is not virtual then we know for sure that the type - # can't be instantiated, and we can produce a compile-time error. - node.raise "Can't pre-initialize abstract class #{instance_type}" - end - end + extra = Call.new( + nil, + "raise", + StringLiteral.new("Can't pre-initialize abstract #{base_type.type_desc} #{base_type}"), + global: true).at(node) + extra.accept self - node.type = instance_type + # This `extra` will replace the Primitive node in CleanupTransformer later on. + node.extra = extra + node.type = @program.no_return + return false + else + # If the type is not virtual then we know for sure that the type + # can't be instantiated, and we can produce a compile-time error. + node.raise "Can't pre-initialize abstract #{instance_type.type_desc} #{instance_type}" + end end + + node.type = instance_type.struct? ? @program.nil_type : instance_type + false end def visit_pointer_malloc(node) diff --git a/src/primitives.cr b/src/primitives.cr index 4f93fac83af3..ec129c252cae 100644 --- a/src/primitives.cr +++ b/src/primitives.cr @@ -58,11 +58,12 @@ class Reference # overloads. It zeroes the memory, sets up the type ID (necessary for dynamic # dispatch), and then runs all inline instance variable initializers. # - # *address* must point to a suitably aligned buffer of at least - # `instance_sizeof(self)` bytes. + # *address* must point to a buffer of at least `instance_sizeof(self)` bytes + # with an alignment of `instance_alignof(self)` or above. `ReferenceStorage` + # fulfils both requirements. # - # WARNING: This method is unsafe, as it assumes the caller is responsible for - # managing the memory at the given *address* manually. + # WARNING: The caller is responsible for managing the memory at the given + # *address*, in particular if the memory is not garbage-collectable. # # ``` # class Foo @@ -88,7 +89,7 @@ class Reference # foo.i # => 123 # ``` # - # See also: `Reference.unsafe_construct`. + # See also: `Reference.unsafe_construct`, `Struct.pre_initialize`. @[Experimental("This API is still under development. Join the discussion about custom reference allocation at [#13481](https://github.com/crystal-lang/crystal/issues/13481).")] @[Primitive(:pre_initialize)] {% if compare_versions(Crystal::VERSION, "1.2.0") >= 0 %} @@ -106,6 +107,47 @@ class Reference {% end %} end +struct Struct + # Performs basic initialization so that the given *address* is ready for use + # as an object's instance data. + # + # More specifically, this is the part of object initialization that occurs + # after memory allocation and before calling one of the `#initialize` + # overloads. It zeroes the memory, and then runs all inline instance variable + # initializers. + # + # *address* must point to a buffer of at least `sizeof(self)` bytes with an + # alignment of `alignof(self)` or above. This can for example be a pointer to + # an uninitialized instance. + # + # ``` + # struct Foo + # getter i : Int64 + # getter str = "abc" + # + # def initialize(@i) + # end + # end + # + # foo = uninitialized Foo + # pointerof(foo).to_slice(1).to_unsafe_bytes.fill(0xFF) + # Foo.pre_initialize(pointerof(foo)) + # foo # => Foo(@i=0, @str="abc") + # ``` + # + # See also: `Reference.pre_initialize`. + @[Experimental("This API is still under development. Join the discussion about custom reference allocation at [#13481](https://github.com/crystal-lang/crystal/issues/13481).")] + @[Primitive(:pre_initialize)] + {% if compare_versions(Crystal::VERSION, "1.2.0") >= 0 %} + def self.pre_initialize(address : Pointer) : Nil + \{% @type %} + end + {% else %} + def self.pre_initialize(address : Pointer) : Nil + end + {% end %} +end + class Class # :nodoc: @[Primitive(:class_crystal_instance_type_id)] From c7f6e0b604f6e6a8811588e48adb646d0b50d99e Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Fri, 13 Jun 2025 05:55:18 +0000 Subject: [PATCH 2/5] fixup --- spec/primitives/struct_spec.cr | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/primitives/struct_spec.cr b/spec/primitives/struct_spec.cr index 75b2f662ab1f..bd268c400df8 100644 --- a/spec/primitives/struct_spec.cr +++ b/spec/primitives/struct_spec.cr @@ -1,9 +1,9 @@ require "spec" -private abstract struct Base +private abstract struct SBase end -private struct Foo < Base +private struct Foo < SBase getter i : Int64 getter str = "abc" @@ -14,7 +14,7 @@ private struct Foo < Base end end -private struct Bar < Base +private struct Bar < SBase getter x : UInt8[128] def initialize(@x) @@ -31,8 +31,8 @@ end describe "Primitives: struct" do describe ".pre_initialize" do it "doesn't fail on complex ivar initializer if value is discarded (#14325)" do - bar_buffer = Pointer(Outer).malloc(1) - Outer.pre_initialize(bar_buffer) + bar = uninitialized Outer + Outer.pre_initialize(pointerof(bar)) 1 end @@ -59,7 +59,7 @@ describe "Primitives: struct" do {% if compare_versions(Crystal::VERSION, "1.2.0") >= 0 %} it "works with virtual type" do foo = uninitialized Foo - Foo.as(Base.class).pre_initialize(pointerof(foo)) + Foo.as(SBase.class).pre_initialize(pointerof(foo)) foo.str.should eq("abc") end {% else %} @@ -67,8 +67,8 @@ describe "Primitives: struct" do {% end %} it "raises on abstract virtual type" do - expect_raises(Exception, "Can't pre-initialize abstract struct Base") do - Base.as(Base.class).pre_initialize(Pointer(Void).null) + expect_raises(Exception, "Can't pre-initialize abstract struct SBase") do + SBase.as(SBase.class).pre_initialize(Pointer(Void).null) end end end From 9abfd39a5e824e7fba9bed90169efd9ff822f823 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Fri, 13 Jun 2025 06:48:15 +0000 Subject: [PATCH 3/5] last is nil --- src/compiler/crystal/codegen/primitives.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/crystal/codegen/primitives.cr b/src/compiler/crystal/codegen/primitives.cr index ee9729e78397..d066b0eb6c53 100644 --- a/src/compiler/crystal/codegen/primitives.cr +++ b/src/compiler/crystal/codegen/primitives.cr @@ -752,7 +752,7 @@ class Crystal::CodeGenVisitor ptr = call_args[target_def.owner.passed_as_self? ? 1 : 0] pre_initialize_aggregate base_type, llvm_struct_type(base_type), ptr - @last = cast_to ptr, type + @last = type.struct? ? llvm_nil : cast_to ptr, type end def codegen_primitive_pointer_malloc(node, target_def, call_args) From 0d74334cf60dd99f005231510cfacee2a518b61f Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Tue, 24 Jun 2025 03:30:56 +0800 Subject: [PATCH 4/5] disallow abstract structs --- spec/compiler/semantic/primitives_spec.cr | 53 +++++++++++++++++++ src/compiler/crystal/semantic/main_visitor.cr | 14 ++++- src/primitives.cr | 3 ++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/spec/compiler/semantic/primitives_spec.cr b/spec/compiler/semantic/primitives_spec.cr index 34b8b3a29501..973d356e2df8 100644 --- a/spec/compiler/semantic/primitives_spec.cr +++ b/spec/compiler/semantic/primitives_spec.cr @@ -451,4 +451,57 @@ describe "Semantic: primitives" do CRYSTAL end end + + describe "Struct.pre_initialize" do + def_struct_pre_initialize = <<-CRYSTAL + struct Struct + @[Primitive(:pre_initialize)] + def self.pre_initialize(address : Pointer) : Nil + {% @type %} + end + end + CRYSTAL + + it "errors on abstract type" do + assert_error <<-CRYSTAL, "Can't pre-initialize abstract struct Foo" + #{def_struct_pre_initialize} + + abstract struct Foo + end + + x = 1 + Foo.pre_initialize(pointerof(x)) + CRYSTAL + end + + it "errors on virtual abstract type" do + assert_error <<-CRYSTAL, "Can't pre-initialize abstract struct Foo" + #{def_struct_pre_initialize} + + abstract struct Foo + end + + struct Bar < Foo + end + + x = 1 + Bar.as(Foo.class).pre_initialize(pointerof(x)) + CRYSTAL + end + + it "errors on abstract pointee type" do + assert_error <<-CRYSTAL, "Can't pre-initialize struct using pointer to abstract struct" + #{def_struct_pre_initialize} + + abstract struct Foo + end + + struct Bar < Foo + end + + x = uninitialized Foo + Bar.pre_initialize(pointerof(x)) + CRYSTAL + end + end end diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index 3895c7e7180e..be2329e2ab73 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2489,7 +2489,7 @@ module Crystal end if instance_type.abstract? - if instance_type.virtual? + if instance_type.virtual? && !instance_type.struct? # This is the same as `.initialize` base_type = instance_type.devirtualize @@ -2507,11 +2507,21 @@ module Crystal else # If the type is not virtual then we know for sure that the type # can't be instantiated, and we can produce a compile-time error. + instance_type = instance_type.devirtualize node.raise "Can't pre-initialize abstract #{instance_type.type_desc} #{instance_type}" end end - node.type = instance_type.struct? ? @program.nil_type : instance_type + if instance_type.struct? + element_type = @vars["address"].type.as(PointerInstanceType).element_type + if element_type.abstract? && element_type.struct? + node.raise "Can't pre-initialize struct using pointer to abstract struct" + end + node.type = @program.nil_type + else + node.type = instance_type + end + false end diff --git a/src/primitives.cr b/src/primitives.cr index ec129c252cae..d5664383f8cb 100644 --- a/src/primitives.cr +++ b/src/primitives.cr @@ -120,6 +120,9 @@ struct Struct # alignment of `alignof(self)` or above. This can for example be a pointer to # an uninitialized instance. # + # This method only works for non-virtual constructions. Neither the struct + # type nor *address*'s pointee type can be an abstract struct. + # # ``` # struct Foo # getter i : Int64 From e36a2bcc1a6a8e50313b472f6935fb91448b51a7 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Tue, 24 Jun 2025 03:38:39 +0800 Subject: [PATCH 5/5] fix specs --- spec/primitives/struct_spec.cr | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/spec/primitives/struct_spec.cr b/spec/primitives/struct_spec.cr index bd268c400df8..e635e1e5f0f6 100644 --- a/spec/primitives/struct_spec.cr +++ b/spec/primitives/struct_spec.cr @@ -1,9 +1,6 @@ require "spec" -private abstract struct SBase -end - -private struct Foo < SBase +private struct Foo getter i : Int64 getter str = "abc" @@ -14,7 +11,7 @@ private struct Foo < SBase end end -private struct Bar < SBase +private struct Bar getter x : UInt8[128] def initialize(@x) @@ -54,22 +51,5 @@ describe "Primitives: struct" do Foo.pre_initialize(foo_buffer) foo_buffer.value.str.should eq("abc") end - - # see notes in `Struct.pre_initialize` - {% if compare_versions(Crystal::VERSION, "1.2.0") >= 0 %} - it "works with virtual type" do - foo = uninitialized Foo - Foo.as(SBase.class).pre_initialize(pointerof(foo)) - foo.str.should eq("abc") - end - {% else %} - pending! "works with virtual type" - {% end %} - - it "raises on abstract virtual type" do - expect_raises(Exception, "Can't pre-initialize abstract struct SBase") do - SBase.as(SBase.class).pre_initialize(Pointer(Void).null) - end - end end end