Can't override `::Data` methods in abstract class

[nevans]

It looks like it isn't possible to override instance methods in a ::Data subclass that is simply inheriting but not defining any fields.

#!/usr/bin/env ruby

puts ?=*72
puts "Ruby Engine:   #{RUBY_ENGINE} #{RUBY_ENGINE_VERSION}"

Simple = Data.define(:foo) do
  def deconstruct            = [:ok,        *super]
  def deconstruct_keys(keys) = {xtra: :ok, **super}
end

class Inherits < Data.define(:foo)
  def deconstruct            = [:ok,        *super]
  def deconstruct_keys(keys) = {xtra: :ok, **super}
end

class Abstract < Data
  def deconstruct            = [:ok,        *super]
  def deconstruct_keys(keys) = {xtra: :ok, **super}
end

Layered = Abstract.define(:foo)

simple  = Simple[foo: :bar]
inherit = Inherits[foo: :bar]
layered = Layered[foo: :bar]
puts "  simple.deconstruct:            %p" % [simple.deconstruct]
puts "  simple.deconstruct_keys(nil):  %p" % [simple.deconstruct_keys(nil)]
puts "  inherit.deconstruct:           %p" % [inherit.deconstruct]
puts "  inherit.deconstruct_keys(nil): %p" % [inherit.deconstruct_keys(nil)]
puts "  layered.deconstruct:           %p" % [layered.deconstruct]
puts "  layered.deconstruct_keys(nil): %p" % [layered.deconstruct_keys(nil)]

__END__
$ ruby test.rb && RBENV_VERSION=truffleruby-dev ruby test.rb
========================================================================
Ruby Engine:   ruby 3.4.4
  simple.deconstruct:            [:ok, :bar]
  simple.deconstruct_keys(nil):  {xtra: :ok, foo: :bar}
  inherit.deconstruct:           [:ok, :bar]
  inherit.deconstruct_keys(nil): {xtra: :ok, foo: :bar}
  layered.deconstruct:           [:ok, :bar]
  layered.deconstruct_keys(nil): {xtra: :ok, foo: :bar}
========================================================================
Ruby Engine:   truffleruby 25.0.0-dev-e36b2ff4
  simple.deconstruct:            [:ok, :bar]
  simple.deconstruct_keys(nil):  {:xtra=>:ok, :foo=>:bar}
  inherit.deconstruct:           [:ok, :bar]
  inherit.deconstruct_keys(nil): {:xtra=>:ok, :foo=>:bar}
  layered.deconstruct:           [:bar]
  layered.deconstruct_keys(nil): {:foo=>:bar}

[nevans]

@eregon I think this is the last TruffleRuby issue discovered by ruby/net-imap#470. When this matches CRuby, TruffleRuby will pass all tests without the Data polyfill (not counting the Regexp.linear_time? exclusions).

[eregon]

Mmh, this is a tough one.
I think subclassing Struct or Data directly is generally problematic and should be avoided.

Part of the idea of Struct and Data is to optimize them well by the Ruby implementation.
That includes defining their methods dynamically to "know/hardcode/constant-fold" which members are available, examples for Struct and for Data.
Without this, many Struct/Data methods would have megamorphic calls and be a lot slower, due to e.g. the inline caches seeing the members of all structs/data classes. Notably getting the members through Primitive.class(self)::CLASS_MEMBERS would become a megamorphic constant lookup and that's a clear no-no for performance.

These methods get defined in their own module, so one can still super if defining methods in the Data.define block, as you do in the first example.
That module ends up before the Data subclass in the ancestors:

$ ruby -ve 'class Abstract < Data; def deconstruct = [:ok, *super]; end; Layered = Abstract.define(:foo); o = Layered[foo: :bar]; p o.class.ancestors; p o.deconstruct'
truffleruby 25.0.0-dev-47bbcccd, like ruby 3.3.7, Oracle GraalVM Native [x86_64-linux]
[Layered, #<Module:0xc8>, Abstract, Data, Object, Kernel, BasicObject]
[:bar]

$ ruby -ve 'class Abstract < Data; def deconstruct = [:ok, *super]; end; Layered = Abstract.define(:foo); o = Layered[foo: :bar]; p o.class.ancestors; p o.deconstruct'
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
[Layered, Abstract, Data, Object, Kernel, BasicObject]
[:ok, :bar]

We can't include the module between Abstract and Data because the module is specific to a given Data.define class/its members.
I wonder if we should raise an error if Data.define is called and the receiver is not Data but a subclass. It's incompatible with current CRuby, but it might be better to error than to silently use a very slow copy of Data to handle that edge case (and having to maintain that copy).

Can I ask what do you subclass Data for? Maybe we can find an easy solution not relying on that.

[eregon]

I think a good solution here is to make Abstract a module instead of a Data subclass, and include it in the Data.define classes you need it:

module Abstract
  def deconstruct            = [:ok,        *super]
  def deconstruct_keys(keys) = {xtra: :ok, **super}
end

Layered = Data.define(:foo) { include Abstract }

[nevans]

The code in net-imap that's currently tripping this up is more of a placeholder than anything else. In its current form, it doesn't really need to use Data at all. Likewise, it really doesn't need to override #deconstruct or #deconstruct_keys. It's not a good example. 😆

I've used the pattern elsewhere and can dig up much better examples, but... IMO, the CRuby semantics are obviously and intuitively correct and compatibility with that is more important than performance. And I'm not sure that I want to put myself in the position of defending individual instances, because we can always find an alternative which may feel subjectively more or less stylistically elegant. We can also rewrite the code in C or Rust; that's also always an alternative. 😉

Yuck. 🙁

It seems to me there must be some way around the worst case scenario of a child class breaking inline caches for all parents and cousins. E.g. mixins like Enumerable or Comparable handle the megamorphism problem just fine, right? And superclasses using the template method pattern wouldn't trigger this issue, would they? If this is already handled for polymorphic methods, can we just use those here, and add a layer of indirection? E.g:

class Data
  def initialize(**kwargs)   = __Data_defined_initialize__(**kwargs)
  def initialize_copy(other) = __Data_defined_initialize_copy(other)
  def members                = __Data_defined_members__
  def deconstruct            = __Data_defined_deconstruct__
  def deconstruct_keys(keys) = __Data_defined_deconstruct_keys(keys)
  # etc

  def self.define(*class_members, &block)
    # ...etc...
    instance_methods_module = Module.new
    instance_methods_module.module_eval "#{<<~'RUBY'}", __FILE__, __LINE__+1
      # Ideally these methods would be _hidden_ from normal ruby code.
      # But adding extra private methods is a trivial incompatibility.
      private

      def __Data_defined_initialize__(**kwargs)   = etc
      def __Data_defined_initialize_copy__(other) = etc
      def __Data_defined_members__ = Primitive.class(self).members
      # ...etc...
    RUBY
    # ...etc...
  end
end

I wonder if we should raise an error if Data.define is called and the receiver is not Data but a subclass.

Please don't. The current implementation only breaks overrides of instance methods that are defined on Data (or, that CRuby defines on Data). Most use cases for inheriting from Data aren't overriding those methods and will work with the current implementation. But if you did that, you'd break compatibility with those other cases unnecessarily.

[eregon]

E.g. mixins like Enumerable or Comparable handle the megamorphism problem just fine, right?

Comparable basically has the same problem, even on CRuby, calls to <=> inside Comparable methods will be megamorphic and basically cannot use inline caching and do slow method lookup on every call.
Splitting helps on TruffleRuby for Comparable, but that means every call site to a Comparable method has its own copy of that method, which is then a footprint and warmup overhead, so not ideal (a copy per class using Comparable would be better, but is pretty hard to do).
vtables could help for Comparable, since all methods just call <=>.

Enumerable is similar in that it calls each and that call site becomes megamorphic, unless splitting is used.
Enumerable methods receive many different blocks, but blocks at least don't need method lookup.

I agree that TruffleRuby should be compatible, it just seems hard trade-offs in this case.
As you can see for example here it is expected Data optimizes the methods it defines well and doesn't use a generic loop over members.

In https://bugs.ruby-lang.org/issues/16122#note-69 and following comments I argued that Data should define all the methods on the subclass, or in a module included in the subclass like TruffleRuby does.
Unfortunately the initial Data implementation in CRuby is rather inefficient and ignored this optimization concern, which now brings this kind of semantics difference and might mean CRuby can never optimize Data properly while keeping the exact same semantics. People already noticed it's like 4.5x slower than Struct here and that hasn't been fixed. Maybe CRuby will eventually change on which class/module the Data methods are defined and do it more like TruffleRuby, only time will tell.

---

I'll think more about how to solve this without killing Data performance.
Probably we can define generic slow versions of the methods on Data like CRuby, and still define our specialized overridden methods, but skip to define a specialized method if the super method isn't the one in Data (i.e. like Abstract#deconstruct in that example).
(or just disable all specialized methods if define is called on a subclass of Data, that's how Struct deals with it)
Then we could emit a performance warning for that case, and recommend to use instead:

class Abstract < Data
  def self.define(...)
    data_class = super(...)
    data_class.class_exec do
      def deconstruct            = [:ok,        *super]
      def deconstruct_keys(keys) = {xtra: :ok, **super}
    end
    data_class
  end
end

Layered = Abstract.define(:foo)
layered = Layered[foo: :bar]
p layered.deconstruct # works fine on current TruffleRuby, and better optimized

or #3892 (comment)

But this is a lot of work and complication for such an edge case.

[eregon]

Regarding your idea, def initialize(**kwargs) = __Data_defined_initialize__(**kwargs) that would be a megamorphic call to __Data_defined_initialize__ (unless initialize is split per call site but that's a big overhead we should avoid for Data).

Do I understand correctly this isn't a blocker for net-imap and so it's not urgent to fix this?