Bug #18806
closedprotected methods defined by refinements can't be called
Description
Hello!
The protected
visibility is a bit unusual, since it depends on where the callee method is considered to be defined. I was looking into making an optimization to calling protected
methods on self, and we came across some surprising behaviour. (Thanks @alanwu (Alan Wu) for finding!)
As far as I can tell, there is no way to call a method refined method is protected
visibility (other than send
).
class A
end
module MyRefine
refine(A) {
private def private_foo = :refined
def private_foo_in_refinement = private_foo
protected def protected_foo = :refined
def protected_foo_in_refinement = protected_foo
}
end
class A
using MyRefine
def call_private = private_foo
def call_private_through_refinement = private_foo_in_refinement
def call_protected = protected_foo
def call_protected_through_refinement = protected_foo_in_refinement
def is_defined = defined?(protected_foo)
end
A.new.call_private
# => :refined
A.new.call_private_through_refinement
# => :refined
A.new.call_protected
# => NoMethodError: protected method `protected_foo' called for #<A:0x00007f23f35e9390>
A.new.call_protected_through_refinement
# => NoMethodError: protected method `protected_foo' called for #<A:0x00007f23f35e9390>
A.new.is_defined
# "method"
I find it confusing that here protected is more restrictive than private (private methods from a refinement can be called as normal), but I'm not sure if it's a rule that it should be or just how I've always assumed it is.
It's also odd that defined?
returns truthy, but I think this might actually be a bug on defined with protected methods (behaviour doesn't match that of method calls).
I think this is probably not intentional or desired behaviour. It's not useful for us to make methods which can't be called. The reason this happens I think is an implementation detail: the "defined class" of the method we're trying to call is the ICLASS including the refinement module onto the A
superclass.
Possible options I see for improving this:
-
Treat defined methods as though they were defined on the refined class. Both examples above will now work, and it is possible to call the refined method on objects other than self.
-
Always allow "fcalls" to
protected
methods. Making them basically an alias forprivate
when used in a refinement. -
Forbid using
protected
inside a refinement (andRefinement#import_methods
), raising an error.
My preference would be 1. I think it would require one extra check when calling protected methods, but calling protected methods is already a slower path so I think it is fine.
Thanks for reading!
Updated by Eregon (Benoit Daloze) over 2 years ago
I agree this looks like unintended behavior/a bug.
TruffleRuby already behaves like:
:refined
:refined
:refined
:refined
"method"
fcalls should ignore visibility, always, so I think 2. is best.
Updated by shugo (Shugo Maeda) over 2 years ago
protected is for a method call with an explicit receiver, so I prefer 1.
private is enough for fcalls.
Updated by Eregon (Benoit Daloze) over 2 years ago
Right, agreed that 1 is best semantically.
I'm not too keen on extra checks, but the checks for protected are already expensive and this won't make it significantly worse.
1 implies 2 BTW in terms of optimizations, i.e., if it's an fcall no need to check any visibility.
Updated by shugo (Shugo Maeda) over 2 years ago
- Assignee set to matz (Yukihiro Matsumoto)
Eregon (Benoit Daloze) wrote in #note-3:
I'm not too keen on extra checks, but the checks for protected are already expensive and this won't make it significantly worse.
Agreed.
Matz, can I change protected methods defined by refinements as follows?
- Treat defined methods as though they were defined on the refined class. Both examples above will now work, and it is possible to call the refined method on objects other than self.
Updated by jhawthorn (John Hawthorn) over 2 years ago
Patch implementing option 1: https://github.com/ruby/ruby/pull/5966
Updated by matz (Yukihiro Matsumoto) over 2 years ago
- Assignee changed from matz (Yukihiro Matsumoto) to shugo (Shugo Maeda)
The option 1 is accepted.
Matz.
Updated by jhawthorn (John Hawthorn) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|ae163cae6b3f01e0fb827e0a18d5889f9703617f.
Allow calling protected methods from refinements
Previously protected methods on refinements could never be called
because they were seen as being "defined" on the hidden refinement
ICLASS.
This commit updates calling refined protected methods so that they are
considered to be defined on the original class (the one being refined).
This ended up using the same behaviour that was used to check whether a
call to super was allowed, so I extracted that into a method.
[Bug #18806]