Skip to content

Remove respond_to check from Class#alloacte#13116

Merged
jhawthorn merged 2 commits intoruby:masterfrom
jhawthorn:class_allocate
May 12, 2025
Merged

Remove respond_to check from Class#alloacte#13116
jhawthorn merged 2 commits intoruby:masterfrom
jhawthorn:class_allocate

Conversation

@jhawthorn
Copy link
Copy Markdown
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
Copy Markdown
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

This comment has been minimized.

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

eregon commented Apr 26, 2025

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

@byroot
Copy link
Copy Markdown
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
Copy Markdown
Member Author

jhawthorn commented May 2, 2025

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
Copy Markdown
Member Author

jhawthorn commented May 8, 2025

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
Copy Markdown
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