Data object encoding

[nevans]

This adds basic encoding/decoding for ruby 3.2 Data objects, using !ruby/data:ClassName (a mapping of member names to member values) and !ruby/data-with-ivars:ClassName tags (a mapping with members and ivars mappings). Like Marshal, this uses rb_struct_initialize to assign member values to new Data objects.

---

Originally (in the first commit), I updated Visitor::ToRuby to simply call data.send(:initialize, **members). But #initialize freezes the object, so we can't set instance variables afterwards.

The second commit sets instance variables before calling #initialize, which I think will lead to unhappy surprises for some users. Unfortunately, Data doesn't give us any other mechanism for setting the members... except for the C API.

The third commit works around the above issue by copying how Marshal loads Data objects, delegating ToRuby#init_struct(obj, members) to rb_struct_initialize. I think this is the least surprising behavior for users. This also bypasses any user-defined #initialize method (which may be seen as a feature or a bug).

[nevans]

FYI: I haven't put any guards in yet to make this work for older versions of ruby (before Data was defined).

[nevans]

I'll update this to pass for earlier versions of ruby.

But I'm also looking for feedback on the approach. The more I think about it, the more I'm thinking this should use rb_struct_initialize from the C API, then copy over any ivars, then freeze. Unless I'm misreading the C code (very possible), that will skip any user-defined #initialize override, and just use the underlying Struct implementation... and, although that's not identical to rb_data_initialize_m, its close enough and it's exported and in the headers. That's what Marshal does, since Marshal just treats Data like Struct... which also means that Marshal does not freeze Data objects!

[nevans]

For what it's worth, I've updated the PR to:

1. pass with ruby versions down to 2.5 (at least on my machine)
2. use rb_struct_initialize from the C API

[nevans]

Sorry. I thought I had it passing before, but I must have made a mistake in my earlier tests with ruby 2.5 (perhaps I had somehow disabled warnings locally)? I've changed the guards from defined?(::Data.define) to RUBY_VERSION >= "3.1.0". Unfortunately, it's impossible to check if a method is defined without triggering deprecated constant warnings.

[nevans]

Any thoughts about this approach? (Is it too close to the 3.4.0 release to hope for getting support for Data in time for 3.4?)

[tenderlove]

@nevans I'm not thrilled about adding more C code, but I understand why we need it. I think if we introduce some C code we'll have to figure out the JRuby side of things though. cc @headius

[headius]

It would be possible to call JRuby's internal API from Ruby, but since we already have an extension for this library we can just add it there.

This feature and the tests provided are guarded to only happen on Ruby 3.2+, so JRuby 9.4 would not be affected (it is 3.1 compatible) and JRuby 10 (3.4 compatible) is unreleased so we have some time to add JRuby support.

[nevans]

@tenderlove Thanks. I did briefly consider abusing Marshal.dump/load with some evil string manipulation. If psych didn't already have a C extension, I probably would've experimented with that approach. 😜

@headius I think that I may have screwed up the ruby 3.2 guard, the last time I updated my branch. I need to rebase anyway, so I'll fix it and double check that the tests pass under older ruby versions.

[nevans]

@tenderlove Okay, I rebased on the latest master and fixed the tests under ruby 3.1.

I also changed the tests to use omit "Data requires ruby >= 3.2" if RUBY_VERSION < "3.2", rather than conditionally defining the test methods. IMO, it's better to have the more explicit test output when skipping tests. But I'll change that back if you'd prefer clean test output for older ruby versions.

[eregon]

I think this is very brittle, and probably only works on CRuby due to this bug or similar: https://bugs.ruby-lang.org/issues/18632
i.e. this is passing [{ ... }] to rb_struct_initialize(), i.e. a positional Hash, not keyword arguments.
Normal behavior would be to treat that Hash as the value of the first member, and don't assign other members. But it seems to accidentally work.

It would be a lot safer to pass the values as a simple Array instead, and query the members from the class by calling struct_or_data_class.members.
From truffleruby/truffleruby#4012 (comment)

[eregon]

I'm trying to repro this in new C API specs but I actually can't, they fail on CRuby too:

      it "accepts a single Hash in an Array" do
        @klass = @s.rb_data_define(nil, "a", "b", "c")
        data = @klass.allocate
        @s.rb_struct_initialize(data, [{ a: 1, b: 2, c: 3 }]).should == nil
        data.a.should == 1
        data.b.should == 2
        data.c.should == 3
      end

gives

ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
2)
C-API Data function rb_struct_initialize accepts a single Hash in an Array FAILED
Expected {:a=>1, :b=>2, :c=>3} == 1
to be truthy but was false

From experimenting by tweaking those specs it only works when using keyword_init: true explicitly (like Struct.new(:a, :b, :c, keyword_init: true)) , and just never works for Data.

Maybe it "works" due to init_struct getting kwargs and accidentally propagating that flag to other C calls or so (just a guess).
Looks like it in https://github.com/ruby/ruby/blob/48c7f349f68846e10d60ae77ad299a38ee014479/struct.c#L766

[eregon]

In marshal.c https://github.com/ruby/ruby/blob/48c7f349f68846e10d60ae77ad299a38ee014479/marshal.c#L2150-L2176
CRuby uses a Hash only if rb_struct_s_keyword_init(), in all other cases it uses an Array of values.

[eregon]

Since for this use case in Psych we only care about Data, not Struct, we should pass an Array of values (same as what Marshal does for Data, as far as I can tell rb_struct_s_keyword_init() is always false for Data classes).
@nevans Would you have time to fix this?

[eregon]

One last thought is it would really be good to ask for rb_data_initialize() upstream at https://bugs.ruby-lang.org/, because reusing rb_struct_initialize() for Data feels like a pretty huge hack, given it doesn't run any function related to Data, only rb_struct_initialize_m() which is not meant to be used with Data.
Of course that can't be used now, but would enable a cleaner way in the future.

[eregon]

Fix in #749

[eregon]

Originally (in the first commit), I updated Visitor::ToRuby to simply call data.send(:initialize, **members). But #initialize freezes the object, so we can't set instance variables afterwards.

I thought maybe we could just not serialize ivars of Data instances and not try to set them, because the user-defined initialize is the only thing that can set ivars on Data instances (after it's frozen), so initialize will just set the ivars again with data.send(:initialize, **members).
However, that doesn't work because the values of these ivars may be mutable (like an Array), and such changes then would be lost.
Also if a value of these ivars is also referenced by other objects in the same Psych.dump then upon unsafe_load they would be separate objects.

IOW, yeah I think we need rb_struct_initialize() (until we have a better API).