Merge pull request #2662 from GauthamGoli/audit_text_rubocop

audit: Port audit_text method to rubocop and add tests

7 files changed, 455 insertions, 96 deletions

diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml
index 66e71027e..8fd64cb1a 100644
--- a/Library/.rubocop.yml
+++ b/Library/.rubocop.yml
@@ -8,6 +8,9 @@ AllCops:
 
 require: ./Homebrew/rubocops.rb
 
+FormulaAudit/Text:
+  Enabled: true
+
 FormulaAuditStrict/BottleBlock:
   Enabled: true
 
diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb
index d1665ea6f..4ceff0f3d 100644
--- a/Library/Homebrew/dev-cmd/audit.rb
+++ b/Library/Homebrew/dev-cmd/audit.rb
@@ -850,14 +850,6 @@ class FormulaAuditor
   end
 
   def audit_text
-    if text =~ /system\s+['"]scons/
-      problem "use \"scons *args\" instead of \"system 'scons', *args\""
-    end
-
-    if text =~ /system\s+['"]xcodebuild/
-      problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args")
-    end
-
     bin_names = Set.new
     bin_names << formula.name
     bin_names += formula.aliases
@@ -872,34 +864,6 @@ class FormulaAuditor
         end
       end
     end
-
-    if text =~ /xcodebuild[ (]*["'*]*/ && !text.include?("SYMROOT=")
-      problem 'xcodebuild should be passed an explicit "SYMROOT"'
-    end
-
-    if text.include? "Formula.factory("
-      problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\""
-    end
-
-    if text.include?("def plist") && !text.include?("plist_options")
-      problem "Please set plist_options when using a formula-defined plist."
-    end
-
-    if text =~ /depends_on\s+['"]openssl['"]/ && text =~ /depends_on\s+['"]libressl['"]/
-      problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)."
-    end
-
-    if text =~ /virtualenv_(create|install_with_resources)/ &&
-       text =~ /resource\s+['"]setuptools['"]\s+do/
-      problem "Formulae using virtualenvs do not need a `setuptools` resource."
-    end
-
-    if text =~ /system\s+['"]go['"],\s+['"]get['"]/
-      problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s."
-    end
-
-    return unless text.include?('require "language/go"') && !text.include?("go_resource")
-    problem "require \"language/go\" is unnecessary unless using `go_resource`s"
   end
 
   def audit_lines
diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb
index 587bf4b20..8627d2c04 100644
--- a/Library/Homebrew/rubocops.rb
+++ b/Library/Homebrew/rubocops.rb
@@ -3,3 +3,4 @@ require_relative "./rubocops/formula_desc_cop"
 require_relative "./rubocops/components_order_cop"
 require_relative "./rubocops/components_redundancy_cop"
 require_relative "./rubocops/homepage_cop"
+require_relative "./rubocops/text_cop"
diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb
index 3f70086b3..d9940a037 100644
--- a/Library/Homebrew/rubocops/extend/formula_cop.rb
+++ b/Library/Homebrew/rubocops/extend/formula_cop.rb
@@ -1,3 +1,5 @@
+require "parser/current"
+
 module RuboCop
   module Cop
     class FormulaCop < Cop
@@ -7,11 +9,11 @@ module RuboCop
       def on_class(node)
         file_path = processed_source.buffer.name
         return unless file_path_allowed?(file_path)
-        class_node, parent_class_node, body = *node
-        return unless formula_class?(parent_class_node)
+        return unless formula_class?(node)
         return unless respond_to?(:audit_formula)
+        class_node, parent_class_node, @body = *node
         @formula_name = class_name(class_node)
-        audit_formula(node, class_node, parent_class_node, body)
+        audit_formula(node, class_node, parent_class_node, @body)
       end
 
       # Checks for regex match of pattern in the node and
@@ -46,12 +48,100 @@ module RuboCop
         nil
       end
 
-      # Returns an array of method call nodes matching method_name inside node
+      # Set the given node as the offending node when required in custom cops
+      def offending_node(node)
+        @offensive_node = node
+        @offense_source_range = node.source_range
+      end
+
+      # Returns an array of method call nodes matching method_name inside node with depth first order (Children nodes)
       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 an array of method call nodes matching method_name in every descendant of node
+      def find_every_method_call_by_name(node, method_name)
+        return if node.nil?
+        node.each_descendant(:send).select { |method_node| method_name == method_node.method_name }
+      end
+
+      # Given a method_name and arguments, yields to a block with
+      # matching method passed as a parameter to the block
+      def find_method_with_args(node, method_name, *args)
+        methods = find_every_method_call_by_name(node, method_name)
+        methods.each do |method|
+          next unless parameters_passed?(method, *args)
+          yield method
+        end
+      end
+
+      # Matches a method with a receiver,
+      # EX: to match `Formula.factory(name)`
+      # call `find_instance_method_call(node, "Formula", :factory)`
+      # yields to a block with matching method node
+      def find_instance_method_call(node, instance, method_name)
+        methods = find_every_method_call_by_name(node, method_name)
+        methods.each do |method|
+          next unless method.receiver.const_name == instance
+          @offense_source_range = method.source_range
+          @offensive_node = method
+          yield method
+        end
+      end
+
+      # Returns nil if does not depend on dependency_name
+      # args: node - dependency_name - dependency's name
+      def depends_on?(dependency_name)
+        dependency_nodes = find_every_method_call_by_name(@body, :depends_on)
+        idx = dependency_nodes.index do |n|
+          depends_on_name_type?(n, dependency_name, :required) ||
+            depends_on_name_type?(n, dependency_name, :build) ||
+            depends_on_name_type?(n, dependency_name, :optional) ||
+            depends_on_name_type?(n, dependency_name, :recommended) ||
+            depends_on_name_type?(n, dependency_name, :run)
+        end
+        return if idx.nil?
+        @offense_source_range = dependency_nodes[idx].source_range
+        @offensive_node = dependency_nodes[idx]
+      end
+
+      # Returns true if given dependency name and dependency type exist in given dependency method call node
+      # TODO: Add case where key of hash is an array
+      def depends_on_name_type?(node, name = nil, type = :required)
+        if name
+          name_match = false
+        else
+          name_match = true # Match only by type when name is nil
+        end
+
+        case type
+        when :required
+          type_match = !node.method_args.nil? && node.method_args.first.str_type?
+          if type_match && !name_match
+            name_match = node_equals?(node.method_args.first, name)
+          end
+        when :build, :optional, :recommended, :run
+          type_match = !node.method_args.nil? &&
+                       node.method_args.first.hash_type? &&
+                       node.method_args.first.values.first.children.first == type
+          if type_match && !name_match
+            name_match = node_equals?(node.method_args.first.keys.first.children.first, name)
+          end
+        end
+
+        if type_match || name_match
+          @offensive_node = node
+          @offense_source_range = node.source_range
+        end
+        type_match && name_match
+      end
+
+      # To compare node with appropriate Ruby variable
+      def node_equals?(node, var)
+        node == Parser::CurrentRuby.parse(var.inspect)
+      end
+
       # Returns a block named block_name inside node
       def find_block(node, block_name)
         return if node.nil?
@@ -112,6 +202,17 @@ module RuboCop
         false
       end
 
+      # Check if method_name is called among every descendant node of given node
+      def method_called_ever?(node, method_name)
+        node.each_descendant(: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|
@@ -138,6 +239,24 @@ module RuboCop
         method_node.method_args
       end
 
+      # Returns true if the given parameters are present in method call
+      # and sets the method call as the offending node
+      # params can be string, symbol, array, hash, matching regex
+      def parameters_passed?(method_node, *params)
+        method_params = parameters(method_node)
+        @offensive_node = method_node
+        @offense_source_range = method_node.source_range
+        params.all? do |given_param|
+          method_params.any? do |method_param|
+            if given_param.class == Regexp
+              regex_match_group(method_param, given_param)
+            else
+              node_equals?(method_param, given_param)
+            end
+          end
+        end
+      end
+
       # Returns the begin position of the node's line in source code
       def line_start_column(node)
         node.source_range.source_buffer.line_range(node.loc.line).begin_pos
@@ -180,10 +299,16 @@ module RuboCop
         node.source_range.source_buffer
       end
 
-      # Returns the string representation if node is of type str(plain) or dstr(interpolated)
+      # Returns the string representation if node is of type str(plain) or dstr(interpolated) or const
       def string_content(node)
-        return node.str_content if node.type == :str
-        node.each_child_node(:str).map(&:str_content).join("") if node.type == :dstr
+        case node.type
+        when :str
+          return node.str_content if node.type == :str
+        when :dstr
+          return node.each_child_node(:str).map(&:str_content).join("") if node.type == :dstr
+        when :const
+          return node.const_name if node.type == :const
+        end
       end
 
       # Returns printable component name
@@ -198,8 +323,9 @@ module RuboCop
 
       private
 
-      def formula_class?(parent_class_node)
-        parent_class_node && parent_class_node.const_name == "Formula"
+      def formula_class?(node)
+        _, class_node, = *node
+        class_node && string_content(class_node) == "Formula"
       end
 
       def file_path_allowed?(file_path)
diff --git a/Library/Homebrew/rubocops/text_cop.rb b/Library/Homebrew/rubocops/text_cop.rb
new file mode 100644
index 000000000..d56c9bf46
--- /dev/null
+++ b/Library/Homebrew/rubocops/text_cop.rb
@@ -0,0 +1,55 @@
+require_relative "./extend/formula_cop"
+
+module RuboCop
+  module Cop
+    module FormulaAudit
+      class Text < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          if !find_node_method_by_name(body_node, :plist_options) &&
+             find_method_def(body_node, :plist)
+            problem "Please set plist_options when using a formula-defined plist."
+          end
+
+          if depends_on?("openssl") && depends_on?("libressl")
+            problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)."
+          end
+
+          if method_called_ever?(body_node, :virtualenv_create) ||
+             method_called_ever?(body_node, :virtualenv_install_with_resources)
+            find_method_with_args(body_node, :resource, "setuptools") do
+              problem "Formulae using virtualenvs do not need a `setuptools` resource."
+            end
+          end
+
+          unless method_called_ever?(body_node, :go_resource)
+            # processed_source.ast is passed instead of body_node because `require` would be outside body_node
+            find_method_with_args(processed_source.ast, :require, "language/go") do
+              problem "require \"language/go\" is unnecessary unless using `go_resource`s"
+            end
+          end
+
+          find_instance_method_call(body_node, "Formula", :factory) do
+            problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\""
+          end
+
+          find_every_method_call_by_name(body_node, :xcodebuild).each do |m|
+            next if parameters_passed?(m, /SYMROOT=/)
+            problem 'xcodebuild should be passed an explicit "SYMROOT"'
+          end
+
+          find_method_with_args(body_node, :system, "xcodebuild") do
+            problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args")
+          end
+
+          find_method_with_args(body_node, :system, "scons") do
+            problem "use \"scons *args\" instead of \"system 'scons', *args\""
+          end
+
+          find_method_with_args(body_node, :system, "go", "get") do
+            problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s."
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb
index 97cc0f152..b90a21b55 100644
--- a/Library/Homebrew/test/dev-cmd/audit_spec.rb
+++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb
@@ -385,57 +385,6 @@ describe FormulaAuditor do
     end
   end
 
-  describe "#audit_text" do
-    specify "xcodebuild suggests symroot" do
-      fa = formula_auditor "foo", <<-EOS.undent
-        class Foo < Formula
-          url "http://example.com/foo-1.0.tgz"
-          homepage "http://example.com"
-
-          def install
-            xcodebuild "-project", "meow.xcodeproject"
-          end
-        end
-      EOS
-
-      fa.audit_text
-      expect(fa.problems.first)
-        .to match('xcodebuild should be passed an explicit "SYMROOT"')
-    end
-
-    specify "bare xcodebuild also suggests symroot" do
-      fa = formula_auditor "foo", <<-EOS.undent
-        class Foo < Formula
-          url "http://example.com/foo-1.0.tgz"
-          homepage "http://example.com"
-
-          def install
-            xcodebuild
-          end
-        end
-      EOS
-
-      fa.audit_text
-      expect(fa.problems.first)
-        .to match('xcodebuild should be passed an explicit "SYMROOT"')
-    end
-
-    specify "disallow go get usage" do
-      fa = formula_auditor "foo", <<-EOS.undent
-        class Foo <Formula
-          url "http://example.com/foo-1.0.tgz"
-
-          def install
-            system "go", "get", "bar"
-          end
-        end
-      EOS
-      fa.audit_text
-      expect(fa.problems.first)
-        .to match("Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s.")
-    end
-  end
-
   describe "#audit_revision_and_version_scheme" do
     let(:origin_tap_path) { Tap::TAP_DIRECTORY/"homebrew/homebrew-foo" }
     let(:formula_subpath) { "Formula/foo#{@foo_version}.rb" }
diff --git a/Library/Homebrew/test/rubocops/text_cop_spec.rb b/Library/Homebrew/test/rubocops/text_cop_spec.rb
new file mode 100644
index 000000000..b218e9c25
--- /dev/null
+++ b/Library/Homebrew/test/rubocops/text_cop_spec.rb
@@ -0,0 +1,261 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/text_cop"
+
+describe RuboCop::Cop::FormulaAudit::Text do
+  subject(:cop) { described_class.new }
+
+  context "When auditing formula text" do
+    it "When xcodebuild is called without SYMROOT" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            xcodebuild "-project", "meow.xcodeproject"
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "xcodebuild should be passed an explicit \"SYMROOT\"",
+                              severity: :convention,
+                              line: 6,
+                              column: 4,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When xcodebuild is called without any args" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            xcodebuild
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "xcodebuild should be passed an explicit \"SYMROOT\"",
+                              severity: :convention,
+                              line: 6,
+                              column: 4,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When go get is executed" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            system "go", "get", "bar"
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s.",
+                              severity: :convention,
+                              line: 6,
+                              column: 4,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When xcodebuild is executed" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            system "xcodebuild", "foo", "bar"
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "use \"xcodebuild *args\" instead of \"system 'xcodebuild', *args\"",
+                              severity: :convention,
+                              line: 6,
+                              column: 4,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When scons is executed" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            system "scons", "foo", "bar"
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "use \"scons *args\" instead of \"system 'scons', *args\"",
+                              severity: :convention,
+                              line: 6,
+                              column: 4,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When plist_options are not defined when using a formula-defined plist" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            system "xcodebuild", "foo", "bar"
+          end
+
+          def plist; <<-EOS.undent
+            <?xml version="1.0" encoding="UTF-8"?>
+            <!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+            <plist version="1.0">
+            <dict>
+              <key>Label</key>
+              <string>org.nrpe.agent</string>
+            </dict>
+            </plist>
+            \EOS
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "Please set plist_options when using a formula-defined plist.",
+                              severity: :convention,
+                              line: 9,
+                              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 language/go is require'd" do
+      source = <<-EOS.undent
+        require "language/go"
+
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            system "go", "get", "bar"
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "require \"language/go\" is unnecessary unless using `go_resource`s",
+                              severity: :convention,
+                              line: 1,
+                              column: 0,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When formula uses virtualenv and also `setuptools` resource" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          resource "setuptools" do
+            url "https://foo.com/foo.tar.gz"
+            sha256 "db0904a28253cfe53e7dedc765c71596f3c53bb8a866ae50123320ec1a7b73fd"
+          end
+
+          def install
+            virtualenv_create(libexec)
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "Formulae using virtualenvs do not need a `setuptools` resource.",
+                              severity: :convention,
+                              line: 5,
+                              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 Formula.factory(name) is used" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url "http://example.com/foo-1.0.tgz"
+          homepage "http://example.com"
+
+          def install
+            Formula.factory(name)
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"",
+                              severity: :convention,
+                              line: 6,
+                              column: 4,
+                              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