From 3b8336f1aef767d5f75878443bf74c8b145b93ea Mon Sep 17 00:00:00 2001 From: Herwin Date: Fri, 13 Dec 2024 18:52:07 +0100 Subject: [PATCH 1/5] Support Data in Marshal.dump This includes a change to make the classes generated with Data.define subclasses of Data. --- spec/core/marshal/dump_spec.rb | 16 +++++----------- spec/core/marshal/shared/load.rb | 12 ++++++------ src/data.rb | 2 +- src/marshal.rb | 14 ++++++++++++++ test/natalie/data_test.rb | 11 +++++++++++ 5 files changed, 37 insertions(+), 18 deletions(-) diff --git a/spec/core/marshal/dump_spec.rb b/spec/core/marshal/dump_spec.rb index dcf1719f3..8986c806f 100644 --- a/spec/core/marshal/dump_spec.rb +++ b/spec/core/marshal/dump_spec.rb @@ -316,23 +316,17 @@ def _dump(level) ruby_version_is "3.2" do describe "with a Data" do it "dumps a Data" do - NATFIXME 'dump Data', exception: SpecFailedException do - Marshal.dump(MarshalSpec::DataSpec::Measure.new(100, 'km')).should == "\x04\bS:#MarshalSpec::DataSpec::Measure\a:\vamountii:\tunit\"\akm" - end + Marshal.dump(MarshalSpec::DataSpec::Measure.new(100, 'km')).should == "\x04\bS:#MarshalSpec::DataSpec::Measure\a:\vamountii:\tunit\"\akm" end it "dumps an extended Data" do - NATFIXME 'dump Data', exception: SpecFailedException do - obj = MarshalSpec::DataSpec::MeasureExtended.new(100, "km") - Marshal.dump(obj).should == "\x04\bS:+MarshalSpec::DataSpec::MeasureExtended\a:\vamountii:\tunit\"\akm" - end + obj = MarshalSpec::DataSpec::MeasureExtended.new(100, "km") + Marshal.dump(obj).should == "\x04\bS:+MarshalSpec::DataSpec::MeasureExtended\a:\vamountii:\tunit\"\akm" end it "ignores overridden name method" do - NATFIXME 'dump Data', exception: SpecFailedException do - obj = MarshalSpec::DataSpec::MeasureWithOverriddenName.new(100, "km") - Marshal.dump(obj).should == "\x04\bS:5MarshalSpec::DataSpec::MeasureWithOverriddenName\a:\vamountii:\tunit\"\akm" - end + obj = MarshalSpec::DataSpec::MeasureWithOverriddenName.new(100, "km") + Marshal.dump(obj).should == "\x04\bS:5MarshalSpec::DataSpec::MeasureWithOverriddenName\a:\vamountii:\tunit\"\akm" end it "raises TypeError with an anonymous Struct" do diff --git a/spec/core/marshal/shared/load.rb b/spec/core/marshal/shared/load.rb index 3560d91e3..620fd953e 100644 --- a/spec/core/marshal/shared/load.rb +++ b/spec/core/marshal/shared/load.rb @@ -870,9 +870,9 @@ def io.binmode; raise "binmode"; end it "loads a Data" do obj = MarshalSpec::DataSpec::Measure.new(100, 'km') dumped = "\x04\bS:#MarshalSpec::DataSpec::Measure\a:\vamountii:\tunit\"\akm" - NATFIXME 'dump and load Data', exception: SpecFailedException do - Marshal.dump(obj).should == dumped + Marshal.dump(obj).should == dumped + NATFIXME 'load Data', exception: ArgumentError, message: 'dump format error' do Marshal.send(@method, dumped).should == obj end end @@ -880,9 +880,9 @@ def io.binmode; raise "binmode"; end it "loads an extended Data" do obj = MarshalSpec::DataSpec::MeasureExtended.new(100, "km") dumped = "\x04\bS:+MarshalSpec::DataSpec::MeasureExtended\a:\vamountii:\tunit\"\akm" - NATFIXME 'dump and load Data', exception: SpecFailedException do - Marshal.dump(obj).should == dumped + Marshal.dump(obj).should == dumped + NATFIXME 'load Data', exception: ArgumentError, message: 'dump format error' do Marshal.send(@method, dumped).should == obj end end @@ -890,9 +890,9 @@ def io.binmode; raise "binmode"; end it "returns a frozen object" do obj = MarshalSpec::DataSpec::Measure.new(100, 'km') dumped = "\x04\bS:#MarshalSpec::DataSpec::Measure\a:\vamountii:\tunit\"\akm" - NATFIXME 'dump and load Data', exception: SpecFailedException do - Marshal.dump(obj).should == dumped + Marshal.dump(obj).should == dumped + NATFIXME 'load Data', exception: ArgumentError, message: 'dump format error' do Marshal.send(@method, dumped).should.frozen? end end diff --git a/src/data.rb b/src/data.rb index 6f1b29e7c..3430f5ec1 100644 --- a/src/data.rb +++ b/src/data.rb @@ -2,7 +2,7 @@ class Data def self.define(*members, &block) members = members.map(&:to_sym) - Class.new do + Class.new(Data) do members.each do |name| define_method(name) { instance_variable_get(:"@#{name}") } end diff --git a/src/marshal.rb b/src/marshal.rb index 528309898..42388b4bb 100644 --- a/src/marshal.rb +++ b/src/marshal.rb @@ -246,6 +246,18 @@ def write_regexp(value) write_encoding_bytes(value) end + def write_data(value) + raise TypeError, "can't dump anonymous class #{value.class}" if value.class.name.nil? + write_char('S') + write(value.class.to_s.to_sym) + values = value.to_h + write_integer_bytes(values.size) + values.each do |name, value| + write(name) + write(value) + end + end + def write_user_marshaled_object_with_allocate(value) write_char('U') write(value.class.to_s.to_sym) @@ -314,6 +326,8 @@ def write(value) write_module(value) elsif value.is_a?(Regexp) write_regexp(value) + elsif value.is_a?(Data) + write_data(value) elsif value.respond_to?(:marshal_dump, true) write_user_marshaled_object_with_allocate(value) elsif value.respond_to?(:_dump, true) diff --git a/test/natalie/data_test.rb b/test/natalie/data_test.rb index 635de562e..83263ffbd 100644 --- a/test/natalie/data_test.rb +++ b/test/natalie/data_test.rb @@ -10,6 +10,10 @@ it 'results in a readable representation' do 1.should == 1 end + + it 'should be a subclass of Data' do + 1.should == 1 + end end end @@ -27,4 +31,11 @@ data.to_s.should == '#' end end + + describe 'Data#is_a?' do + it 'should be a subclass of Data' do + data = DataSpecs::Measure.new(amount: 42, unit: 'km') + data.should be_kind_of(Data) + end + end end From f0e3905b400e0be9a6f1e7f624bab95a066f6b4d Mon Sep 17 00:00:00 2001 From: Herwin Date: Fri, 13 Dec 2024 19:36:55 +0100 Subject: [PATCH 2/5] Fix multilevel constant lookup in Marshal.load This is a bit of hack, it should be fixed in const_get instead. --- spec/core/marshal/shared/load.rb | 10 ++++------ src/marshal.rb | 3 ++- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/spec/core/marshal/shared/load.rb b/spec/core/marshal/shared/load.rb index 620fd953e..c8644efe3 100644 --- a/spec/core/marshal/shared/load.rb +++ b/spec/core/marshal/shared/load.rb @@ -855,13 +855,11 @@ def io.binmode; raise "binmode"; end Thread.current[threadlocal_key] = nil - NATFIXME "It can't find the class for some reason", exception: ArgumentError, message: 'undefined class/module MarshalSpec::StructWithUserInitialize' do - dumped = Marshal.dump(s) - loaded = Marshal.send(@method, dumped) + dumped = Marshal.dump(s) + loaded = Marshal.send(@method, dumped) - Thread.current[threadlocal_key].should == nil - loaded.a.should == 'foo' - end + Thread.current[threadlocal_key].should == nil + loaded.a.should == 'foo' end end diff --git a/src/marshal.rb b/src/marshal.rb index 42388b4bb..514f6bca3 100644 --- a/src/marshal.rb +++ b/src/marshal.rb @@ -608,7 +608,8 @@ def read_value def find_constant(name) begin - Object.const_get(name) + # NATFIXME: Should be supported directly in const_get + name.to_s.split('::').reduce(Object) { |memo, part| memo.const_get(part) } rescue NameError raise ArgumentError, "undefined class/module #{name}" end From 73daef5e0eb785e8d3afcca23cc49a83460f12e5 Mon Sep 17 00:00:00 2001 From: Herwin Date: Fri, 13 Dec 2024 19:30:21 +0100 Subject: [PATCH 3/5] Write local specs for Data#== --- test/natalie/data_test.rb | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/natalie/data_test.rb b/test/natalie/data_test.rb index 83263ffbd..2b29982f5 100644 --- a/test/natalie/data_test.rb +++ b/test/natalie/data_test.rb @@ -14,6 +14,22 @@ it 'should be a subclass of Data' do 1.should == 1 end + + it "returns true if the other is the same object" do + 1.should == 1 + end + + it "returns true if the other has all the same fields" do + 1.should == 1 + end + + it "returns false if the other is a different object or has different fields" do + 1.should == 1 + end + + it "returns false if other is of a different class" do + 1.should == 1 + end end end @@ -38,4 +54,30 @@ data.should be_kind_of(Data) end end + + describe 'Data#==' do + it "returns true if the other is the same object" do + measure = same_measure = DataSpecs::Measure.new(amount: 42, unit: 'km') + measure.should == same_measure + end + + it "returns true if the other has all the same fields" do + measure = DataSpecs::Measure.new(amount: 42, unit: 'km') + similar_measure = DataSpecs::Measure.new(amount: 42, unit: 'km') + measure.should == similar_measure + end + + it "returns false if the other is a different object or has different fields" do + measure = DataSpecs::Measure.new(amount: 42, unit: 'km') + different_measure = DataSpecs::Measure.new(amount: 26, unit: 'miles') + measure.should != different_measure + end + + it "returns false if other is of a different class" do + measure = DataSpecs::Measure.new(amount: 42, unit: 'km') + klass = Data.define(:amount, :unit) + clone = klass.new(amount: 42, unit: 'km') + measure.should != clone + end + end end From 42290f734ca9447799774b5af6f5d62d64e36610 Mon Sep 17 00:00:00 2001 From: Herwin Date: Fri, 13 Dec 2024 19:31:56 +0100 Subject: [PATCH 4/5] Implement Data#== --- src/data.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/data.rb b/src/data.rb index 3430f5ec1..100441d93 100644 --- a/src/data.rb +++ b/src/data.rb @@ -55,6 +55,10 @@ def self.define(*members, &block) end end + define_method :== do |other| + self.class == other.class && to_h == other.to_h + end + define_singleton_method(:[]) { |*args, **kwargs| new(*args, **kwargs) } define_singleton_method(:members) { members } From b8cdb404643afede4cb95fa5e988889e23a0285d Mon Sep 17 00:00:00 2001 From: Herwin Date: Fri, 13 Dec 2024 19:24:07 +0100 Subject: [PATCH 5/5] Support Data in Marshal.load/Marshal.restore --- spec/core/marshal/shared/load.rb | 22 +++++++--------------- src/marshal.rb | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/spec/core/marshal/shared/load.rb b/spec/core/marshal/shared/load.rb index c8644efe3..4dd398ea7 100644 --- a/spec/core/marshal/shared/load.rb +++ b/spec/core/marshal/shared/load.rb @@ -830,20 +830,18 @@ def io.binmode; raise "binmode"; end it "loads a struct having ivar" do obj = Struct.new("Thick").new obj.instance_variable_set(:@foo, 5) - NATFIXME 'loads a struct having ivar', exception: ArgumentError, message: 'dump format error' do + NATFIXME 'ivar names', exception: NameError, message: "`@@foo' is not allowed as an instance variable name" do reloaded = Marshal.send(@method, "\004\bIS:\022Struct::Thick\000\006:\t@fooi\n") reloaded.should == obj reloaded.instance_variable_get(:@foo).should == 5 - Struct.send(:remove_const, :Thick) end + Struct.send(:remove_const, :Thick) end it "loads a struct having fields" do obj = Struct.new("Ure1", :a, :b).new - NATFIXME 'loads a struct having fields', exception: ArgumentError, message: 'dump format error' do - Marshal.send(@method, "\004\bS:\021Struct::Ure1\a:\006a0:\006b0").should == obj - Struct.send(:remove_const, :Ure1) - end + Marshal.send(@method, "\004\bS:\021Struct::Ure1\a:\006a0:\006b0").should == obj + Struct.send(:remove_const, :Ure1) end it "does not call initialize on the unmarshaled struct" do @@ -870,9 +868,7 @@ def io.binmode; raise "binmode"; end dumped = "\x04\bS:#MarshalSpec::DataSpec::Measure\a:\vamountii:\tunit\"\akm" Marshal.dump(obj).should == dumped - NATFIXME 'load Data', exception: ArgumentError, message: 'dump format error' do - Marshal.send(@method, dumped).should == obj - end + Marshal.send(@method, dumped).should == obj end it "loads an extended Data" do @@ -880,9 +876,7 @@ def io.binmode; raise "binmode"; end dumped = "\x04\bS:+MarshalSpec::DataSpec::MeasureExtended\a:\vamountii:\tunit\"\akm" Marshal.dump(obj).should == dumped - NATFIXME 'load Data', exception: ArgumentError, message: 'dump format error' do - Marshal.send(@method, dumped).should == obj - end + Marshal.send(@method, dumped).should == obj end it "returns a frozen object" do @@ -890,9 +884,7 @@ def io.binmode; raise "binmode"; end dumped = "\x04\bS:#MarshalSpec::DataSpec::Measure\a:\vamountii:\tunit\"\akm" Marshal.dump(obj).should == dumped - NATFIXME 'load Data', exception: ArgumentError, message: 'dump format error' do - Marshal.send(@method, dumped).should.frozen? - end + Marshal.send(@method, dumped).should.frozen? end end end diff --git a/src/marshal.rb b/src/marshal.rb index 514f6bca3..c4a346b5c 100644 --- a/src/marshal.rb +++ b/src/marshal.rb @@ -529,6 +529,24 @@ def read_user_marshaled_object_without_allocate object_class._load(data) end + def read_struct + name = read_value + object_class = find_constant(name) + size = read_integer + values = size.times.each_with_object({}) do |_, tmp| + name = read_value + value = read_value + tmp[name] = value + end + if object_class.ancestors.include?(Data) + object_class.new(**values) + else + object = object_class.allocate + values.each { |k, v| object.send(:"#{k}=", v) } + object + end + end + def read_object name = read_value object_class = find_constant(name) @@ -595,6 +613,8 @@ def read_value read_user_marshaled_object_with_allocate when 'u' read_user_marshaled_object_without_allocate + when 'S' + read_struct when 'o' read_object when 'I'