-
Notifications
You must be signed in to change notification settings - Fork 7
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
Remove redundant nil checks in RBI::Visitor
#312
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,33 +8,30 @@ class NestNonPublicMethods < Visitor | |
|
||
sig { override.params(node: T.nilable(Node)).void } | ||
def visit(node) | ||
return unless node | ||
|
||
case node | ||
when Tree | ||
Comment on lines
-13
to
-14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing these |
||
public_group = VisibilityGroup.new(Public.new) | ||
protected_group = VisibilityGroup.new(Protected.new) | ||
private_group = VisibilityGroup.new(Private.new) | ||
|
||
node.nodes.dup.each do |child| | ||
visit(child) | ||
next unless child.is_a?(Method) | ||
|
||
child.detach | ||
case child.visibility | ||
when Protected | ||
protected_group << child | ||
when Private | ||
private_group << child | ||
else | ||
public_group << child | ||
end | ||
return unless node.is_a?(Tree) | ||
|
||
public_group = VisibilityGroup.new(Public.new) | ||
protected_group = VisibilityGroup.new(Protected.new) | ||
private_group = VisibilityGroup.new(Private.new) | ||
|
||
node.nodes.dup.each do |child| | ||
visit(child) | ||
next unless child.is_a?(Method) | ||
|
||
child.detach | ||
case child.visibility | ||
when Protected | ||
protected_group << child | ||
when Private | ||
private_group << child | ||
else | ||
public_group << child | ||
end | ||
|
||
node << public_group unless public_group.empty? | ||
node << protected_group unless protected_group.empty? | ||
node << private_group unless private_group.empty? | ||
end | ||
|
||
node << public_group unless public_group.empty? | ||
node << protected_group unless protected_group.empty? | ||
node << private_group unless private_group.empty? | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,8 +81,6 @@ def visit_all(nodes) | |
|
||
sig { override.params(node: T.nilable(Node)).void } | ||
def visit(node) | ||
return unless node | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced removing these is a great idea. They're technically redundant, but they also fail fast on |
||
|
||
case node | ||
when Scope | ||
visit_all(node.nodes) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,28 +13,28 @@ def visit(node) | |
return unless node.is_a?(Tree) | ||
|
||
visit_all(node.nodes) | ||
original_order = node.nodes.map.with_index.to_h | ||
original_order = node.nodes.each_with_index.to_h | ||
|
||
# The child nodes could contain private/protected markers. If so, they should not be moved in the file. | ||
# Otherwise, some methods could see their privacy change. To avoid that problem, divide the array of child | ||
# nodes into chunks based on whether any Visibility nodes appear, and sort the chunks independently. This | ||
# applies the ordering rules from the node_rank method as much as possible, while preserving visibility. | ||
sorted_nodes = node.nodes.chunk do |n| | ||
n.is_a?(Visibility) | ||
end.flat_map do |_, nodes| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no Rubocop rule that can enforce this (last I checked), but having |
||
nodes.sort! do |a, b| | ||
# First we try to compare the nodes by their node rank (based on the node type) | ||
res = node_rank(a) <=> node_rank(b) | ||
next res if res != 0 # we can sort the nodes by their rank, let's stop here | ||
sorted_nodes = node.nodes | ||
.chunk { |n| n.is_a?(Visibility) } | ||
.flat_map do |_, nodes| | ||
nodes.sort! do |a, b| | ||
# First we try to compare the nodes by their node rank (based on the node type) | ||
res = node_rank(a) <=> node_rank(b) | ||
next res if res != 0 # we can sort the nodes by their rank, let's stop here | ||
|
||
# Then, if the nodes ranks are the same (res == 0), we try to compare the nodes by their name | ||
res = node_name(a) <=> node_name(b) | ||
next res if res && res != 0 # we can sort the nodes by their name, let's stop here | ||
# Then, if the nodes ranks are the same (res == 0), we try to compare the nodes by their name | ||
res = node_name(a) <=> node_name(b) | ||
next res if res && res != 0 # we can sort the nodes by their name, let's stop here | ||
|
||
# Finally, if the two nodes have the same rank and the same name or at least one node is anonymous then, | ||
T.must(original_order[a]) <=> T.must(original_order[b]) # we keep the original order | ||
# Finally, if the two nodes have the same rank and the same name or at least one node is anonymous then, | ||
T.must(original_order[a]) <=> T.must(original_order[b]) # we keep the original order | ||
end | ||
end | ||
end | ||
|
||
node.nodes.replace(sorted_nodes) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this causes a type failure before, even if I make
Indexable
a subtype ofNode
(by addingrequires_ancestor { Node }
):Should
T.all(RBI::Indexable, T.nilable(RBI::Node))
be simplified toT.all(RBI::Indexable, RBI::Node)
(since it needs to be Indexable, it can't also be nil), and therefore be a subtype ofRBI::Node
?