From: "alanwu (Alan Wu)" <noreply@...>
Date: 2022-01-05T01:23:17+00:00
Subject: [ruby-core:106968] [Ruby master Bug#18435] Calling `protected` on ancestor method changes result of `instance_methods(false)`

Issue #18435 has been updated by alanwu (Alan Wu).


I agree this is a confusing part of the API surface. This ticket reminds me of
the discussion from [Bug #16106].

Consider the following setup:

```ruby
class Parent
  def foo; end
end

class Child < Parent
  protected :foo
end

p Child.instance_method(:foo).owner # => Parent
```

`Module#instance_methods` (plural) only returns public and protected methods,
but `Module#instance_method` (singular) doesn't filter based on visibility.
Also, as Jeremy pointed out in [ruby-core:106839]
`Child.protected_instance_methods(false)` gives `[:foo]`, but
`Child.instance_method(:foo).protected?` gives `false` surprisingly.

So currently, `Module#protected_instance_methods` and similar APIs can provide
more information than `Module#instance_method`. APIs with plural names can
observe the effects of using `Child.protected(:foo)`.

An important question is whether `Module#protected` and other visibility change
APIs semantically define new methods when used in a subclass. If not, the
particular wording for relevant APIs cover the current behavior:

> Module#protected: ... With arguments, sets the named methods to have
> protected visibility ...
>
> UnboundedMethod#owner: Returns the class or module that *defines* the method.
> ...

The wording for `Module#instance_method` is unclear as to what should happen
when there is a visibility difference:

> Module#instance_method: Returns an +UnboundMethod+ representing the given
> instance method in _mod_.

I think Jeremy's [PR] makes it a rule that using visibility change methods in
subclasses semantically define new methods, but it opens up some design issues
I posted as a [comment] on GitHub. I don't think it should be merged as a
simple bug fix since it's a breaking change to APIs that are fairly fundamental
to the language. I do agree that the addition of `public?` and friends have
made how `Module#instance_method` behaves with respect to visibility change
APIs more surprising.

I think it's worth mentioning in docs that `Module#instance_method{s,}` are
very different despite having names that imply a relationship.

[PR]: https://github.com/ruby/ruby/pull/5356
[comment]: https://github.com/ruby/ruby/pull/5356#issuecomment-1005298809


----------------------------------------
Bug #18435: Calling `protected` on ancestor method changes result of `instance_methods(false)`
https://bugs.ruby-lang.org/issues/18435#change-95796

* Author: ufuk (Ufuk Kayserilioglu)
* Status: Open
* Priority: Normal
* ruby -v: ruby 2.7.5p203 (2021-11-24 revision f69aeb8314) [x86_64-darwin20]
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
As documented `instance_methods(false)` works as follows:

```ruby
module A
  def method1()  end
end

class B
  include A

  def method2()  end
end

p B.instance_methods(false) #=> [:method2]
```

However, calling `protected` on the method defined by `A`, unexpectedly changes the result of `instance_methods(false)` on `B`, even though the owner of the method is still `A`:
```ruby
module A
  def method1()  end
end

class B
  include A

  protected :method1

  def method2()  end
end

p B.instance_methods(false) #=> [:method1, :method2]
p B.instance_method(:method1).owner #=> A
```

In contrast, calling `private` or `public` on the same method does not cause any changes on the result of `B.instance_methods(false)`.

This feels like a bug in the implementation of `instance_methods(false)`, but, if it is by design, it should at least be documented on `Module#instance_methods`.

This reproduction script gives the same output all the way from Ruby 2.0 up to Ruby-HEAD:
https://wandbox.org/permlink/LqbXMBTYxURRZmDz



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>