Issue #21267 has been updated by Eregon (Benoit Daloze). I think it makes sense to replace or remove that "incorrect" check given it doesn't apply to `new`. The proper fix seems to also do the correct check in `new`. I think it's best to keep the possibility for 3rd party C extensions to undefine `allocate` (to prevent creating uninitialized objects) and still allow dup/clone (which should work fine if people don't monkey-patch `initialize_{copy,clone,dup}`, if they do the segfault/error is their responsibility then). With this change there is no protection in place left, so `Class.instance_method(:allocate).bind_call(MyType)` could segfault, even if MyType undefines `allocate`. Looking at the PR, how about setting `alloc_prohibited = true` when undefining `allocate`? (and set it back to false if it's defined again later) Another way would be to have separate allocation paths for "new object" and "copy" but this requires much more thought. Or prevent this case of "no new/allocate but dup/clone", but then for core types too for consistency (maybe too incompatible? maybe not?). Or core types could actually define dup & clone as overridden methods, and not use the alloc function at all and so use `rb_undef_alloc_func()`. That seems the cleanest actually, WDYT?
it doesn't provide any safety
Well, using bind_call is not rare and might even do it accidentally, OTOH defining a singleton `initialize_copy`/`allocate` method is pretty hardcore, and that should be no surprise it blows things up. ---------------------------------------- Bug #21267: respond_to check in Class#allocate is inconsistent https://bugs.ruby-lang.org/issues/21267#change-112888 * Author: jhawthorn (John Hawthorn) * Status: Open * Backport: 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN ---------------------------------------- `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, more robust, 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| ``` https://github.com/ruby/ruby/pull/13116 -- https://bugs.ruby-lang.org/