Less strict parsing of visibility method arguments - #1803
Open
nevans wants to merge 5 commits into
Open
Conversation
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 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:
```ruby
# 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 ruby#1790.
This updates inferred attribute name parsing for the `:attr:`, `:attr_reader:`, `:attr_writer:`, and `:attr_accessor:` directives, to avoid creating unnamed attributes from unparsable arguments. Previously, an unnamed (nil) attribute would be created. Now, any non-string/non-symbol arguments are simply ignored.
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 commit, 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).
There aren't any `private_class_function` and `public_class_function` methods. 😉
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.
Contributor
Author
|
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. |
Collaborator
|
🚀 Preview deployment available at: https://1a6e72ae.rdoc-6cd.pages.dev (commit: 242a1e9) |
st0012
approved these changes
Aug 31, 2026
st0012
left a comment
st0012
left a comment
Member
There was a problem hiding this comment.
LGTM, just some nits
Thanks for fixing it 👍
Comment on lines
+1348
to
+1349
| return unless names = call_node_name_arguments(call_node) | ||
| @scanner.container.set_constant_visibility_for(names, :public) |
Member
There was a problem hiding this comment.
nit: I think the original condition shape can be kept and is clearer
Suggested change
| return unless names = call_node_name_arguments(call_node) | |
| @scanner.container.set_constant_visibility_for(names, :public) | |
| names = call_node_name_arguments(call_node) | |
| @scanner.container.set_constant_visibility_for(names, :public) if names |
| names = symbol_arguments(call_node) | ||
| @scanner.container.set_constant_visibility_for(names.map(&:to_s), :private) if names | ||
| return unless names = call_node_name_arguments(call_node) | ||
| @scanner.container.set_constant_visibility_for(names, :private) |
| names = symbol_arguments(call_node) | ||
| @scanner.add_attributes(names.map(&:to_s), rw, call_node.location.start_line) if names | ||
| return unless names = call_node_name_arguments(call_node) | ||
| @scanner.add_attributes(names, rw, call_node.location.start_line) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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,protectedprivate_class_method,public_class_methodprivate_constant,public_constantmodule_functionPrior 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_argumentsis used to parse the arguments list).Additionally:
Parser::Rubydocs forprivate_class_methodandpublic_class_method.There aren't any
private_class_functionandpublic_class_functionmethods. 😉Parser::Ruby#call_node_name_argumentsinto 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_mapand convert[]tonil, 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.