Bug #19749
closedConfirm correct behaviour when attaching private method with `#define_method`
Description
This issue is a special case of https://bugs.ruby-lang.org/issues/19745:
Should dynamically added private methods via .singleton_class.send(:define_method,...
at the top-level be accessible publicly?
See the following example:
def bar; end
foo = Object.new
foo.singleton_class.define_method(:bar, method(:bar))
foo.bar # No error.
The script above runs fine on latest Ruby 3.3. Is this correct to ignore the fact that the visibility in the caller context is the default top-level private visibility?
This came up during a TruffleRuby investigation (https://github.com/oracle/truffleruby/issues/3134) where the result for the same script is: private method 'bar' called for #<Object:0xc8> (NoMethodError)
Updated by jeremyevans0 (Jeremy Evans) over 1 year ago
- Status changed from Open to Closed
As in #19745, it is expected that define_method
will not use the method body (second argument/block) to determine method visibility. define_method
will only consider current default scope visibility to determine visibility:
def bar; end
foo = Object.new
foo.singleton_class.class_exec do
private
foo.singleton_class.send(:define_method, :bar, method(:bar))
end
foo.bar # NoMethodError
The important principle here for both define_method
and define_singleton_method
is that the method body (second argument/body), only provides the implementation of the method, and nothing else.
Updated by Eregon (Benoit Daloze) over 1 year ago
@jeremyevans0 (Jeremy Evans) That does not explain why the original example defines the method as public (note I edited the description on Redmine, it was not asking the right question).
will only consider current default scope visibility to determine visibility:
And that visibility is private at the top-level. So why is the method defined as public on CRuby?
Updated by Eregon (Benoit Daloze) over 1 year ago
To clarify, this proves the top-level visibility is private:
def foo; end
o = self
o.foo # => private method `foo' called for main:Object (NoMethodError)
Updated by Eregon (Benoit Daloze) over 1 year ago
- Status changed from Closed to Open
Updated by jeremyevans0 (Jeremy Evans) over 1 year ago
Eregon (Benoit Daloze) wrote in #note-3:
@jeremyevans0 (Jeremy Evans) That does not explain why the original example defines the method as public (note I edited the description on Redmine, it was not asking the right question).
will only consider current default scope visibility to determine visibility:
And that visibility is private at the top-level. So why is the method defined as public on CRuby?
Scope visibility is only used if the receiver of the method is the same as current scope, which was not the case in the previous examples given.
define_method
at top level does not define private methods. I think this is because the receiver of the define_method
is main
, but the method is defined in Object
, so the scope visibility does not apply. I don't think this is a bug, see #9005.
However, I think there is a bug here, in that define_method
will not change the existing visibility of a method if define_method
is called with the current method body:
def bar; end
define_method(:bar, method(:bar))
Object.new.bar # NoMethodError
This is not related to top-level:
class Foo
def bar; end
define_method(:bar, method(:bar))
new.bar # NoMethodError
end
Note that define_method
does not copy the method body visibility if there is not an existing method entry:
def bar; end
define_method(:baz, method(:bar))
Object.new.baz # no error
It will override the method visibility if the method is already defined but does not have the same implementation, but in that case it will not use the method body visibility:
def bar; end
def baz; end
define_method(:baz, method(:bar))
Object.new.baz # no error
This also affects define_singleton_method
:
foo = Object.new
foo.singleton_class.class_exec do
private def bar; end
end
foo.define_singleton_method(:bar, foo.method(:bar))
foo.bar # NoMethodError
I think define_method
/define_singleton_method
should be changed to reset the method visibility if the current method body is passed, either to the scope visibility if the receiver is the same as the current scope, or to public otherwise.
Updated by Eregon (Benoit Daloze) over 1 year ago
jeremyevans0 (Jeremy Evans) wrote in #note-7:
define_method
at top level does not define private methods. I think this is because the receiver of thedefine_method
ismain
, but the method is defined inObject
, so the scope visibility does not apply. I don't think this is a bug, see #9005.
OK, so it compares the caller self (or the default definee?) and the receiver of define_method
then, and only considers scope visibility if they are the same.
I'll try to implement that.
I suppose that's what rb_vm_cref_in_context()
does but that's pretty cryptic.
I think
define_method
/define_singleton_method
should be changed to reset the method visibility if the current method body is passed, either to the scope visibility if the receiver is the same as the current scope, or to public otherwise.
Agreed, whether there is already a method with that name or not shouldn't change anything for define_method
/define_singleton_method
.
Updated by Eregon (Benoit Daloze) over 1 year ago
I find it very surprising/unintuitive that define_method
sometimes ignores the scope visibility, with hard-to-explain and undocumented conditions, but def
always respects the scope visibility (AFAIK, not counting "defs"=def r.m
).
A metaprogramming API should as much as possible be equivalent to the non-metaprogramming API.
Updated by itarato (Peter Arato) over 1 year ago
For visibility leaving the examples here that we suspect a CRuby bug:
class Foo5
private
def bar; end
public
define_method(:bar, Foo5.instance_method(:bar))
end
Foo5.new.bar # NoMethodError in CRuby, but should be public
and
class Foo6
public
def bar; end
private
define_method(:bar, Foo6.instance_method(:bar))
end
Foo6.new.bar # No error in CRuby but should be NoMethodError
Updated by jeremyevans0 (Jeremy Evans) over 1 year ago
I submitted a pull request to fix this: https://github.com/ruby/ruby/pull/8009
Updated by mame (Yusuke Endoh) over 1 year ago
Discussed at the dev meeting. In conclusion, @matz (Yukihiro Matsumoto) said he would like to keep the status quo.
Method visibility is not an attribute of a method itself, but an attribute of the class to which the method belongs. Therefore, X.define_method(:bar, method(:foo))
should not inherit the visibility of foo.
Currently, Module#define_method
defines a private/protected method if the context is private/protected and if the class/module of the context is equal to the receiver of define_method
. Otherwise, it defines a public method. This behavior is hard to document, though.
@matz (Yukihiro Matsumoto) discussed the behavior of define_method
as follows:
- Ideally, I want
define_method
to always define a public method. However, it is too late to change. - The next desirable behavior is to define a public method if the receiver of define_method is explicit (i.e., NODE_CALL), and to follow context visibility if it is not explicit (i.e., NODE_FCALL). However, Matz gave up this idea because @ko1 (Koichi Sasada) stated that it was difficult to implement efficiently.
- The next next desirable is the current behavior.
Updated by jeremyevans0 (Jeremy Evans) over 1 year ago
I don't think my discussion from comment #7 was considered during the dev meeting. Discussion focused on general define_method
behavior, which I don't think is a bug. I should have been more clear in the dev meeting ticket exactly what I would like to fix.
Here's a reduced example showing the problem:
class A
def a; end
private def b; end
m = instance_method(:b)
define_method(:c, m) # defines public method
define_method(:b, m) # defines private method, should define public method
private
m = instance_method(:a)
define_method(:d, m) # defines private method
define_method(:a, m) # defines public method, should define private method
p public_instance_methods(false) # => [:c, :a] # should be [:c, :b]
p private_instance_methods(false) # => [:b, :d] # should be [:a, :d]
end
I think this particular behavior of not changing visibility of a method when calling define_method
with the same method body is a bug that should be fixed.
Updated by austin (Austin Ziegler) over 1 year ago
Would it be worth adding a third parameter to Module#define_method
for visibility? X.define_method(:bar, method(:foo), :private)
or X.define_method(:bar, method(:foo), :inherit)
?
Updated by mame (Yusuke Endoh) over 1 year ago ยท Edited
@jeremyevans0 (Jeremy Evans) Thanks, so the question is whether visibility should be updated when redefining a method by define_method
with the method itself.
class C
def foo; end
private
define_method(:foo, instance_method(:foo))
# current: the method foo is kept public
# expected: the method foo is changed private
end
@matz (Yukihiro Matsumoto) What do you think?
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
@matz (Yukihiro Matsumoto) This was reviewed during the August 2023 developer meeting and is still waiting for your reply.
Updated by mame (Yusuke Endoh) 5 months ago
Discussed at the dev meeting. @matz (Yukihiro Matsumoto) said define_method(:foo, instance_method(:foo))
should change the visibility of foo
to that of the current context.
Updated by jeremyevans (Jeremy Evans) 5 months ago
- Status changed from Open to Closed
Applied in changeset git|ad29527920b2a37ee427dc4991ff74d7a2f53e05.
Fix Module#define_method to change visibility when passed existing method body
Fixes [Bug #19749]