Port audit_components method to rubocops and add corresponding 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 @@ class FormulaAuditor
     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 @@ class FormulaAuditor
         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 @@ 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

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

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)
+        # Returns an array of block nodes named block_name inside node
-          # Check if a method is called inside a block
+        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

Library/Homebrew/test/dev-cmd/audit_spec.rb

@@ -141,100 +141,6 @@ describe FormulaAuditor do
       fa.audit_file
       expect(fa.problems).to eq([])
     end
-
-    specify "strict: ordering issue" do
-      fa = formula_auditor "foo", <<-EOS.undent, strict: true
-        class Foo < Formula
-          url "http://example.com/foo-1.0.tgz"
-          homepage "http://example.com"
-        end
-      EOS
-
-      fa.audit_file
-      expect(fa.problems)
-        .to eq(["`homepage` (line 3) should be put before `url` (line 2)"])
-    end
-
-    specify "strict: resource placement" do
-      fa = formula_auditor "foo", <<-EOS.undent, strict: true
-        class Foo < Formula
-          url "https://example.com/foo-1.0.tgz"
-
-          resource "foo2" do
-            url "https://example.com/foo-2.0.tgz"
-          end
-
-          depends_on "openssl"
-        end
-      EOS
-
-      fa.audit_file
-      expect(fa.problems)
-        .to eq(["`depends_on` (line 8) should be put before `resource` (line 4)"])
-    end
-
-    specify "strict: plist placement" do
-      fa = formula_auditor "foo", <<-EOS.undent, strict: true
-        class Foo < Formula
-          url "https://example.com/foo-1.0.tgz"
-
-          test do
-            expect(shell_output("./dogs")).to match("Dogs are terrific")
-          end
-
-          def plist
-          end
-        end
-      EOS
-
-      fa.audit_file
-      expect(fa.problems)
-        .to eq(["`plist block` (line 8) should be put before `test block` (line 4)"])
-    end
-
-    specify "strict: url outside of stable block" do
-      fa = formula_auditor "foo", <<-EOS.undent, strict: true
-        class Foo < Formula
-          url "http://example.com/foo-1.0.tgz"
-          stable do
-            # stuff
-          end
-        end
-      EOS
-
-      fa.audit_file
-      expect(fa.problems).to eq(["`url` should be put inside `stable block`"])
-    end
-
-    specify "strict: head and head do" do
-      fa = formula_auditor "foo", <<-EOS.undent, strict: true
-        class Foo < Formula
-          head "http://example.com/foo.git"
-          head do
-            # stuff
-          end
-        end
-      EOS
-
-      fa.audit_file
-      expect(fa.problems).to eq(["Should not have both `head` and `head do`"])
-    end
-
-    specify "strict: bottle and bottle do" do
-      fa = formula_auditor "foo", <<-EOS.undent, strict: true
-        class Foo < Formula
-          url "http://example.com/foo-1.0.tgz"
-          bottle do
-            # bottles go here
-          end
-          bottle :unneeded
-        end
-      EOS
-
-      fa.audit_file
-      expect(fa.problems)
-        .to eq(["Should not have `bottle :unneeded/:disable` and `bottle do`"])
-    end
   end
 
   describe "#audit_class" do

Library/Homebrew/test/rubocops/components_order_cop_spec.rb

@@ -0,0 +1,116 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/components_order_cop"
+
+describe RuboCop::Cop::Homebrew::FormulaComponentsOrder do
+  subject(:cop) { described_class.new }
+
+  context "When auditing formula components order" do
+    it "When url precedes homepage" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+        end
+      EOS
+
+      expected_offenses = [{  message: "`homepage` (line 3) should be put before `url` (line 2)",
+                              severity: :convention,
+                              line: 3,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When `resource` precedes `depends_on`" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "https://example.com/foo-1.0.tgz"
+
+          resource "foo2" do
+            url "https://example.com/foo-2.0.tgz"
+          end
+
+          depends_on "openssl"
+        end
+      EOS
+
+      expected_offenses = [{  message: "`depends_on` (line 8) should be put before `resource` (line 4)",
+                              severity: :convention,
+                              line: 8,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When `test` precedes `plist`" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "https://example.com/foo-1.0.tgz"
+
+          test do
+            expect(shell_output("./dogs")).to match("Dogs are terrific")
+          end
+
+          def plist
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "`plist` (line 8) should be put before `test` (line 4)",
+                              severity: :convention,
+                              line: 8,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When only one of many `depends_on` precedes `conflicts_with`" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          depends_on "autoconf" => :build
+          conflicts_with "visionmedia-watch"
+          depends_on "automake" => :build
+          depends_on "libtool" => :build
+          depends_on "pkg-config" => :build
+          depends_on "gettext"
+        end
+      EOS
+
+      expected_offenses = [{  message: "`depends_on` (line 4) should be put before `conflicts_with` (line 3)",
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    def expect_offense(expected, actual)
+      expect(actual.message).to eq(expected[:message])
+      expect(actual.severity).to eq(expected[:severity])
+      expect(actual.line).to eq(expected[:line])
+      expect(actual.column).to eq(expected[:column])
+    end
+  end
+end

Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb

@@ -0,0 +1,87 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/components_redundancy_cop"
+
+describe RuboCop::Cop::Homebrew::ComponentsRedundancy do
+  subject(:cop) { described_class.new }
+
+  context "When auditing formula components common errors" do
+    it "When url outside stable block" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          stable do
+            # stuff
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "`url` should be put inside `stable` block",
+                              severity: :convention,
+                              line: 2,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When both `head` and `head do` are present" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          head "http://example.com/foo.git"
+          head do
+            # stuff
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "`head` and `head do` should not be simultaneously present",
+                              severity: :convention,
+                              line: 3,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When both `bottle :modifier` and `bottle do` are present" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          bottle do
+            # bottles go here
+          end
+          bottle :unneeded
+        end
+      EOS
+
+      expected_offenses = [{  message: "`bottle :modifier` and `bottle do` should not be simultaneously present",
+                              severity: :convention,
+                              line: 3,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    def expect_offense(expected, actual)
+      expect(actual.message).to eq(expected[:message])
+      expect(actual.severity).to eq(expected[:severity])
+      expect(actual.line).to eq(expected[:line])
+      expect(actual.column).to eq(expected[:column])
+    end
+  end
+end