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>

8 files changed, 250 insertions, 73 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
diff --git a/Library/Homebrew/diagnostic.rb b/Library/Homebrew/diagnostic.rb
index c0ecf018e..a048f0761 100644
--- a/Library/Homebrew/diagnostic.rb
+++ b/Library/Homebrew/diagnostic.rb
@@ -85,21 +85,6 @@ module Homebrew
       end
       ############# END HELPERS
 
-      def inject_dump_stats!
-        self.extend Module.new {
-          def send(method, *)
-            time = Time.now
-            super
-          ensure
-            $times[method] = Time.now - time
-          end
-        }
-        $times = {}
-        at_exit do
-          puts $times.sort_by { |_k, v| v }.map { |k, v| "#{k}: #{v}" }
-        end
-      end
-
       # See https://github.com/Homebrew/homebrew/pull/9986
       def check_path_for_trailing_slashes
         all_paths = ENV["PATH"].split(File::PATH_SEPARATOR)
diff --git a/Library/Homebrew/utils.rb b/Library/Homebrew/utils.rb
index a6920a336..d1d7b540e 100644
--- a/Library/Homebrew/utils.rb
+++ b/Library/Homebrew/utils.rb
@@ -276,6 +276,39 @@ module Homebrew
       EOS
     end
   end
+
+  # Hash of Module => Set(method_names)
+  @@injected_dump_stat_modules = {}
+
+  def inject_dump_stats!(the_module, pattern)
+    @@injected_dump_stat_modules[the_module] ||= []
+    injected_methods = @@injected_dump_stat_modules[the_module]
+    the_module.module_eval do
+      instance_methods.grep(pattern).each do |name|
+        next if injected_methods.include? 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
+
+    if $times.nil?
+      $times = {}
+      at_exit do
+        col_width = [$times.keys.map(&:size).max + 2, 15].max
+        $times.sort_by { |_k, v| v }.each do |method, time|
+          puts format("%-*s %0.4f sec", col_width, "#{method}:", time)
+        end
+      end
+    end
+  end
 end
 
 def with_system_path
diff --git a/Library/Homebrew/utils/popen.rb b/Library/Homebrew/utils/popen.rb
index 1463df530..64dee8ddf 100644
--- a/Library/Homebrew/utils/popen.rb
+++ b/Library/Homebrew/utils/popen.rb
@@ -3,6 +3,10 @@ module Utils
     popen(args, "rb", &block)
   end
 
+  def self.popen_read_text(*args, &block)
+    popen(args, "r", &block)
+  end
+
   def self.popen_write(*args, &block)
     popen(args, "wb", &block)
   end
diff --git a/share/doc/homebrew/brew.1.html b/share/doc/homebrew/brew.1.html
index ed7957c01..2a790aaf6 100644
--- a/share/doc/homebrew/brew.1.html
+++ b/share/doc/homebrew/brew.1.html
@@ -35,17 +35,20 @@ If no search term is given, all locally available formulae are listed.</p></dd>
 <h2 id="COMMANDS">COMMANDS</h2>
 
 <dl>
-<dt><code>audit</code> [<code>--strict</code>] [<code>--online</code>] [<var>formulae</var>]</dt><dd><p>Check <var>formulae</var> for Homebrew coding style violations. This should be
+<dt><code>audit</code> [<code>--strict</code>] [<code>--online</code>] [<code>--display-cop-names</code>] [<var>formulae</var>]</dt><dd><p>Check <var>formulae</var> for Homebrew coding style violations. This should be
 run before submitting a new formula.</p>
 
 <p>If no <var>formulae</var> are provided, all of them are checked.</p>
 
-<p>If <code>--strict</code> is passed, additional checks are run. This should be used
-when creating for new formulae.</p>
+<p>If <code>--strict</code> is passed, additional checks are run, including RuboCop
+style checks. This should be used when creating new formulae.</p>
 
 <p>If <code>--online</code> is passed, additional slower checks that require a network
 connection are run. This should be used when creating for new formulae.</p>
 
+<p>If <code>--display-cop-names</code> is passed, the RuboCop cop name for each violation
+is included in the output.</p>
+
 <p><code>audit</code> exits with a non-zero status if any errors are found. This is useful,
 for instance, for implementing pre-commit hooks.</p></dd>
 <dt><code>cat</code> <var>formula</var></dt><dd><p>Display the source to <var>formula</var>.</p></dd>
@@ -289,6 +292,23 @@ Homebrew build logic to help your <code>./configure &amp;&amp; make &amp;&amp; m
 or even your <code>gem install</code> succeed. Especially handy if you run Homebrew
 in an Xcode-only configuration since it adds tools like <code>make</code> to your <code>PATH</code>
 which otherwise build-systems would not find.</p></dd>
+<dt><code>style</code> [<code>--fix</code>] [<code>--display-cop-names</code>] [<var>formulae</var>|<var>files</var>]</dt><dd><p>Check formulae or files for conformance to Homebrew style guidelines.</p>
+
+<p><var>formulae</var> is a list of formula names.</p>
+
+<p><var>files</var> is a list of file names.</p>
+
+<p><var>formulae</var> and <var>files</var> may not be combined. If both are omitted, style will run
+style checks on the whole Homebrew <code>Library</code>, including core code and all
+formulae.</p>
+
+<p>If <code>--fix</code> is passed and <code>HOMEBREW_DEVELOPER</code> is set, style violations
+will be automatically fixed using RuboCop's <code>--auto-correct</code> feature.</p>
+
+<p>If <code>--display-cop-names</code> is passed, the RuboCop cop name for each violation
+is included in the output.</p>
+
+<p>Exits with a non-zero status if any style violations are found.</p></dd>
 <dt><code>switch</code> <var>name</var> <var>version</var></dt><dd><p>Symlink all of the specific <var>version</var> of <var>name</var>'s install to Homebrew prefix.</p></dd>
 <dt class="flush"><code>tap</code></dt><dd><p>List all installed taps.</p></dd>
 <dt><code>tap</code> [<code>--full</code>] <var>user</var><code>/</code><var>repo</var> [<var>URL</var>]</dt><dd><p>Tap a formula repository.</p>
diff --git a/share/man/man1/brew.1 b/share/man/man1/brew.1
index 18f92e5ac..a0462b58e 100644
--- a/share/man/man1/brew.1
+++ b/share/man/man1/brew.1
@@ -44,19 +44,22 @@ Perform a substring search of formula names for \fItext\fR\. If \fItext\fR is su
 .SH "COMMANDS"
 .
 .TP
-\fBaudit\fR [\fB\-\-strict\fR] [\fB\-\-online\fR] [\fIformulae\fR]
+\fBaudit\fR [\fB\-\-strict\fR] [\fB\-\-online\fR] [\fB\-\-display\-cop\-names\fR] [\fIformulae\fR]
 Check \fIformulae\fR for Homebrew coding style violations\. This should be run before submitting a new formula\.
 .
 .IP
 If no \fIformulae\fR are provided, all of them are checked\.
 .
 .IP
-If \fB\-\-strict\fR is passed, additional checks are run\. This should be used when creating for new formulae\.
+If \fB\-\-strict\fR is passed, additional checks are run, including RuboCop style checks\. This should be used when creating new formulae\.
 .
 .IP
 If \fB\-\-online\fR is passed, additional slower checks that require a network connection are run\. This should be used when creating for new formulae\.
 .
 .IP
+If \fB\-\-display\-cop\-names\fR is passed, the RuboCop cop name for each violation is included in the output\.
+.
+.IP
 \fBaudit\fR exits with a non\-zero status if any errors are found\. This is useful, for instance, for implementing pre\-commit hooks\.
 .
 .TP
@@ -394,6 +397,28 @@ Search for \fItext\fR in the given package manager\'s list\.
 Instantiate a Homebrew build environment\. Uses our years\-battle\-hardened Homebrew build logic to help your \fB\./configure && make && make install\fR or even your \fBgem install\fR succeed\. Especially handy if you run Homebrew in an Xcode\-only configuration since it adds tools like \fBmake\fR to your \fBPATH\fR which otherwise build\-systems would not find\.
 .
 .TP
+\fBstyle\fR [\fB\-\-fix\fR] [\fB\-\-display\-cop\-names\fR] [\fIformulae\fR|\fIfiles\fR]
+Check formulae or files for conformance to Homebrew style guidelines\.
+.
+.IP
+\fIformulae\fR is a list of formula names\.
+.
+.IP
+\fIfiles\fR is a list of file names\.
+.
+.IP
+\fIformulae\fR and \fIfiles\fR may not be combined\. If both are omitted, style will run style checks on the whole Homebrew \fBLibrary\fR, including core code and all formulae\.
+.
+.IP
+If \fB\-\-fix\fR is passed and \fBHOMEBREW_DEVELOPER\fR is set, style violations will be automatically fixed using RuboCop\'s \fB\-\-auto\-correct\fR feature\.
+.
+.IP
+If \fB\-\-display\-cop\-names\fR is passed, the RuboCop cop name for each violation is included in the output\.
+.
+.IP
+Exits with a non\-zero status if any style violations are found\.
+.
+.TP
 \fBswitch\fR \fIname\fR \fIversion\fR
 Symlink all of the specific \fIversion\fR of \fIname\fR\'s install to Homebrew prefix\.
 .