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

Jump to bottom

on_os_blocks: add audit #7436

Merged
merged 1 commit into from Jun 5, 2020
Merged

on_os_blocks: add audit #7436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
178 changes: 148 additions & 30 deletions Library/Homebrew/rubocops/components_order.rb
View file
Open in desktop
Expand Up @@ -10,6 +10,11 @@ module FormulaAudit
# - `component_precedence_list` has component hierarchy in a nested list
# where each sub array contains components' details which are at same precedence level
class ComponentsOrder < FormulaCop
# `aspell`: options and resources should be grouped by language
COMPONENT_WHITELIST = %w[
aspell
].freeze

def audit_formula(_node, _class_node, _parent_class_node, body_node)
component_precedence_list = [
[{ name: :include, type: :method_call }],
Expand All @@ -34,6 +39,8 @@ def audit_formula(_node, _class_node, _parent_class_node, body_node)
[{ name: :deprecated_option, type: :method_call }],
[{ name: :depends_on, type: :method_call }],
[{ name: :uses_from_macos, type: :method_call }],
[{ name: :on_macos, type: :block_call }],
[{ name: :on_linux, type: :block_call }],
[{ name: :conflicts_with, type: :method_call }],
[{ name: :skip_clean, type: :method_call }],
[{ name: :cxxstdlib_check, type: :method_call }],
Expand All @@ -49,50 +56,115 @@ def audit_formula(_node, _class_node, _parent_class_node, body_node)
[{ name: :test, type: :block_call }],
]

@present_components = component_precedence_list.map do |components|
relevant_components = []
components.each do |component|
case component[:type]
when :method_call
relevant_components += find_method_calls_by_name(body_node, component[:name]).to_a
when :block_call
relevant_components += find_blocks(body_node, component[:name]).to_a
when :method_definition
relevant_components << find_method_def(body_node, component[:name])
end
end
relevant_components.delete_if(&:nil?)
@present_components, @offensive_nodes = check_order(component_precedence_list, body_node)

component_problem @offensive_nodes[0], @offensive_nodes[1] if @offensive_nodes

component_precedence_list = [
[{ name: :depends_on, type: :method_call }],
[{ name: :resource, type: :block_call }],
[{ name: :patch, type: :method_call }, { name: :patch, type: :block_call }],
]

on_macos_blocks = find_blocks(body_node, :on_macos)

if on_macos_blocks.length > 1
@offensive_node = on_macos_blocks.second
@offense_source_range = on_macos_blocks.second.source_range
problem "there can only be one `on_macos` block in a formula."
end

# Check if each present_components is above rest of the present_components
@present_components.take(@present_components.size - 1).each_with_index do |preceding_component, p_idx|
next if preceding_component.empty?
check_on_os_block_content(component_precedence_list, on_macos_blocks.first) if on_macos_blocks.any?

@present_components.drop(p_idx + 1).each do |succeeding_component|
next if succeeding_component.empty?
on_linux_blocks = find_blocks(body_node, :on_linux)

if on_linux_blocks.length > 1
@offensive_node = on_linux_blocks.second
@offense_source_range = on_linux_blocks.second.source_range
problem "there can only be one `on_linux` block in a formula."
end

check_on_os_block_content(component_precedence_list, on_linux_blocks.first) if on_linux_blocks.any?

resource_blocks = find_blocks(body_node, :resource)
resource_blocks.each do |resource_block|
on_macos_blocks = find_blocks(resource_block.body, :on_macos)
on_linux_blocks = find_blocks(resource_block.body, :on_linux)

@offensive_nodes = check_precedence(preceding_component, succeeding_component)
component_problem @offensive_nodes[0], @offensive_nodes[1] if @offensive_nodes
if on_macos_blocks.length.zero? && on_linux_blocks.length.zero?
# Found nothing. Try without .body as depending on the code,
# on_macos or on_linux might be in .body or not ...
on_macos_blocks = find_blocks(resource_block, :on_macos)
on_linux_blocks = find_blocks(resource_block, :on_linux)

next if on_macos_blocks.length.zero? && on_linux_blocks.length.zero?
end

@offensive_node = resource_block
@offense_source_range = resource_block.source_range

if on_macos_blocks.length > 1
problem "there can only be one `on_macos` block in a resource block."
next
end

if on_linux_blocks.length > 1
problem "there can only be one `on_linux` block in a resource block."
next
end

if on_macos_blocks.length == 1 && on_linux_blocks.length.zero?
problem "you need to define an `on_linux` block within your resource block."
next
end

if on_macos_blocks.length.zero? && on_linux_blocks.length == 1
problem "you need to define an `on_macos` block within your resource block."
next
end

on_macos_block = on_macos_blocks.first
on_linux_block = on_linux_blocks.first

child_nodes = on_macos_block.body.child_nodes
if child_nodes[0].method_name.to_s != "url" && child_nodes[1].method_name.to_s != "sha256"
problem "only an url and a sha256 (in the right order) are allowed in a `on_macos` " \
"block within a resource block."
next
end

child_nodes = on_linux_block.body.child_nodes
if child_nodes[0].method_name.to_s != "url" && child_nodes[1].method_name.to_s != "sha256"
problem "only an url and a sha256 (in the right order) are allowed in a `on_linux` " \
"block within a resource block."
end
end
end

# `aspell`: options and resources should be grouped by language
WHITELIST = %w[
aspell
].freeze
def check_on_os_block_content(component_precedence_list, on_os_block)
_, offensive_node = check_order(component_precedence_list, on_os_block.body)
component_problem(*offensive_node) if offensive_node
on_os_block.body.child_nodes.each do |child|
valid_node = depends_on_node?(child)
# Check for RuboCop::AST::SendNode instances only, as we are checking the
# method_name for patches and resources.
next unless child.instance_of? RuboCop::AST::SendNode
iMichka marked this conversation as resolved.
Show resolved Hide resolved

# Method to format message for reporting component precedence violations
def component_problem(c1, c2)
return if WHITELIST.include?(@formula_name)
valid_node ||= child.method_name.to_s == "patch"
valid_node ||= child.method_name.to_s == "resource"

problem "`#{format_component(c1)}` (line #{line_number(c1)}) " \
"should be put before `#{format_component(c2)}` " \
"(line #{line_number(c2)})"
@offensive_node = on_os_block
@offense_source_range = on_os_block.source_range
unless valid_node
problem "`#{on_os_block.method_name}` can only include `depends_on`, `patch` and `resource` nodes."
end
end
end

# autocorrect method gets called just after component_problem method call
def autocorrect(_node)
return if @offensive_nodes.nil?

succeeding_node = @offensive_nodes[0]
preceding_node = @offensive_nodes[1]
lambda do |corrector|
Expand Down Expand Up @@ -130,6 +202,52 @@ def get_state(node1)
return [idx, comp.index(node1), comp] if comp.member?(node1)
end
end

def check_order(component_precedence_list, body_node)
present_components = component_precedence_list.map do |components|
components.flat_map do |component|
case component[:type]
when :method_call
find_method_calls_by_name(body_node, component[:name]).to_a
when :block_call
find_blocks(body_node, component[:name]).to_a
when :method_definition
find_method_def(body_node, component[:name])
end
end.compact
end

# Check if each present_components is above rest of the present_components
offensive_nodes = nil
present_components.take(present_components.size - 1).each_with_index do |preceding_component, p_idx|
next if preceding_component.empty?

present_components.drop(p_idx + 1).each do |succeeding_component|
next if succeeding_component.empty?

offensive_nodes = check_precedence(preceding_component, succeeding_component)
break if offensive_nodes
end
end

[present_components, offensive_nodes]
end

# Method to format message for reporting component precedence violations
def component_problem(c1, c2)
return if COMPONENT_WHITELIST.include?(@formula_name)

problem "`#{format_component(c1)}` (line #{line_number(c1)}) " \
"should be put before `#{format_component(c2)}` " \
"(line #{line_number(c2)})"
end

# Node pattern method to match
# `depends_on` variants
def_node_matcher :depends_on_node?, <<~EOS
{(if _ (send nil? :depends_on ...) nil?)
(send nil? :depends_on ...)}
EOS
end
end
end
Expand Down