audit: audit_components method to rubocops and tests

Library/.rubocop.yml

@@ -14,6 +14,12 @@ Homebrew/CorrectBottleBlock:
 Homebrew/FormulaDesc:
   Enabled: true
 
+Homebrew/FormulaComponentsOrder:
+  Enabled: true
+
+Homebrew/ComponentsRedundancy:
+  Enabled: true
+
 Metrics/AbcSize:
   Enabled: false
 

Library/Homebrew/dev-cmd/audit.rb

@@ -248,70 +248,6 @@ def audit_style
     end
   end
 
-  def component_problem(before, after, offset = 0)
-    problem "`#{before[1]}` (line #{before[0] + offset}) should be put before `#{after[1]}` (line #{after[0] + offset})"
-  end
-
-  # scan in the reverse direction for remaining problems but report problems
-  # in the forward direction so that contributors don't reverse the order of
-  # lines in the file simply by following instructions
-  def audit_components(reverse = true, previous_pair = nil)
-    component_list = [
-      [/^  include Language::/,            "include directive"],
-      [/^  desc ["'][\S\ ]+["']/,          "desc"],
-      [/^  homepage ["'][\S\ ]+["']/,      "homepage"],
-      [/^  url ["'][\S\ ]+["']/,           "url"],
-      [/^  mirror ["'][\S\ ]+["']/,        "mirror"],
-      [/^  version ["'][\S\ ]+["']/,       "version"],
-      [/^  (sha1|sha256) ["'][\S\ ]+["']/, "checksum"],
-      [/^  revision/,                      "revision"],
-      [/^  version_scheme/,                "version_scheme"],
-      [/^  head ["'][\S\ ]+["']/,          "head"],
-      [/^  stable do/,                     "stable block"],
-      [/^  bottle do/,                     "bottle block"],
-      [/^  devel do/,                      "devel block"],
-      [/^  head do/,                       "head block"],
-      [/^  bottle (:unneeded|:disable)/,   "bottle modifier"],
-      [/^  keg_only/,                      "keg_only"],
-      [/^  option/,                        "option"],
-      [/^  depends_on/,                    "depends_on"],
-      [/^  conflicts_with/,                "conflicts_with"],
-      [/^  (go_)?resource/,                "resource"],
-      [/^  def install/,                   "install method"],
-      [/^  def caveats/,                   "caveats method"],
-      [/^  (plist_options|def plist)/,     "plist block"],
-      [/^  test do/,                       "test block"],
-    ]
-    if previous_pair
-      previous_before = previous_pair[0]
-      previous_after = previous_pair[1]
-    end
-    offset = previous_after && previous_after[0] && previous_after[0] >= 1 ? previous_after[0] - 1 : 0
-    present = component_list.map do |regex, name|
-      lineno = if reverse
-        text.reverse_line_number regex
-      else
-        text.line_number regex, offset
-      end
-      next unless lineno
-      [lineno, name]
-    end.compact
-    no_problem = true
-    present.each_cons(2) do |c1, c2|
-      if reverse
-        # scan in the forward direction from the offset
-        audit_components(false, [c1, c2]) if c1[0] > c2[0] # at least one more offense
-      elsif c1[0] > c2[0] && (offset.zero? || previous_pair.nil? || (c1[0] + offset) != previous_before[0] || (c2[0] + offset) != previous_after[0])
-        component_problem c1, c2, offset
-        no_problem = false
-      end
-    end
-    if no_problem && previous_pair
-      component_problem previous_before, previous_after
-    end
-    present
-  end
-
   def audit_file
     # Under normal circumstances (umask 0022), we expect a file mode of 644. If
     # the user's umask is more restrictive, respect that by masking out the
@@ -374,26 +310,6 @@ def audit_file
         EOS
       end
     end
-
-    return unless @strict
-
-    present = audit_components
-
-    present.map!(&:last)
-    if present.include?("stable block")
-      %w[url checksum mirror].each do |component|
-        if present.include?(component)
-          problem "`#{component}` should be put inside `stable block`"
-        end
-      end
-    end
-
-    if present.include?("head") && present.include?("head block")
-      problem "Should not have both `head` and `head do`"
-    end
-
-    return unless present.include?("bottle modifier") && present.include?("bottle block")
-    problem "Should not have `bottle :unneeded/:disable` and `bottle do`"
   end
 
   def audit_class

Library/Homebrew/rubocops.rb

@@ -1,2 +1,4 @@
 require_relative "./rubocops/bottle_block_cop"
 require_relative "./rubocops/formula_desc_cop"
+require_relative "./rubocops/components_order_cop"
+require_relative "./rubocops/components_redundancy_cop"

Library/Homebrew/rubocops/bottle_block_cop.rb

@@ -13,7 +13,7 @@ class CorrectBottleBlock < FormulaCop
         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

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

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