Less strict parsing of visibility method arguments (#1803)

NOTE: this PR stacks on #1793. Only [the last three
commits](https://github.com/ruby/rdoc/pull/1803/changes/f4607d26d8db4ca8d96b819028bf083e8ad5f5c4..242a1e9a28d2c5abdc261e7141bd26ba2999e202)
are specific to this PR.

This updates method name argument parsing for the visibility methods:
* `private`, `public`, `protected`
* `private_class_method`, `public_class_method`
* `private_constant`, `public_constant`
* `module_function`

Prior to this PR, the parser is stricter about visibility method name
arguments than it should be: it only parses method names when _all_
arguments are symbols.

This updates the prism parser to behave more like the rdoc 7.2, so every
(non-interpolated) string and symbol in the argument list is used
(`call_node_name_arguments` is used to parse the arguments list).

Additionally:
* Fix `Parser::Ruby` docs for `private_class_method` and
`public_class_method`.
There aren't any `private_class_function` and `public_class_function`
methods. 😉
* Refactor `Parser::Ruby#call_node_name_arguments` into plural and
singular versions.
Since meta-programmed method names are only ever inferred from the first
argument, the singular version only reads the first argument. The plural
version is now free to use `filter_map` and convert `[]` to `nil`,
enabling the short-circuiting pattern used in the visitor.
Arguably, these methods belong more to the visitor than the "scanner".
Since they simply process prism nodes without any ivar references, they
should probably be converted into module functions on a utility module.
But I'm considering that out of scope for this PR.

---------

Co-authored-by: tomoya ishida <tomoyapenguin@gmail.com>

Author: nevans

lib/rdoc/parser/ruby.rb

@@ -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
@@ -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?
+      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
@@ -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
@@ -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,
@@ -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)
@@ -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
@@ -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)

test/rdoc/parser/ruby_test.rb

@@ -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
 
@@ -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
@@ -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
@@ -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'
@@ -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
@@ -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
@@ -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
@@ -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'
@@ -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