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.

Sign up for GitHub

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
Open

Define Node#*_type? methods using macro #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3470a3c
Define `Node#*_type?` methods using macro
sambostock Jun 19, 2024
78c7935
Fallback to dynamically defining node type predicates
sambostock Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 162 additions & 10 deletions lib/rubocop/ast/node.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,18 @@ def #{recursive_kind} # def recursive_litera
RUBY
end

# Define a +_type?+ predicate method for the given node type.
private_class_method def self.def_node_type_predicate(name, type = name)
@node_types_with_documented_predicate_method << type

class_eval <<~RUBY, __FILE__, __LINE__ + 1
def #{name}_type? # def block_type?
@type == :#{type} # @type == :block
end # end
RUBY
end
@node_types_with_documented_predicate_method = []

# @see https://www.rubydoc.info/gems/ast/AST/Node:initialize
def initialize(type, children = EMPTY_CHILDREN, properties = EMPTY_PROPERTIES)
@mutable_attributes = {}
Expand Down Expand Up @@ -164,20 +176,160 @@ def type?(*types)
!group_type.nil? && types.include?(group_type)
end

(Parser::Meta::NODE_TYPES - [:send]).each do |node_type|
method_name = "#{node_type.to_s.gsub(/\W/, '')}_type?"
class_eval <<~RUBY, __FILE__, __LINE__ + 1
def #{method_name} # def block_type?
@type == :#{node_type} # @type == :block
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.


# @!macro [attach] def_node_type_predicate
# @!method $1_type?
# @return [Boolean]
Comment on lines +182 to +183
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

...

def_node_type_predicate :true
def_node_type_predicate :false
def_node_type_predicate :nil
def_node_type_predicate :int
def_node_type_predicate :float
def_node_type_predicate :str
def_node_type_predicate :dstr
def_node_type_predicate :sym
def_node_type_predicate :dsym
def_node_type_predicate :xstr
def_node_type_predicate :regopt
def_node_type_predicate :regexp
def_node_type_predicate :array
def_node_type_predicate :splat
def_node_type_predicate :pair
def_node_type_predicate :kwsplat
def_node_type_predicate :hash
def_node_type_predicate :irange
def_node_type_predicate :erange
def_node_type_predicate :self
def_node_type_predicate :lvar
def_node_type_predicate :ivar
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.

def_node_type_predicate :lvasgn
def_node_type_predicate :ivasgn
def_node_type_predicate :cvasgn
def_node_type_predicate :gvasgn
def_node_type_predicate :casgn
def_node_type_predicate :mlhs
def_node_type_predicate :masgn
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
Comment on lines +184 to +316
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 •
edited
Loading

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.


# Most nodes are of 'send' type, so this method is defined
# separately to make this check as fast as possible.
def send_type?
# Most nodes are of 'send' type, so this method is defined
# separately to make this check as fast as possible.
Comment on lines +319 to +320
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.

false
end
@node_types_with_documented_predicate_method << :send

# Ensure forward compatibility with new node types by defining methods for unknown node types.
# Note these won't get auto-generated documentation, which is why we prefer defining them above.
(Parser::Meta::NODE_TYPES - @node_types_with_documented_predicate_method).each do |node_type|
method_name = :"#{node_type.to_s.gsub(/\W/, '')}_type?"
define_method(method_name) { false }
end

# @!endgroup

# Returns the parent node, or `nil` if the receiver is a root node.
#
Expand Down
40 changes: 40 additions & 0 deletions spec/rubocop/ast/node_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,4 +1109,44 @@ class << expr
end
end
end

Parser::Meta::NODE_TYPES.each do |node_type|
node_name = node_type.to_s.gsub(/\W/, '')
method_name = :"#{node_name}_type?"

describe "##{method_name}" do
it 'is false' do
expect(described_class.allocate.public_send(method_name)).to be(false)
end

it 'is documented' do
expect(node_type_predicate_is_documented?(node_type)).to(
be(true),
missing_documentation_failure_message(method_name, node_name, node_type)
)
end

private

def node_type_predicate_is_documented?(node_type)
described_class
.instance_variable_get(:@node_types_with_documented_predicate_method)
.include?(node_type)
end

def missing_documentation_failure_message(method_name, node_name, node_type)
name_matches_type = node_type.to_s == node_name

<<~MSG
#{described_class.name}##{method_name} is not documented as it was generated automatically as a fallback.

To fix this, define it using the following macro instead:

class #{described_class.name} < #{described_class.superclass.name}
# ...
def_node_type_predicate :#{node_name}#{", :#{node_type}" unless name_matches_type}
MSG
end
end
end
end