Feature #17660: Expose information about which basic methods have been redefined

• Status: Open
• Assignee: -
• Target version: -

[tenderlovemaking (Aaron Patterson)]

I would like to tell if code is redefining methods that can impact
MRI's optimizations. This commit exposes which basic methods have been
redefined. For example:

class Integer
  def +(x); x ** self; end
end

p RubyVM.redefined_methods # => {Integer=>[:+]}

This will allow us to prevent basic method redefinitions from happening
by checking for them in CI environments. For example:

Minitest.after_run {
  fail "Basic methods have been redefine" if RubyVM.redefined_methods.any?
}

[tenderlovemaking (Aaron Patterson)]

• File 0001-Expose-information-about-which-basic-methods-have-be.patch 0001-Expose-information-about-which-basic-methods-have-be.patch added

I forgot to attach the patch, so here it is 😄

[Eregon (Benoit Daloze)]

It sounds useful, +1 from me.
I think we should clarify in the name that only redefined basic methods are reported, and not all redefined methods.
How about redefined_basic_methods?

Putting it under RubyVM increases the issue that RubyVM is becoming less and less CRuby-specific.
But I'm pretty much convinced this will happen anyway since ruby-core doesn't seem to want to avoid that (https://bugs.ruby-lang.org/issues/15752#note-28).
So I'm not against it, just making note of this aspect.

I think having this method under a "VM"-named module makes sense at least.
We should consider this method like any other new method, i.e., RubyVM is de facto no longer experimental.

Another place where this method might make sense is as a instance method of Class (or Module).

[marcandre (Marc-Andre Lafortune)]

I don't object (actually I needed that method for tests in backports gem), but I wonder why a C method is preferable to a pure Ruby gem?

[Eregon (Benoit Daloze)]

@marcandre (Marc-Andre Lafortune) the information about redefined basic methods is not available to Ruby or C extensions AFAIK (since it is some kind of VM implementation detail).

I guess your reply reinforces my point that we should clarify this is only about basic methods (e.g., that have their own bytecode instruction in CRuby, or are AST-inlined in TruffleRuby), and where checking if a method is redefined is enough to perform the logic inline (typically used for "leaf" classes).

[marcandre (Marc-Andre Lafortune)]

Oh, IIUC, knowing if a particular method (basic or not) has been redefined is available in pure Ruby, but not the information about which methods are "basic" methods (for any given Ruby version).

[tenderlovemaking (Aaron Patterson)]

Eregon (Benoit Daloze) wrote in #note-2:

It sounds useful, +1 from me.
I think we should clarify in the name that only redefined basic methods are reported, and not all redefined methods.
How about redefined_basic_methods?

This sound good to me. I wasn't very happy with the name 😄

Putting it under RubyVM increases the issue that RubyVM is becoming less and less CRuby-specific.
But I'm pretty much convinced this will happen anyway since ruby-core doesn't seem to want to avoid that (https://bugs.ruby-lang.org/issues/15752#note-28).
So I'm not against it, just making note of this aspect.

I don't really have an opinion where to put this new method, but it's definitely CRuby specific and I want to make sure that fact is reflected. I kind of assumed the methods on RubyVM were specific to the VM being run, so some methods change or are optional depending on the runtime (similar to methods on GC). One example is RubyVM.stat. The values returned on MRI don't make sense in JRuby, so I wouldn't expect it to be implemented.

I think having this method under a "VM"-named module makes sense at least.
We should consider this method like any other new method, i.e., RubyVM is de facto no longer experimental.

Another place where this method might make sense is as a instance method of Class (or Module).

As I said, I don't have any strong opinion about where to put this method as long as a) I can have the method, and b) it communicates to the user that this is CRuby specific and only for basic operations like Integer#+ or NilClass#nil?, etc.

[Eregon (Benoit Daloze)]

tenderlovemaking (Aaron Patterson) wrote in #note-6:

this is CRuby specific and only for basic operations like Integer#+ or NilClass#nil?, etc.

It is not CRuby-specific, in fact this new method does make sense on at least TruffleRuby too.
(https://github.com/oracle/truffleruby/blob/cbb97d56017a10497925e062c87b55bdea545b19/src/main/java/org/truffleruby/core/inlined/CoreMethodAssumptions.java#L63 for details)

RubyVM.stat is also not CRuby-specific, other Rubies might just have different keys.
GC.stat is similar, and is already implemented on JRuby & TruffleRuby, with slightly different keys.
In general, nothing is CRuby-specific, because gems will rely on it and then other Ruby implementations typically prefer to implement it than change all gems depending on it
(some gems use a feature check so it doesn't fail, but typically the extra insight from those methods is useful).

It seems good to mention in the docs that the set of redefined methods reported might change by version and Ruby implementation.

[Dan0042 (Daniel DeLorme)]

Does this work with refinements?

module X
  refine Integer do
    def +(x); x ** self; end
  end
end

In that case RubyVM.redefined_basic_methods should only return {Integer=>[:+]} if it's called from a context where the refinement is active? Or is this irrelevant to the use case? I'm not sure what's the use case precisely. Is the purpose only to forbid redefinitions that would negatively affect performance?