Skip to content

Preserve ancestor visibility when creating module functions - #1828

Open
st0012 wants to merge 1 commit into
ruby:masterfrom
st0012:codex/module-function-visibility
Open

st0012 wants to merge 1 commit into
ruby:masterfrom
st0012:codex/module-function-visibility

Conversation

@st0012

@st0012 st0012 commented Sep 20, 2026

Copy link
Copy Markdown
Member

Found when reviewing #1825: Calling module_function :foo can incorrectly change the visibility of all foo methods in the module's inheritance chain. For module_function's handling, the visibility change shouldn't go through ancestors.

@st0012 st0012 added the bug label Sep 20, 2026
@github-actions

Copy link
Copy Markdown

Documentation preview

View the preview

Commit: db04c99

@st0012
st0012 marked this pull request as ready for review September 20, 2026 21:29
Copilot AI balanced review requested due to automatic review settings September 20, 2026 21:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new inherited-method path can emit a spurious duplicate-method warning.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Preserves ancestor method visibility when creating module functions.

Changes:

  • Uses scoped visibility handling for module functions.
  • Adds inherited and overridden method regression tests.
File Description
test/​rdoc/​parser/​ruby_test.rb Tests ancestor visibility preservation.
lib/​rdoc/​parser/​ruby.rb Changes module-function visibility handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/rdoc/parser/ruby.rb

def change_method_to_module_function(names)
@container.set_visibility_for(names, :private, false)
change_method_visibility(names, :private, singleton: false)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants