Less strict parsing of visibility method arguments

[nevans]

NOTE: this PR stacks on #1793. Only the last three commits 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.

[nevans]

NOTE: This PR stacks on #1793. But if I make that the PR's branch the base branch, GitHub will convert this PR into a PR on my repo. Only the last three commits are specific to this PR.

[matzbot]

🚀 Preview deployment available at: https://1a6e72ae.rdoc-6cd.pages.dev (commit: 242a1e9)

[st0012]

LGTM, just some nits
Thanks for fixing it 👍

[github-actions]

Documentation preview

View the preview

Commit: 2c3972c

[nevans]

@st0012 I only just got back around to addressing your nits. And I rebased it after #1795, which made every line a conflict. But the changes here were small and simple to replay.

[tompng]

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]

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]

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]

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

[tompng]

👍