Explicitly use public visibility for define_singleton_method#7055
Merged
headius merged 2 commits intojruby:jruby-9.3from Mar 16, 2022
Merged
Explicitly use public visibility for define_singleton_method#7055headius merged 2 commits intojruby:jruby-9.3from
headius merged 2 commits intojruby:jruby-9.3from
Conversation
This bypasses the frame-checking logic in define_method since we know the target class (target object's singleton class) will never match the caller environment.
In CRuby this logic happens to always be public because the target class (the target object's singleton class) will almost never match the caller's scope. Pending CRuby confirming that this is the intended behavior and making it explicit so the caller's frame is never used for define_singleton_method visibility.
Member
Author
|
This change appears to be getting some traction with the CRuby folks (ruby/ruby#5636) so we will optimistically proceed with this change in 9.3.4. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes public visibility the only visiblity for
define_singleton_methodmethods, and adds a spec testing a simple case of this.This is a bit of an open question, since in CRuby
define_singleton_methodends up producing public methods nearly all of the time due to a quirk in how the frame's visibility is accessed.There's at least one edge case where
define_singleton_methoddoes obey the frame visibility, but it requires callingdefine_singleton_methodfrom within a scope rooted on the destination singleton class. I would argue this is a peculiar-enough edge case that it should be made to explicitly use public visibility in all cases.See ruby/ostruct#40 for a related issue, where we warn about aliasing
define_singleton_methoddue to its rarely-used ability to source visibility from the caller's scope.See https://bugs.ruby-lang.org/issues/18561 for my CRuby issue to codify this behavior and eliminate the edge cases.