brew-audit: pull style checks in to main audit output

This collects all violations for each formula in a single place, instead
of doing `brew style` outputs for all formulae first, and then the other
audit checks.

Closes #112.

Signed-off-by: Andrew Janke <andrew@apjanke.net>

3 files changed, 163 insertions, 53 deletions

diff --git a/Library/Homebrew/cmd/audit.rb b/Library/Homebrew/cmd/audit.rb
index cdf5bb9fe..eca4fbb1e 100644
--- a/Library/Homebrew/cmd/audit.rb
+++ b/Library/Homebrew/cmd/audit.rb
@@ -1,18 +1,24 @@
-#:  * `audit` [`--strict`] [`--online`] [<formulae>]:
+#:  * `audit` [`--strict`] [`--online`] [`--display-cop-names`] [<formulae>]:
 #:    Check <formulae> for Homebrew coding style violations. This should be
 #:    run before submitting a new formula.
 #:
 #:    If no <formulae> are provided, all of them are checked.
 #:
-#:    If `--strict` is passed, additional checks are run. This should be used
-#:    when creating for new formulae.
+#:    If `--strict` is passed, additional checks are run, including RuboCop
+#:    style checks. This should be used when creating new formulae.
 #:
 #:    If `--online` is passed, additional slower checks that require a network
 #:    connection are run. This should be used when creating for new formulae.
 #:
+#:    If `--display-cop-names` is passed, the RuboCop cop name for each violation
+#:    is included in the output.
+#:
 #:    `audit` exits with a non-zero status if any errors are found. This is useful,
 #:    for instance, for implementing pre-commit hooks.
 
+# Undocumented options:
+#     -D activates debugging and profiling of the audit methods (not the same as --debug)
+
 require "formula"
 require "formula_versions"
 require "utils"
@@ -20,68 +26,51 @@ require "extend/ENV"
 require "formula_cellar_checks"
 require "official_taps"
 require "cmd/search"
+require "cmd/style"
 require "date"
 
 module Homebrew
+  RUBY_2_OR_LATER = RUBY_VERSION.split(".").first.to_i >= 2
+
   def audit
+    if ARGV.switch? "D"
+      Homebrew.inject_dump_stats!(FormulaAuditor, /^audit_/)
+    end
+
     formula_count = 0
     problem_count = 0
 
     strict = ARGV.include? "--strict"
-    if strict && ARGV.resolved_formulae.any? && RUBY_VERSION.split(".").first.to_i >= 2
-      require "cmd/style"
-      ohai "brew style #{ARGV.resolved_formulae.join " "}"
-      style
-    end
-
+    style = strict && RUBY_2_OR_LATER
     online = ARGV.include? "--online"
 
     ENV.activate_extensions!
     ENV.setup_build_environment
 
-    if ARGV.switch? "D"
-      FormulaAuditor.module_eval do
-        instance_methods.grep(/audit_/).map do |name|
-          method = instance_method(name)
-          define_method(name) do |*args, &block|
-            begin
-              time = Time.now
-              method.bind(self).call(*args, &block)
-            ensure
-              $times[name] ||= 0
-              $times[name] += Time.now - time
-            end
-          end
-        end
-      end
-
-      $times = {}
-      at_exit { puts $times.sort_by { |_k, v| v }.map { |k, v| "#{k}: #{v}" } }
-    end
-
-    ff = if ARGV.named.empty?
-      Formula
+    if ARGV.named.empty?
+      ff = Formula
+      files = Tap.map(&:formula_dir)
     else
-      ARGV.resolved_formulae
+      ff = ARGV.resolved_formulae
+      files = ARGV.resolved_formulae.map(&:path)
+    end
+    if style
+      # Check style in a single batch run up front for performance
+      style_results = check_style_json(files, :realpath => true)
     end
-
-    output_header = !strict
 
     ff.each do |f|
-      fa = FormulaAuditor.new(f, :strict => strict, :online => online)
+      options = { :strict => strict, :online => online }
+      options[:style_offenses] = style_results.file_offenses(f.path) if style
+      fa = FormulaAuditor.new(f, options)
       fa.audit
 
-      unless fa.problems.empty?
-        unless output_header
-          puts
-          ohai "audit problems"
-          output_header = true
-        end
+      next if fa.problems.empty?
 
-        formula_count += 1
-        problem_count += fa.problems.size
-        puts "#{f.full_name}:", fa.problems.map { |p| " * #{p}" }, ""
-      end
+      formula_count += 1
+      problem_count += fa.problems.size
+      problem_lines = fa.problems.map { |p| "  * #{p.chomp.gsub("\n", "\n    ")}" }
+      puts "#{f.full_name}:", problem_lines
     end
 
     unless problem_count.zero?
@@ -152,11 +141,21 @@ class FormulaAuditor
     @formula = formula
     @strict = !!options[:strict]
     @online = !!options[:online]
+    # Accept precomputed style offense results, for efficiency
+    @style_offenses = options[:style_offenses]
     @problems = []
     @text = FormulaText.new(formula.path)
     @specs = %w[stable devel head].map { |s| formula.send(s) }.compact
   end
 
+  def audit_style
+    return unless @style_offenses
+    display_cop_names = ARGV.include?("--display-cop-names")
+    @style_offenses.each do |offense|
+      problem offense.to_s(:display_cop_name => display_cop_names)
+    end
+  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
@@ -983,6 +982,7 @@ class FormulaAuditor
     audit_installed
     audit_prefix_has_contents
     audit_reverse_migration
+    audit_style
   end
 
   private
diff --git a/Library/Homebrew/cmd/doctor.rb b/Library/Homebrew/cmd/doctor.rb
index 01f2f8264..d54e8238a 100644
--- a/Library/Homebrew/cmd/doctor.rb
+++ b/Library/Homebrew/cmd/doctor.rb
@@ -2,10 +2,15 @@
 #:    Check your system for potential problems. Doctor exits with a non-zero status
 #:    if any problems are found.
 
+# Undocumented options:
+#     -D activates debugging and profiling of the audit methods (not the same as --debug)
+
 require "diagnostic"
 
 module Homebrew
   def doctor
+    inject_dump_stats!(Diagnostic::Checks, /^check_*/) if ARGV.switch? "D"
+
     checks = Diagnostic::Checks.new
 
     if ARGV.include? "--list-checks"
@@ -13,7 +18,6 @@ module Homebrew
       exit
     end
 
-    checks.inject_dump_stats! if ARGV.switch? "D"
 
     if ARGV.named.empty?
       slow_checks = %w[
diff --git a/Library/Homebrew/cmd/style.rb b/Library/Homebrew/cmd/style.rb
index 20575c065..0f089b8ae 100644
--- a/Library/Homebrew/cmd/style.rb
+++ b/Library/Homebrew/cmd/style.rb
@@ -1,3 +1,25 @@
+#:  * `style` [`--fix`] [`--display-cop-names`] [<formulae>|<files>]:
+#:    Check formulae or files for conformance to Homebrew style guidelines.
+#:
+#:    <formulae> is a list of formula names.
+#:
+#:    <files> is a list of file names.
+#:
+#:    <formulae> and <files> may not be combined. If both are omitted, style will run
+#:    style checks on the whole Homebrew `Library`, including core code and all
+#:    formulae.
+#:
+#:    If `--fix` is passed and `HOMEBREW_DEVELOPER` is set, style violations
+#:    will be automatically fixed using RuboCop's `--auto-correct` feature.
+#:
+#:    If `--display-cop-names` is passed, the RuboCop cop name for each violation
+#:    is included in the output.
+#:
+#:    Exits with a non-zero status if any style violations are found.
+
+require "utils"
+require "utils/json"
+
 module Homebrew
   def style
     target = if ARGV.named.empty?
@@ -8,18 +30,102 @@ module Homebrew
       ARGV.formulae.map(&:path)
     end
 
+    Homebrew.failed = check_style_and_print(target, :fix => ARGV.flag?("--fix"))
+  end
+
+  # Checks style for a list of files, printing simple RuboCop output.
+  # Returns true if violations were found, false otherwise.
+  def check_style_and_print(files, options = {})
+    check_style_impl(files, :print, options)
+  end
+
+  # Checks style for a list of files, returning results as a RubocopResults
+  # object parsed from its JSON output.
+  def check_style_json(files, options = {})
+    check_style_impl(files, :json, options)
+  end
+
+  def check_style_impl(files, output_type, options = {})
+    fix = options[:fix]
     Homebrew.install_gem_setup_path! "rubocop", "0.39"
 
-    args = [
-      "--format", "simple", "--force-exclusion", "--config",
-      "#{HOMEBREW_LIBRARY}/.rubocop.yml",
+    args = %W[
+      --force-exclusion
+      --config #{HOMEBREW_LIBRARY}/.rubocop.yml
     ]
+    args << "--auto-correct" if ARGV.homebrew_developer? && fix
+    args += files
 
-    args << "--auto-correct" if ARGV.homebrew_developer? && ARGV.flag?("--fix")
+    case output_type
+    when :print
+      args << "--display-cop-names" if ARGV.include? "--display-cop-names"
+      system "rubocop", "--format", "simple", *args
+      !$?.success?
+    when :json
+      json = Utils.popen_read_text("rubocop", "--format", "json", *args)
+      # exit status of 1 just means violations were found; others are errors
+      raise "Error while running rubocop" if $?.exitstatus > 1
+      RubocopResults.new(Utils::JSON.load(json))
+    else
+      raise "Invalid output_type for check_style_impl: #{output_type}"
+    end
+  end
 
-    args += target
+  class RubocopResults
+    def initialize(json)
+      @metadata = json["metadata"]
+      @file_offenses = {}
+      json["files"].each do |f|
+        next if f["offenses"].empty?
+        file = File.realpath(f["path"])
+        @file_offenses[file] = f["offenses"].map { |x| RubocopOffense.new(x) }
+      end
+    end
+
+    def file_offenses(path)
+      @file_offenses[path.to_s]
+    end
+  end
 
-    system "rubocop", *args
-    Homebrew.failed = !$?.success?
+  class RubocopOffense
+    attr_reader :severity, :message, :corrected, :location, :cop_name
+
+    def initialize(json)
+      @severity = json["severity"]
+      @message = json["message"]
+      @cop_name = json["cop_name"]
+      @corrected = json["corrected"]
+      @location = RubocopLineLocation.new(json["location"])
+    end
+
+    def severity_code
+      @severity[0].upcase
+    end
+
+    def to_s(options = {})
+      if options[:display_cop_name]
+        "#{severity_code}: #{location.to_short_s}: #{cop_name}: #{message}"
+      else
+        "#{severity_code}: #{location.to_short_s}: #{message}"
+      end
+    end
+  end
+
+  class RubocopLineLocation
+    attr_reader :line, :column, :length
+
+    def initialize(json)
+      @line = json["line"]
+      @column = json["column"]
+      @length = json["length"]
+    end
+
+    def to_s
+      "#{line}: col #{column} (#{length} chars)"
+    end
+
+    def to_short_s
+      "#{line}: col #{column}"
+    end
   end
 end