-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
base: master
Are you sure you want to change the base?
Conversation
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. |
`.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.
✅ All Tests passed!✖️no tests failed ✔️61863 tests passed(2 flakes) |
There was a problem hiding this 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.
Why does MatchData define an alloc_func, what is used for? |
For |
https://bugs.ruby-lang.org/issues/21267
Class#allocate
has an additionalrb_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
.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:
Or even override
respond_to_missing?
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:
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.