Skip to content

Less strict parsing of visibility method arguments #1803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nevans wants to merge 4 commits into
base: master
Choose a base branch
from
+66 −35
Open

Less strict parsing of visibility method arguments #1803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b426663
Less strict parsing of visibility method arguments
nevans Aug 28, 2026
5d3c8cf
Fix Parser::Ruby docs for `*_class_method`
nevans Aug 30, 2026
e30f6d6
Refactor Parser::Ruby#call_node_name_arguments
nevans Aug 30, 2026
2c3972c
Address review comment
nevans Sep 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 29 additions & 22 deletions lib/rdoc/parser/ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class Parser
# * constants
# * aliases
# * private, public, protected
# * private_class_function, public_class_function
# * private_class_method, public_class_method
# * private_constant, public_constant
# * module_function
# * attr, attr_reader, attr_writer, attr_accessor
Expand Down Expand Up @@ -388,15 +388,23 @@ def handle_modifier_directive(code_object, line_no) # :nodoc:
end

def call_node_name_arguments(call_node) # :nodoc:
return [] unless call_node.arguments
call_node.arguments.arguments.map do |arg|
case arg
when Prism::SymbolNode
arg.value
when Prism::StringNode
arg.unescaped
end
end || []
return unless arguments_node = call_node.arguments
names = arguments_node.arguments.filter_map { |arg| argument_name(arg) }
names unless names.empty?

@tompng tompng Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Return value was always an array.
It'll be changed to nil-able, so code around L415 (updated in #1793) needs change.

# Around L415
when 'attr', 'attr_reader', 'attr_writer', 'attr_accessor'
  # attributes ||= call_node_name_arguments(node).compact if is_call_node
  # ↓
  attributes ||= call_node_name_arguments(node) || [] if is_call_node
  ...
end

if attributes
  # handles attr, attr_reader, attr_writer, attr_accessor (so attributes must be non-nil)
elsif line_no || node
  # handles non-attr ghost method
end

@nevans nevans Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

While it is true that it was previously always an array:

  • it was only used inside an if attributes clause
  • the elsif line_no || node clause is triggered if and only if there's also a method or singleton-method directive.

Although it's a weird edge case (which explains why there's no tests for it), allowing method or singleton-method to trigger when attributes failed to collect any names seems like maybe a good thing to me? Anyway, that was my reasoning when I made the change. Is there some way this could cause an issue?

Mostly I changed it for the consistency with the similar Visitor methods (constant_arguments_names, symbol_arguments, visibility_method_arguments, etc). While call_node_name_arguments lives on the scanner, like those visitor methods it processes an AST node and doesn't reference any ivars. It feels to me like it ought to be a module_function in a shared module together with those other methods, so I figured it made sense to be consistent with them.

But I'm happy to change it back to always returning an array: unless there's some important distinction between empty vs non-existent, I generally prefer to return an empty collection rather than nil. It just feels odd for it to behave differently from the other related methods.

@nevans nevans Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, I reasoned incorrectly. While line_no is only set when those other directives are visible, node could be there for any ## comments that are matched to a subsequent node.

@nevans nevans Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tompng Thanks for catching that. I've added some examples to test_invalid_meta_method that capture the bug I'd created.

end

def call_node_name_argument(call_node) # :nodoc:
return unless call_node.arguments
argument_name(call_node.arguments.arguments.first)
end

def argument_name(argument_node) # :nodoc:
case argument_node
when Prism::SymbolNode
argument_node.value
when Prism::StringNode
argument_node.unescaped
end
end

# Handles meta method comments
Expand All @@ -412,7 +420,7 @@ def handle_meta_method_comment(comment, directives, node)
case directive
when 'attr', 'attr_reader', 'attr_writer', 'attr_accessor'
attributes = [param] if param
attributes ||= call_node_name_arguments(node).compact if is_call_node
attributes ||= call_node_name_arguments(node) || [] if is_call_node
rw = directive == 'attr_writer' ? 'W' : directive == 'attr_accessor' ? 'RW' : 'R'
when 'method'
method_name = param if param
Expand All @@ -438,7 +446,7 @@ def handle_meta_method_comment(comment, directives, node)
mark_container_documentable(@container)
end
elsif line_no || node
method_name ||= call_node_name_arguments(node).first if is_call_node
method_name ||= call_node_name_argument(node) if is_call_node
line_no = node.location.start_line if node
internal_add_method(
method_name,
Expand Down Expand Up @@ -1216,8 +1224,7 @@ def constant_arguments_names(call_node)
end

def call_node_name_arguments(call_node)
names = @scanner.call_node_name_arguments(call_node).compact
names unless names.empty?
@scanner.call_node_name_arguments(call_node)
end

def symbol_arguments(call_node)
Expand All @@ -1229,10 +1236,10 @@ def symbol_arguments(call_node)
def visibility_method_arguments(call_node, singleton:)
arguments_node = call_node.arguments
return unless arguments_node
symbols = symbol_arguments(call_node)
if symbols
# module_function :foo, :bar
return symbols.map(&:to_s)
names = call_node_name_arguments(call_node)
if names
# module_function :foo, "bar"
return names
else
return unless arguments_node.arguments.size == 1
arg = arguments_node.arguments.first
Expand Down Expand Up @@ -1322,14 +1329,14 @@ def _visit_call_extend(call_node)

def _visit_call_public_constant(call_node)
return if @scanner.in_proc_block || @scanner.singleton
names = symbol_arguments(call_node)
@scanner.container.set_constant_visibility_for(names.map(&:to_s), :public) if names
names = call_node_name_arguments(call_node)
@scanner.container.set_constant_visibility_for(names, :public) if names
end

def _visit_call_private_constant(call_node)
return if @scanner.in_proc_block || @scanner.singleton
names = symbol_arguments(call_node)
@scanner.container.set_constant_visibility_for(names.map(&:to_s), :private) if names
names = call_node_name_arguments(call_node)
@scanner.container.set_constant_visibility_for(names, :private) if names
end

def _visit_call_attr_reader_writer_accessor(call_node, rw)
Expand Down
50 changes: 37 additions & 13 deletions test/rdoc/parser/ruby_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,24 @@ class Foo

# :attr-accessor: arw3
add_my_attribute_arw3

# Unparsable attributes should not parse as "unknown" methods

##
# :attr:
add_my_attribute_a4("foo" + "bar")

##
# :attr_reader:
add_my_attribute_ar4("foo" + "bar")

##
# :attr_writer:
add_my_attribute_aw4("foo" + "bar")

##
# :attr_accessor:
add_my_attribute_arw4("foo" + "bar")
end
RUBY

Expand Down Expand Up @@ -1372,7 +1390,7 @@ module A
def m1; end
def m2; end
def m3; end
module_function :m1, :m3
module_function :m1, "m3", kwarg: ignored
module_function def m4; end
end
RUBY
Expand All @@ -1391,8 +1409,8 @@ class A
def self.m1; end
def self.m2; end
def self.m3; end
private_class_method :m1, :m2
public_class_method :m1, :m3
private_class_method ignored, :m1, "m2"
public_class_method :m1, ignored, :m3
private_class_method def self.m4; end
public_class_method def self.m5; end
end
Expand All @@ -1410,17 +1428,17 @@ def m2; end
def m3; end
def m4; end
def m5; end
private :m2, :m3, :m4
public :m1, :m3
private :m2, :m3, "m4", kwarg: :ignored
public :m1, ignored, :m3
end
class << A
def m1; end
def m2; end
def m3; end
def m4; end
def m5; end
private :m1, :m2, :m3
public :m2, :m4
private :m1, :m2, "m3"
public ignored, :m2, :m4
end
RUBY
klass = @store.find_class_named 'A'
Expand All @@ -1435,7 +1453,8 @@ def test_undocumentable_change_visibility
class A
def m1; end
def self.m2; end
private 42, :m # maybe not Module#private
def m3; end
private 42, "m3"
# ignore all non-standard `private def` and `private_class_method def`
private def self.m1; end
private_class_method def m2; end
Expand All @@ -1444,7 +1463,10 @@ def self.m2; end
end
RUBY
klass = @store.find_class_named 'A'
assert_equal [:public] * 4, klass.method_list.map(&:visibility)
singleton_methods, instance_methods = klass.method_list.partition(&:singleton)
.map { |methods| methods.to_h { |m| [m.name, m.visibility] } }
assert_equal({'m1' => :public, 'm3' => :private, 'm2' => :public}, instance_methods)
assert_equal({'m2' => :public, 'm1' => :public}, singleton_methods)
end

def test_singleton_class_def_with_visibility
Expand Down Expand Up @@ -1490,10 +1512,11 @@ def test_singleton_method_visibility_change_in_subclass
class A
def self.m1; end
def self.m2; end
private_class_method :m2
ignored = :m1
private_class_method ignored, "m2"
end
class B < A
private_class_method :m1
private_class_method :m1, ignored
public_class_method :m2
end
RUBY
Expand Down Expand Up @@ -1943,8 +1966,8 @@ class C
private_constant
private_constant foo
private_constant :A
private_constant :B, :C
public_constant :B
private_constant :B, bar, :C
public_constant baz, "B"
end
RUBY
klass = @store.find_class_named 'C'
Expand Down Expand Up @@ -2706,6 +2729,7 @@ def foo; end
tap do end
def foo; end
module_function :foo
def foo; end
end
def bar; end
module_function :bar
Expand Down