Skip to content

Remove respond_to check from Class#bind_call #13116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Apr 15, 2025

https://bugs.ruby-lang.org/issues/21267

Class#allocate has an additional rb_obj_respond_to(klass, rb_intern("allocate")) check to forbid allocate being called on a class where it has been made private or undefined, even if used via ex. bind_call.

>> Rational.allocate
(irb):1:in '<main>': undefined method 'allocate' for class Rational (NoMethodError)
>> Class.instance_method(:allocate).bind_call(Rational)
(irb):1:in 'Class#allocate': calling Rational.allocate is prohibited (TypeError)

However I don't think this provides any additional protection from users accessing an uninitialized object, as the user can redefine allocate to anything to bypass the check:

>> Class.instance_method(:allocate).bind_call(Class.new(Rational) {def self.allocate; end})
=> (0/1)

Or even override respond_to_missing?

>> Class.instance_method(:allocate).bind_call(Class.new(Rational) {def self.respond_to_missing? *; true; end})
=> (0/1)

So I think we should remove this check. For classes that we need to forbid allocation we should use rb_undef_alloc_func.

The classes I see this used for are:

  • MatchData
  • Refinement
  • Module
  • Complex
  • Rational

My main motivation is that this check makes Class#allocate slow. There are ways we could improve that, but I don't think the check as-is is useful. If there's an alternative check we'd like to do instead of simply removing this I'd be happy to implement it.

This makes allocate ~75% faster.

|allocate_no_params            |     19.009M|   20.087M|
|                              |           -|     1.06x|
|allocate_allocate             |     20.587M|   35.882M|
|                              |           -|     1.74x|

@byroot
Copy link
Member

byroot commented Apr 15, 2025

For classes that we need to forbid allocation we should use rb_undef_alloc_func.

In many cases these classes do have an alloc func, e.g.:

    rb_cMatch  = rb_define_class("MatchData", rb_cObject);
    rb_define_alloc_func(rb_cMatch, match_alloc);
    rb_undef_method(CLASS_OF(rb_cMatch), "new");
    rb_undef_method(CLASS_OF(rb_cMatch), "allocate");

But perhaps there is another way we could mark these classes as not publicly allocatable.

jhawthorn and others added 2 commits April 15, 2025 16:24
`.allocate` is banned on some types for good reasons, and allowing
it is dangerous as many code paths likely aren't tested nor
implemented with uninitialized objects in mind.

However `respond_to` is too expensive of a check, so we can
instead mark the class as not allocatable.
Copy link

launchable-app bot commented Apr 15, 2025

All Tests passed!

✖️no tests failed ✔️61863 tests passed(2 flakes)

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current change in the PR means non-core classes can no longer benefit from that check (unless they already use rb_undef_alloc_func but that's not always possible, e.g. for pure-Ruby gems).

Maybe we could have a hook when undefining a method, specifically allocate to set this flag internally so it's just a performance optimization but has the exact same semantics?
Though I'm not sure the current check is ideal either.

@eregon
Copy link
Member

eregon commented Apr 26, 2025

Why does MatchData define an alloc_func, what is used for?

@byroot
Copy link
Member

byroot commented Apr 26, 2025

Why does MatchData define an alloc_func, what is used for?

For MatchData#dup. Which is of questionable use, but since MatchData isn;t inherently frozen, you can store ivars on it etc..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants