Project

General

Profile

Actions

Bug #18435

open

Calling `protected` on ancestor method changes result of `instance_methods(false)`

Added by ufuk (Ufuk Kayserilioglu) 6 months ago. Updated 18 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.7.5p203 (2021-11-24 revision f69aeb8314) [x86_64-darwin20]
[ruby-core:106828]

Description

As documented instance_methods(false) works as follows:

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:

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


Related issues 2 (1 open1 closed)

Related to Ruby master - Bug #18729: Method#owner and UnboundMethod#owner are incorrect after using Module#public/protected/privateRejectedActions
Related to Ruby master - Bug #18751: Regression on master for Method#== when comparing public with private methodOpenActions

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

I don't think this is a bug, I think it is expected behavior. When you call protected in the class, it creates a method entry in the class with a different visibility, even if the owner of the method is still the module. Hopefully a committer with more experience can confirm that.

I don't think this should be documented in instance_methods. After all, it is not unique to instance_methods, but all related methods (methods, protected_instance_methods, etc.). If we document this behavior at all, it would be better to document the creation of method entries in doc/syntax/modules_and_classes.rdoc, in the section on Visibility. However, I don't think that this behavior is worth documenting.

Updated by ufuk (Ufuk Kayserilioglu) 6 months ago

I understand why the difference in behaviour is happening, but I respectfully disagree that this is not a bug.

Regardless of how protected is implemented internally, the return value of instance_methods(false) should not include methods that explicitly say that their owner is a different constant in the ancestor chain. The fact that those methods are being returned is a leak of internal implementation details. Users of the method should not need to know how and why protected would have such a side-effect. Moreover, as I stated in my original report private does not have a similar problem, either.

Basically the documentation of instance_methods explicitly states:

If the optional parameter is false, the methods of any ancestors are not included.

and, in this case, that statement is not correct.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

ufuk (Ufuk Kayserilioglu) wrote in #note-2:

I understand why the difference in behaviour is happening, but I respectfully disagree that this is not a bug.

That's fair. It's not 100% clear that this isn't a bug. Which I why I would like to get input from other committers.

Moreover, as I stated in my original report private does not have a similar problem, either.

This is incorrect, private has exactly the same issue, it's just that instance_methods doesn't include private methods:

module A
  def method1()  end
end

class B
  include A

  private :method1

  private def method2()  end
end

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

public has the same issue:

module A
  private def method1()  end
end

class B
  include A

  public :method1

  def method2()  end
end

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

Basically the documentation of instance_methods explicitly states:

If the optional parameter is false, the methods of any ancestors are not included.

and, in this case, that statement is not correct.

This depends on your definition of "methods of any ancestors". As I mentioned, public/private/protected create method entries in the current class if the method whose visibility they are affecting is defined in the parent class. I think that makes them methods of the current class, even if the definition occurs in the ancestor.

After doing some more testing, I think there is a bug here, but it's related to instance_method/method returning the wrong information. Evidence of this behavior can be found via the newly introduced methods for checking method visibility:

module A
  def method1()  end
end

class B
  include A
  protected :method1
end

p A.instance_method(:method1).public? #=> true
p B.instance_method(:method1).public? #=> true (should be false)
p A.instance_method(:method1).protected? #=> false
p B.instance_method(:method1).protected? #=> false (should be true)
p B.new.method(:method1).public? #=> true (should be false)
p B.new.method(:method1).protected? #=> false (should be true)

I'll see if I can work on a patch to fix this.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

I've submitted a pull request to fix this issue: https://github.com/ruby/ruby/pull/5356 . It makes method/instance_method (and similar methods) no longer skip ZSUPER method entries, so owner will correctly show the class where the method entry is defined, and the visibility methods will return the correct results.

Updated by alanwu (Alan Wu) 6 months ago

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:

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.

Updated by Eregon (Benoit Daloze) 6 months ago

I'm not fully clear on the expected semantics of private/protected/public in a subclass, that would probably be good to improve in the docs of these methods or in general docs.

However since the current semantics seems to clearly create new method entries on the subclass (when the visibility isn't already what's requested) then I think it would make a lot of sense to update the owner of the new method entry to be the subclass, since it's effectively defined on that class (the source_location would remain untouched, of course).

Updated by jeremyevans0 (Jeremy Evans) 5 months ago

I've pushed a simpler fix that doesn't change the semantics of method, but still returns the correct visibility for ZSUPER methods, by storing the correct visibility as a member of struct METHOD: https://github.com/ruby/ruby/pull/5356/commits/8fd730db1d414f61f42cfb75b8e8711347b1d032

It may be worth discussing whether to change the semantics of ZSUPER Method objects (such as who the owner should be), but for right now, it's probably best to just fix the bug.

Actions #8

Updated by jeremyevans (Jeremy Evans) 5 months ago

  • Status changed from Open to Closed

Applied in changeset git|58dc8bf8f15df9a33d191074e8a5d4946a3d59d5.


Fix {Method,UnboundMethod}#{public?,private?,protected?} for ZSUPER methods

Add a visibility member to struct METHOD storing the original
method visibility, and use that, instead of taking the visibility
from the stored method entry (which may have different visibility
for ZSUPER methods).

Consider Method/UnboundMethod objects different if they have
different visibilities.

Fixes [Bug #18435]

Actions #9

Updated by matz (Yukihiro Matsumoto) 24 days ago

  • Related to Bug #18729: Method#owner and UnboundMethod#owner are incorrect after using Module#public/protected/private added
Actions #10

Updated by matz (Yukihiro Matsumoto) 24 days ago

  • Related to Bug #18751: Regression on master for Method#== when comparing public with private method added

Updated by matz (Yukihiro Matsumoto) 24 days ago

  • Status changed from Closed to Open

I thought it was OK to accept this behavior, but it caused issues like #18729 and #18751. At the time of the decision, I haven't noticed those corner cases. Although it has already been shipped with 3.1, I proposed to revert this change. I estimate the impact of reverting incompatibility is minimal.

Instead of changing old behavior, the documentation of instance_methods(false) should be updated to explain why method1 is included in the above example (visibility changes or making aliases are considered as definition by instance_methods).

Matz.

Updated by Eregon (Benoit Daloze) 18 days ago

I believe the only way that makes sense here is to remove "ZSUPER methods" altogether.
Other Ruby implementations do not have this needless complexity and near-impossible-to-understand semantics.
https://bugs.ruby-lang.org/issues/18751#note-11 might help to be closer, but IMHO we should remove "ZSUPER methods" altogether.

public/private/protected should shallow-copy a method entry (but still change Method#owner of course), just like alias_method already behaves.
This is what TruffleRuby and JRuby do, it's simpler, it is what Ruby users expect (Method is a Ruby object that captures a method entry, at the time it was requested), and it is consistent (.owner is always the module which has that method entry in its method table: #18729).

For instance this should be :p1\n:orig1 but currently it's :p2\n:orig1 on CRuby.
I claim no Ruby user expects that, because Method should capture a specific method entry, that's why we have bind/call and that's how Method objects are used.

class P
  private
  def m
    :p1
  end
  
  public
  def orig
    :orig1
  end
end

class C < P
  public :m
  alias_method :alias, :orig
end

class P
  private
  def m
    :p2
  end
  
  public
  def orig
    :orig2
  end
end

p C.new.m
p C.new.alias

@matz (Yukihiro Matsumoto) OK to remove "ZSUPER methods" and make public/protected/private much simpler by having them shallow copy method entries, just like alias_method already does it?
This will solve a lot of confusion and inconsistency for Method objects of methods defined by public/protected/private.

Actions

Also available in: Atom PDF