Bug #19749: Confirm correct behaviour when attaching private method with `#define_method`

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 3.3.0
• Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN

[itarato (Peter Arato)]

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)

[Eregon (Benoit Daloze)]

• Description updated (diff)

[jeremyevans0 (Jeremy Evans)]

• 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.

[Eregon (Benoit Daloze)]

@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?

[Eregon (Benoit Daloze)]

• Description updated (diff)

[Eregon (Benoit Daloze)]

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)

[Eregon (Benoit Daloze)]

• Status changed from Closed to Open

[jeremyevans0 (Jeremy Evans)]

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.

[Eregon (Benoit Daloze)]

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 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.

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.

[Eregon (Benoit Daloze)]

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.

[itarato (Peter Arato)]

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

[jeremyevans0 (Jeremy Evans)]

I submitted a pull request to fix this: https://github.com/ruby/ruby/pull/8009

[mame (Yusuke Endoh)]

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:

1. Ideally, I want define_method to always define a public method. However, it is too late to change.
2. 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.
3. The next next desirable is the current behavior.

[jeremyevans0 (Jeremy Evans)]

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.

[austin (Austin Ziegler)]

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)?

[mame (Yusuke Endoh)]

@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?

[jeremyevans0 (Jeremy Evans)]

@matz (Yukihiro Matsumoto) This was reviewed during the August 2023 developer meeting and is still waiting for your reply.

[mame (Yusuke Endoh)]

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.

• 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]