Feature #21981: Remove CREF rewriting for methods on cloned classes/modules

• Status: Closed
• Assignee: -
• Target version: -

[jhawthorn (John Hawthorn)]

When a class or module is cloned, Ruby currently rewrites the CREF chain of each copied method so that it points at the new class instead of the original. I don't think this should happen, and methods on a cloned class should retain their original lexical scope.

This behaviour was introduced in Ruby 1.9 to fix [Bug #7107], which was reporting that a constant in the original class wasn't available. I think it was done this way because of Ruby's implementation at that time rather than a design choice. This had further changes and fixes in [Bug #10885], [Bug #15877] which were crashes or inconsistencies (stale cache). I'd like us to revisit the design of this behaviour so that it's more intuitive and consistent.

class C
  A = 1
  def a; A; end
end

D = C.clone
D.const_set(:A, 2)

p C.new.a  # => 1
p D.new.a  # => 2, but I think it should be 1

In most all other cases in Ruby, methods keep their original lexical scope for constant references: inheritance, mixins, define_method(..., klass.instance_method(...)), etc. Cloning a class should behave the same.

I think cloning a class is fairly rare, and setting constants on the cloned class more so, but this could be a compatibility issue. This affects @@class_variables as well as CONSTANTS. FWIW, truffleruby doesn't seem to have this behaviour (which makes sense, part of why I want to remove it is that it's awkward for JIT compiling 😅).

https://github.com/ruby/ruby/pull/16651

[jhawthorn (John Hawthorn)]

• Description updated (diff)

[matz (Yukihiro Matsumoto)]

Agreed in principle. The current behavior was a workaround for #7107, not a design decision, and the follow-ups in #10885 and #15877 were patches on top of that. Keeping the original lexical scope is consistent with inheritance, mixins, and UnboundMethod rebinding.

To be explicit: with this change, D.new.a should return 1, but

class D
  def b; A; end
end

D.new.b should return 2, since b is defined in D's lexical scope. That's the right behavior.

Compatibility is the only concern. Let's land it on master and see what breaks.

Matz.

[jhawthorn (John Hawthorn)]

• Status changed from Open to Closed

Applied in changeset git|88095eb19621b80f3547abe32adbf3f3c82c42ad.

---

Remove CREF rewriting for cloned classes/modules

When a class or module was cloned, rb_vm_rewrite_cref would rewrite the
CREF chain of each cloned method to point at the new class. Remove this
special case so that clone behaves consistently with all other ways of
adopting a method.

This also removes the RCLASS_CLONED flag which was used to prevent
constant inline cache sharing between cloned classes [Bug #15877], and
the vm_cref_new_use_prev helper which only existed for CREF rewriting.

[Feature #21981]