Remove Context#visibility and Context#current_line_visibility - #1800
Conversation
GitHub Actions
Waiting
There was a problem hiding this comment.
Pull request overview
Moves visibility ownership from contexts to parsers, addressing ri-store visibility corruption from #346.
Changes:
- Removes visibility state from
RDoc::Context. - Sets visibility before registering methods and attributes.
- Adds legacy-cache fallbacks and a regression test.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/rdoc/code_object/any_method.rb |
Defaults legacy nil visibility to public. |
lib/rdoc/code_object/attr.rb |
Defaults legacy nil visibility to public. |
lib/rdoc/code_object/class_module.rb |
Removes context visibility handling during conversion, loading, and embedding. |
lib/rdoc/code_object/context.rb |
Removes context-owned visibility state. |
lib/rdoc/parser/c.rb |
Assigns method visibility before registration. |
lib/rdoc/parser/rbs.rb |
Assigns declaration visibility before registration. |
lib/rdoc/parser/ruby.rb |
Assigns parser visibility before registration. |
test/rdoc/rdoc_context_test.rb |
Updates expectations for object-owned visibility. |
test/rdoc/rdoc_stats_test.rb |
Removes an invalid constant setup. |
test/rdoc/rdoc_store_test.rb |
Covers visibility preservation across repeated saves. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -442,8 +440,6 @@ def marshal_load(array) # :nodoc: | |||
|
|
|||
| array[8].each do |type, visibilities| | |||
| visibilities.each do |visibility, methods| | |||
There was a problem hiding this comment.
Add method.visibility = visibility and a test for it
st0012
left a comment
st0012
left a comment
There was a problem hiding this comment.
Cleanup looks good, just have some questions
| assert_equal :public, @meth.visibility | ||
|
|
||
| @s.save_class @klass # save again to trigger ClassModule#merge | ||
| assert_equal :public, @meth.visibility |
There was a problem hiding this comment.
Hmm I'm not sure if I understand what this test checks against 🤔
There was a problem hiding this comment.
:private case and description added
| def test_add_method | ||
| meth = RDoc::AnyMethod.new 'old_name' | ||
| meth.visibility = nil | ||
| meth.visibility = :public |
There was a problem hiding this comment.
Why are these necessary? Should method objects instantiate with default public visibility?
There was a problem hiding this comment.
This is not needed.
I added this line when I mistakenly thought the default is nil.
removed
Context doesn't need visibility. It's a parser's lexical state. Visibility of added method won't be modified in `add_method`, so we can remove all workarounds related to method/attribute/alias and its visibility: Set/modify visibility after adding method/attribute/alias. AnyMethod#marshal_load and Attr#marshal_load fall back to :public when a cache written by an older RDoc has nil visibility, the same as Constant#marshal_load. rdoc_stats_test passed a NormalClass to add_constant, which only worked because Context responded to `visibility`.
d2e3968 to
61fa6c8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
lib/rdoc/code_object/class_module.rb:446
- The compact class-descriptor loader bypasses
AnyMethod#marshal_load, so an oldercdescentry whose visibility isnilstill leaves this method withnilvisibility. That defeats the new compatibility fallback and allows the invalid value to be written again on the next save; default it here as the attribute path already does, and cover a nil visibility group in this test path.
method.visibility = visibility
| @@ -638,7 +638,6 @@ def add_alias_method(old_name, new_name, line_no) | |||
| if should_document?(a) | |||
| mark_container_documentable(@container) | |||
| @container.add_alias(a) | |||
| @name ||= "unknown" | ||
| @parent = nil |
|
🚀 Preview deployment available at: https://46ab52e8.rdoc-6cd.pages.dev (commit: 61fa6c8) |
Fixes #346
Context doesn't need visibility. It's a parser's lexical state.
Visibility of added method won't be modified in
add_method, so we can remove all workarounds related to method/attribute/alias and its visibility: Set/modify visibility after adding method/attribute/alias.AnyMethod#marshal_load and Attr#marshal_load fall back to :public when a cache written by an older RDoc has nil visibility, the same as Constant#marshal_load.
rdoc_stats_test passed a NormalClass to add_constant, which only worked because Context responded to
visibility.Added test: from #1736