Skip to content

Remove respond_to check from Class#alloacte #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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 12, 2025
Merged

Remove respond_to check from Class#alloacte #13116

merged 2 commits into from
May 12, 2025

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Apr 15, 2025 •
edited
Loading

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.

@launchable-app Launchable app

This comment has been minimized.

Sign in to view
eregon
eregon reviewed Apr 26, 2025
View reviewed changes
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..

@jhawthorn jhawthorn force-pushed the class_allocate branch 2 times, most recently from 1f1a3db to a6365ce Compare May 2, 2025 10:43
@jhawthorn jhawthorn changed the title Remove respond_to check from Class#bind_call Remove respond_to check from Class#alloacte May 2, 2025
@jhawthorn
Copy link
Member Author

so it's just a performance optimization but has the exact same semantics

My point in opening https://bugs.ruby-lang.org/issues/21267 is that the exact existing semantics are useless, as it only protects against someone trying really hard to make an uninitialized object (by calling bind_call) but doesn't effectively prevent someone trying a little harder (defining a method named initialize, or overriding respond_to).

For pure-Ruby gems. I don't think this "feature" is necessary, because uninitialized objects won't crash the VM (which I believe is the motivation for the extra checking).

@jhawthorn
Copy link
Member Author

Per https://bugs.ruby-lang.org/issues/21267?next_issue_id=21302&prev_issue_id=21298#note-1 I've reverted this to my original proposal, simply removing the check.

@eregon
Copy link
Member

eregon commented May 8, 2025

I'm OK with the removal of that check (in case it wasn't clear from my replies on https://bugs.ruby-lang.org/issues/21267).

@jhawthorn jhawthorn merged commit b0502e8 into ruby:master May 12, 2025
81 checks passed
@jhawthorn jhawthorn deleted the class_allocate branch May 12, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jhawthorn @byroot @eregon