Allow less strict attribute argument parsing

[nevans]

This updates argument parsing for the attr, attr_reader, attr_writer, and attr_accessor methods, so they behave more like rdoc 7.2's parser.

The prism parser is (prior to this PR) strict about attribute arguments: it only parses as an attribute when all arguments are symbols. rdoc 7.2's parser allowed all symbol or string arguments and ignored the rest.

As an example, the rdoc for Net::IMAP::Config intentionally took advantage of the looser parsing done by rdoc 7.2. That class redefines attr_reader, attr_writer and attr_accessor to add keyword arguments for type validation/coercion and defaults:

      # Seconds to wait until a connection is opened.
      #
      # Applied separately for establishing TCP connection and starting a TLS
      # connection.
      #
      # If the IMAP object cannot open a connection within this time,
      # it raises a Net::OpenTimeout exception.
      #
      # See Net::IMAP.new and Net::IMAP#starttls.
      #
      # The default value is +30+ seconds.
      attr_accessor :open_timeout, type: Integer, default: 30

rdoc 7.2 simply ignored the unknown keyword args, and parses this no differently from attr_accessor :open_timeout.

Fixes #1790.

[st0012]

I feel symbol_arguments should already work like initial_symbol_arguments.

@tompng why was it designed to return nil when there's any non-symbol arguments? What's the case we're guarding against?

[tompng]

To handle alias_method.
We need to prevent alias_method :foo, bar, :baz, blah: true treated as alias_method :foo, :baz.
In all other case, we can just use the non-strict version.

[nevans]

I feel like maybe expanding the scope to everything other than attributes either deserves separate PRs (automatically calling out in separate line items in generated release notes), or (at the very least) renaming this PR. :)

[tompng]

How about using arguments.grep(Prism::SymbolNode) instead of take_while and renaming the method?
I think it's worth adding a test like this, which is possible in the original attr_accessor spec

# document :foo, :bar and :baz
attr_accessor :foo, *ignored1, :bar, (ignored2), :baz

[nevans]

Yeah, I was trying to be a little bit conservative, since I wasn't sure if the change was intentional. But I'm even happier with that.

I feel like maybe both methods should be renamed, to avoid ambiguity confusion...

• this grep-based variation => any_symbol_arguments or every_symbol_argument
  (or grep_symbol_arguments? I prefer "any".)
• symbol_arguments => exclusively_symbol_arguments?

I (originally) added "node" to the names to emphasize that this is only matching symbol literal nodes.
(edited to add) But I dropped it in the version I pushed, both to be consistent with the original naming, and also because the methods are returning symbols and only working with literal SymbolNode is an implementation detail that can be inferred from context.

[nevans]

Do we want to preserve any form of test_undocumentable_attributes? That is, what if the arguments aren't variables or expressions that're too dynamic or too complex to extract names from, but are instead integers or other literals which suggest an entirely different API is being used?

I'm updating the PR to just repurpose that test for your grep suggestion. But that test case (added in fde99f1) makes me think that you maybe had a specific scenario you wanted to prohibit?

[tompng]

In my mind, there were two possible design:

1. Only accept attr_accessor :a, :b and reject attr_accessor :a, opt:
2. Accept attr_accessor :a, 1, expr, :b, true and also attr_accessor :a, opt:

And I selected the first one. test_undocumentable_attributes is testing this design decision.
IMO, if were going to be conservative, reject a usage that might be a completely different API, attr_accessor :a, opt: is one of the case that would be rejected. It's not ruby core attr_accessor usage. It's not a bug.
I think it's OK to change the design from 1 to 2, and allow any dynamic expression mixed with symbols.

[nevans]

@st0012 @tompng As a separate question, do we want to also allow string literal arguments? Because I'm pretty sure that all of the Module methods in question (attr, attr_reader, attr_writer, attr_accessor, alias_method, public, private, protected, module_function, private_class_function, public_class_function, private_constant, public_constant) also support string arguments. The only ones I was unsure about were the const visibility methods, and I just tested those (they work with strings).

BUT it's pretty weird to use string literals rather than symbol literals. Sure, for convenience, maybe we sometimes pass some variables or expressions into them which might evaluate to strings, to avoid the extra .to_sym calls. But using string literals just isn't done (at least, I haven't seen it in long time)!

So if it is done, maybe there's a reason, and maybe (for some of these) that reason could be that it's a simple (if somewhat hacky) way to avoid rdoc from picking up on them?

But I dunno... I can only really see that argument working for the attribute methods and alias_method. For the visibility methods, I think we should err on the side of reading every visibility change we can reasonably parse. So I'd want the visibility methods to support StringNode in addition to SymbolNode.

Anyway, after talking myself through it (and writing it down above), I'm leaning towards allowing both SymbolNode and StringNode for all of these. What do you think?

[nevans]

Related to that, I feel like maybe the following should not be ignored, but should issue a warning instead?

private def self.foo = bar
# => WARNING: "private def self.foo" makes "M#foo" private, not "M.foo"
private_class_method def foo = bar
# => WARNING: "private_class_method def foo" makes "M.foo" private, not "M#foo"
private def to_s.foo = bar
# => WARNING: "private def (expr).foo" makes "M#foo' private, not "(expr).foo"

Because def (expr).foo always returns :foo, all of those will make a(n existing) method private. Even if that isn't what the user expected, it's what they've done.

That's more scope than I want to push into this PR, but I'm curious about your thoughts?

[tompng]

Accepting StringNode is the original behavior in the old Ripper based parser. The Prism based parser just ported that behavior.

I think there is no strong reason to keep that behavior because string isn't used.
Actual code found on GitHub:
https://github.com/search?q=%22attr_accessor%28%5C%22%22+language%3ARuby&type=code&l=Ruby
Other: Dynamic string, RuboCop BAD example comment, test code.

I'm fine either way - keeping or dropping the behavior accepting StringNode.

private def self.foo = bar

I think it's OK to do that: Warn and make instance method foo private if it exist.

[nevans]

Accepting StringNode is the original behavior in the old Ripper based parser. The Prism based parser just ported that behavior.

@tompng Just to be clear: the Prism based parser does not currently accept string arguments for these methods. I think you're saying the old parser did support string arguments. So you're okay with me adding support for them to the Prism parser?

[nevans]

It looks like the current prism parser supports either StringNode or SymbolNode as the first argument for meta method comments (call_node_name_arguments) (edited to add:) or for meta programmed attributes (with an attribute directive).

[st0012]

I think we should accept string arguments in those calls.

[nevans]

@st0012 @tompng I've updated the PR to use @scanner.call_node_name_arguments, which is doing almost exactly what we want. I've also updated the parsing of metaprogrammed attribute name detection, so unparsed arguments don't generate unnamed attributes.

It might make sense to extract that method (and several others which process prism nodes and don't use any ivars) into a shared utilities module, but that's out of scope for this PR, IMO.

I'll post a second (stacked) PR to update the all of the visibility methods.