Port audit_components method to rubocops and add corresponding tests

4 files changed, 191 insertions, 17 deletions

diff --git a/Library/Homebrew/rubocops/bottle_block_cop.rb b/Library/Homebrew/rubocops/bottle_block_cop.rb
index 4d7a94461..141d87b35 100644
--- a/Library/Homebrew/rubocops/bottle_block_cop.rb
+++ b/Library/Homebrew/rubocops/bottle_block_cop.rb
@@ -13,7 +13,7 @@ module RuboCop
         def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
           bottle = find_block(formula_class_body_node, :bottle)
           return if bottle.nil? || block_size(bottle).zero?
-          problem "Use rebuild instead of revision in bottle block" if method_called?(bottle, :revision)
+          problem "Use rebuild instead of revision in bottle block" if method_called_in_block?(bottle, :revision)
         end
 
         private
diff --git a/Library/Homebrew/rubocops/components_order_cop.rb b/Library/Homebrew/rubocops/components_order_cop.rb
new file mode 100644
index 000000000..c66b5614e
--- /dev/null
+++ b/Library/Homebrew/rubocops/components_order_cop.rb
@@ -0,0 +1,71 @@
+require_relative "./extend/formula_cop"
+
+module RuboCop
+  module Cop
+    module Homebrew
+      # This cop checks for correct order of components in a Formula
+      #
+      # - component_precedence_list has component hierarchy in a nested list
+      #   where each sub array contains components' details which are at same precedence level
+      class FormulaComponentsOrder < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
+          component_precedence_list = [
+            [{ name: :include,  type: :method_call }],
+            [{ name: :desc,     type: :method_call }],
+            [{ name: :homepage, type: :method_call }],
+            [{ name: :url,      type: :method_call }],
+            [{ name: :mirror,   type: :method_call }],
+            [{ name: :version,  type: :method_call }],
+            [{ name: :sha256,   type: :method_call }],
+            [{ name: :revision, type: :method_call }],
+            [{ name: :version_scheme, type: :method_call }],
+            [{ name: :head,     type: :method_call }],
+            [{ name: :stable,   type: :block_call }],
+            [{ name: :bottle,   type: :block_call }],
+            [{ name: :devel,    type: :block_call }],
+            [{ name: :head,     type: :block_call }],
+            [{ name: :bottle,   type: :method_call }],
+            [{ name: :keg_only, type: :method_call }],
+            [{ name: :option,   type: :method_call }],
+            [{ name: :depends_on, type: :method_call }],
+            [{ name: :conflicts_with, type: :method_call }],
+            [{ name: :go_resource, type: :block_call }, { name: :resource, type: :block_call }],
+            [{ name: :install, type: :method_definition }],
+            [{ name: :caveats, type: :method_definition }],
+            [{ name: :plist_options, type: :method_call }, { name: :plist, type: :method_definition }],
+            [{ 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(formula_class_body_node, component[:name]).to_a
+              when :block_call
+                relevant_components += find_blocks(formula_class_body_node, component[:name]).to_a
+              when :method_definition
+                relevant_components << find_method_def(formula_class_body_node, component[:name])
+              end
+            end
+            relevant_components.delete_if(&:nil?)
+          end
+
+          present_components = present_components.delete_if(&:empty?)
+
+          present_components.each_cons(2) do |preceding_component, succeeding_component|
+            offensive_nodes = check_precedence(preceding_component, succeeding_component)
+            component_problem offensive_nodes[0], offensive_nodes[1] if offensive_nodes
+          end
+        end
+
+        private
+
+        def component_problem(c1, c2)
+          # Method to format message for reporting component precedence violations
+          problem "`#{format_component(c1)}` (line #{line_number(c1)}) should be put before `#{format_component(c2)}` (line #{line_number(c2)})"
+        end
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/rubocops/components_redundancy_cop.rb b/Library/Homebrew/rubocops/components_redundancy_cop.rb
new file mode 100644
index 000000000..3d95d6c05
--- /dev/null
+++ b/Library/Homebrew/rubocops/components_redundancy_cop.rb
@@ -0,0 +1,33 @@
+require_relative "./extend/formula_cop"
+
+module RuboCop
+  module Cop
+    module Homebrew
+      # This cop checks if redundant components are present and other component errors
+      #
+      # - `url|checksum|mirror` should be inside `stable` block
+      # - `head` and `head do` should not be simultaneously present
+      # - `bottle :unneeded/:disable` and `bottle do` should not be simultaneously present
+
+      class ComponentsRedundancy < FormulaCop
+        HEAD_MSG = "`head` and `head do` should not be simultaneously present".freeze
+        BOTTLE_MSG = "`bottle :modifier` and `bottle do` should not be simultaneously present".freeze
+
+        def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
+          stable_block = find_block(formula_class_body_node, :stable)
+          if stable_block
+            [:url, :sha256, :mirror].each do |method_name|
+              problem "`#{method_name}` should be put inside `stable` block" if method_called?(formula_class_body_node, method_name)
+            end
+          end
+
+          problem HEAD_MSG if method_called?(formula_class_body_node, :head) &&
+                              find_block(formula_class_body_node, :head)
+
+          problem BOTTLE_MSG if method_called?(formula_class_body_node, :bottle) &&
+                                find_block(formula_class_body_node, :bottle)
+        end
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb
index 49108bd48..11e5d93d3 100644
--- a/Library/Homebrew/rubocops/extend/formula_cop.rb
+++ b/Library/Homebrew/rubocops/extend/formula_cop.rb
@@ -4,8 +4,8 @@ module RuboCop
       class FormulaCop < Cop
         @registry = Cop.registry
 
+        # This method is called by RuboCop and is the main entry point
         def on_class(node)
-          # This method is called by RuboCop and is the main entry point
           file_path = processed_source.buffer.name
           return unless file_path_allowed?(file_path)
           class_node, parent_class_node, body = *node
@@ -15,9 +15,9 @@ module RuboCop
           audit_formula(node, class_node, parent_class_node, body)
         end
 
+        # Checks for regex match of pattern in the node and
+        # Sets the appropriate instance variables to report the match
         def regex_match_group(node, pattern)
-          # Checks for regex match of pattern in the node and
-          # Sets the appropriate instance variables to report the match
           string_repr = string_content(node)
           match_object = string_repr.match(pattern)
           return unless match_object
@@ -32,8 +32,8 @@ module RuboCop
           match_object
         end
 
+        # Returns method_node matching method_name
         def find_node_method_by_name(node, method_name)
-          # Returns method_node matching method_name
           return if node.nil?
           node.each_child_node(:send) do |method_node|
             next unless method_node.method_name == method_name
@@ -47,8 +47,14 @@ module RuboCop
           nil
         end
 
+        # Returns an array of method call nodes matching method_name inside node
+        def find_method_calls_by_name(node, method_name)
+          return if node.nil?
+          node.each_child_node(:send).select { |method_node| method_name == method_node.method_name }
+        end
+
+        # Returns a block named block_name inside node
         def find_block(node, block_name)
-          # Returns a block named block_name inside node
           return if node.nil?
           node.each_child_node(:block) do |block_node|
             next if block_node.method_name != block_name
@@ -62,8 +68,30 @@ module RuboCop
           nil
         end
 
-        def method_called?(node, method_name)
-          # Check if a method is called inside a block
+        # Returns an array of block nodes named block_name inside node
+        def find_blocks(node, block_name)
+          return if node.nil?
+          node.each_child_node(:block).select { |block_node| block_name == block_node.method_name }
+        end
+
+        # Returns a method definition node with method_name
+        def find_method_def(node, method_name)
+          return if node.nil?
+          node.each_child_node(:def) do |def_node|
+            def_method_name = method_name(def_node)
+            next unless method_name == def_method_name
+            @offensive_node = def_node
+            @offense_source_range = def_node.source_range
+            return def_node
+          end
+          # If not found then, parent node becomes the offensive node
+          @offensive_node = node.parent
+          @offense_source_range = node.parent.source_range
+          nil
+        end
+
+        # Check if a method is called inside a block
+        def method_called_in_block?(node, method_name)
           block_body = node.children[2]
           block_body.each_child_node(:send) do |call_node|
             next unless call_node.method_name == method_name
@@ -74,54 +102,96 @@ module RuboCop
           false
         end
 
+        # Check if method_name is called among the direct children nodes in the given node
+        def method_called?(node, method_name)
+          node.each_child_node(:send) do |call_node|
+            next unless call_node.method_name == method_name
+            @offensive_node = call_node
+            @offense_source_range = call_node.source_range
+            return true
+          end
+          false
+        end
+
+        # Checks for precedence, returns the first pair of precedence violating nodes
+        def check_precedence(first_nodes, next_nodes)
+          next_nodes.each do |each_next_node|
+            first_nodes.each do |each_first_node|
+              if component_precedes?(each_first_node, each_next_node)
+                return [each_first_node, each_next_node]
+              end
+            end
+          end
+          nil
+        end
+
+        # If first node does not precede next_node, sets appropriate instance variables for reporting
+        def component_precedes?(first_node, next_node)
+          return false if line_number(first_node) < line_number(next_node)
+          @offense_source_range = first_node.source_range
+          @offensive_node = first_node
+          true
+        end
+
+        # Returns the array of arguments of the method_node
         def parameters(method_node)
-          # Returns the array of arguments of the method_node
           return unless method_node.send_type?
           method_node.method_args
         end
 
+        # Returns the begin position of the node's line in source code
         def line_start_column(node)
-          # Returns the begin position of the node's line in source code
           node.source_range.source_buffer.line_range(node.loc.line).begin_pos
         end
 
+        # Returns the begin position of the node in source code
         def start_column(node)
-          # Returns the begin position of the node in source code
           node.source_range.begin_pos
         end
 
+        # Returns the line number of the node
         def line_number(node)
-          # Returns the line number of the node
           node.loc.line
         end
 
+        # Returns the class node's name, nil if not a class node
         def class_name(node)
-          # Returns the class node's name, nil if not a class node
           @offensive_node = node
           @offense_source_range = node.source_range
           node.const_name
         end
 
+        # Returns the method name for a def node
+        def method_name(node)
+          node.children[0] if node.def_type?
+        end
+
+        # Returns the node size in the source code
         def size(node)
-          # Returns the node size in the source code
           node.source_range.size
         end
 
+        # Returns the block length of the block node
         def block_size(block)
-          # Returns the block length of the block node
           block_length(block)
         end
 
+        # Source buffer is required as an argument to report style violations
         def source_buffer(node)
-          # Source buffer is required as an argument to report style violations
           node.source_range.source_buffer
         end
 
+        # Returns the string representation if node is of type str
         def string_content(node)
-          # Returns the string representation if node is of type str
           node.str_content if node.type == :str
         end
 
+        # Returns printable component name
+        def format_component(component_node)
+          return component_node.method_name if component_node.send_type? || component_node.block_type?
+          method_name(component_node) if component_node.def_type?
+        end
+
         def problem(msg)
           add_offense(@offensive_node, @offense_source_range, msg)
         end