Skip to content
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.

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

Define Node#*_type? methods using macro #297

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sambostock
Copy link
Contributor

@sambostock sambostock commented Jun 20, 2024

This allows us to document the methods, without the verbosity of statically defining each one individually.

Specs are dynamically generated for these methods, so there is no danger of omitting any.

I don't think we need a changelog entry, as no functionality has changed, but I'm happy to add one if desired.


To view the generated docs locally, run yard server -r in your terminal and visit http://localhost:8808/docs/RuboCop/AST/Node in your browser.

image

...

image

...

Comment on lines +120 to +183
# @!method $1_type?
# @return [Boolean]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about adding more to the documentation. On the one hand, more details are generally good.

Suggested change
# @!method $1_type?
# @return [Boolean]
# @!method $1_type?
# Checks if the node is of type +${-1}+.
# @return [Boolean] +true+ if the node is of type +${-1}+, +false+ otherwise.
image

...

image

...


Note that if we add a detailed @return without a description, YARD repeats the @return as the description:

Suggested change
# @!method $1_type?
# @return [Boolean]
# @!method $1_type?
# @return [Boolean] +true+ if the node is of type +${-1}+, +false+ otherwise.
image

...

image

...

Comment on lines +122 to +316
def_node_type_predicate :op_asgn
def_node_type_predicate :and_asgn
def_node_type_predicate :ensure
def_node_type_predicate :rescue
def_node_type_predicate :arg_expr
def_node_type_predicate :or_asgn
def_node_type_predicate :back_ref
def_node_type_predicate :nth_ref
def_node_type_predicate :match_with_lvasgn
def_node_type_predicate :match_current_line
def_node_type_predicate :module
def_node_type_predicate :class
def_node_type_predicate :sclass
def_node_type_predicate :def
def_node_type_predicate :defs
def_node_type_predicate :undef
def_node_type_predicate :alias
def_node_type_predicate :args
def_node_type_predicate :cbase
def_node_type_predicate :arg
def_node_type_predicate :optarg
def_node_type_predicate :restarg
def_node_type_predicate :blockarg
def_node_type_predicate :block_pass
def_node_type_predicate :kwarg
def_node_type_predicate :kwoptarg
def_node_type_predicate :kwrestarg
def_node_type_predicate :kwnilarg
def_node_type_predicate :csend
def_node_type_predicate :super
def_node_type_predicate :zsuper
def_node_type_predicate :yield
def_node_type_predicate :block
def_node_type_predicate :and
def_node_type_predicate :not
def_node_type_predicate :or
def_node_type_predicate :if
def_node_type_predicate :when
def_node_type_predicate :case
def_node_type_predicate :while
def_node_type_predicate :until
def_node_type_predicate :while_post
def_node_type_predicate :until_post
def_node_type_predicate :for
def_node_type_predicate :break
def_node_type_predicate :next
def_node_type_predicate :redo
def_node_type_predicate :return
def_node_type_predicate :resbody
def_node_type_predicate :kwbegin
def_node_type_predicate :begin
def_node_type_predicate :retry
def_node_type_predicate :preexe
def_node_type_predicate :postexe
def_node_type_predicate :iflipflop
def_node_type_predicate :eflipflop
def_node_type_predicate :shadowarg
def_node_type_predicate :complex
def_node_type_predicate :rational
def_node_type_predicate :__FILE__
def_node_type_predicate :__LINE__
def_node_type_predicate :__ENCODING__
def_node_type_predicate :ident
def_node_type_predicate :lambda
def_node_type_predicate :indexasgn
def_node_type_predicate :index
def_node_type_predicate :procarg0
def_node_type_predicate :restarg_expr
def_node_type_predicate :blockarg_expr
def_node_type_predicate :objc_kwarg
def_node_type_predicate :objc_restarg
def_node_type_predicate :objc_varargs
def_node_type_predicate :numargs
def_node_type_predicate :numblock
def_node_type_predicate :forward_args
def_node_type_predicate :forwarded_args
def_node_type_predicate :forward_arg
def_node_type_predicate :case_match
def_node_type_predicate :in_match
def_node_type_predicate :in_pattern
def_node_type_predicate :match_var
def_node_type_predicate :pin
def_node_type_predicate :match_alt
def_node_type_predicate :match_as
def_node_type_predicate :match_rest
def_node_type_predicate :array_pattern
def_node_type_predicate :match_with_trailing_comma
def_node_type_predicate :array_pattern_with_tail
def_node_type_predicate :hash_pattern
def_node_type_predicate :const_pattern
def_node_type_predicate :if_guard
def_node_type_predicate :unless_guard
def_node_type_predicate :match_nil_pattern
def_node_type_predicate :empty_else
def_node_type_predicate :find_pattern
def_node_type_predicate :kwargs
def_node_type_predicate :match_pattern_p
def_node_type_predicate :match_pattern
def_node_type_predicate :forwarded_restarg
def_node_type_predicate :forwarded_kwrestarg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are in the same order as Parser::Meta::NODE_TYPES, excluding :send. Alternatively, they could be sorted lexicographically.

Copy link
Member

@dvandersluis dvandersluis Nov 9, 2024

Choose a reason for hiding this comment

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

Any reason we couldn't just iterate over that set instead of having a line for each one? Never mind I guess that takes us back to where we started 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, yeah, the goal of the PR is to do it statically enough that YARD can generate docs.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to extract the lines to a module to include and still retain the benefit? Node is already a very large file and there are 132 lines of def_node_type_predicate that to me would make it harder to go through.

Comment on lines +257 to +320
# Most nodes are of 'send' type, so this method is defined
# separately to make this check as fast as possible.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This documentation is specific to the implementation, but does not affect consumers, so I moved it inside the method.

def_node_type_predicate :cvar
def_node_type_predicate :gvar
def_node_type_predicate :const
def_node_type_predicate :defined, :defined?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This node type is an edge case because it contains a ?, so we have to distinguish between the name to use as part of the method and the type to check for. We used to do this dynamically, but that prevents documentation.

end # end
RUBY
end
# @!group Type Predicates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grouped these together so they don't add noise in the Instance Method Summary section.

@marcandre
Copy link
Contributor

Very nice PR, thank you.

I have one concern though which is that the current code is future proof, in that it will work fine with newer versions of parser, even if these future versions add a new node type.
Maybe we should add call def_node_type_predicate on the list of node types - the list of node types we know of, in case that list isn't empty?

@bbatsov
Copy link
Contributor

bbatsov commented Nov 7, 2024

@sambostock ping :-)

This allows us to document the methods, without the verbosity of
statically defining each one individually.

Specs are dynamically generated for these methods, so there is no danger
of omitting any.
@sambostock
Copy link
Contributor Author

@marcandre I've tentatively pushed a commit which adds the fallback. However, the only scenario I can think of in which it would be relevant would be if the host application bumps their parser version and gains support for a new Ruby version's syntax (or some other change that introduces a node type), and has custom cops which expect to use a predicate method for the new node type.

This seems unlikely, and I'm not entirely sure if it's possible (wouldn't they need to use a version of RuboCop supporting their TargetRubyVersion?). I made sure to add a test which fails if any new Parser::Meta::NODE_TYPES are introduced. Do you think that's sufficient, or is there a scenario I'm not thinking of?

While we have tests which enforce that we're calling
`def_node_type_predicate` for all known `Parser::Meta::NODE_TYPES`, it
is possible for a host application to use a newer version of `parser`,
which might support additional nodes, and for the application to attempt
to access those nodes in custom cops.

To preserve the previous forward compatibility, we fallback to
generating any missing methods. They won't be documented, but at least
they'll work.

The tests will enforce that if rubocop-ast bumps its Parser version, all
node type predicates are generated via `dev_node_type_predicate`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants