Merge pull request #8540 from reitermarkus/shellcheck-json

Split `check_style_impl` into `run_rubocop` and `run_shellcheck`.

Library/Homebrew/style.rb

 # frozen_string_literal: true
 
+require "shellwords"
+
 module Homebrew
   # Helper module for running RuboCop.
   #
@@ -20,8 +22,42 @@ module Homebrew
     end
 
     def check_style_impl(files, output_type,
-                         fix: false, except_cops: nil, only_cops: nil, display_cop_names: false,
+                         fix: false,
+                         except_cops: nil, only_cops: nil,
+                         display_cop_names: false,
                          debug: false, verbose: false)
+      raise ArgumentError, "Invalid output type: #{output_type.inspect}" unless [:print, :json].include?(output_type)
+
+      shell_files, ruby_files =
+        Array(files).map(&method(:Pathname))
+                    .partition { |f| f.realpath == HOMEBREW_BREW_FILE.realpath || f.extname == ".sh" }
+
+      rubocop_result = if shell_files.any? && ruby_files.none?
+        output_type == :json ? [] : true
+      else
+        run_rubocop(ruby_files, output_type,
+                    fix: fix,
+                    except_cops: except_cops, only_cops: only_cops,
+                    display_cop_names: display_cop_names,
+                    debug: debug, verbose: verbose)
+      end
+
+      shellcheck_result = if ruby_files.any? && shell_files.none?
+        output_type == :json ? [] : true
+      else
+        run_shellcheck(shell_files, output_type)
+      end
+
+      if output_type == :json
+        RubocopResults.new(rubocop_result + shellcheck_result)
+      else
+        rubocop_result && shellcheck_result
+      end
+    end
+
+    def run_rubocop(files, output_type,
+                    fix: false, except_cops: nil, only_cops: nil, display_cop_names: false,
+                    debug: false, verbose: false)
       Homebrew.install_bundler_gems!
       require "rubocop"
       require "rubocops"
@@ -58,11 +94,11 @@ module Homebrew
         args << "--only" << cops_to_include.join(",")
       end
 
-      has_non_formula = Array(files).any? do |file|
+      has_non_formula = files.any? do |file|
         File.expand_path(file).start_with? HOMEBREW_LIBRARY_PATH
       end
 
-      if files.present? && !has_non_formula
+      if files.any? && !has_non_formula
         config = if files.first && File.exist?("#{files.first}/spec")
           HOMEBREW_LIBRARY/".rubocop_rspec.yml"
         else
@@ -79,63 +115,98 @@ module Homebrew
 
       cache_env = { "XDG_CACHE_HOME" => "#{HOMEBREW_CACHE}/style" }
 
-      rubocop_success = false
-
       case output_type
       when :print
         args << "--debug" if debug
         args << "--format" << "simple" if files.present?
-        system(cache_env, "rubocop", *args)
-        rubocop_success = $CHILD_STATUS.success?
-      when :json
-        json, err, status =
-          Open3.capture3(cache_env, "rubocop", "--format", "json", *args)
-        # exit status of 1 just means violations were found; other numbers mean
-        # execution errors.
-        # exitstatus can also be nil if RuboCop process crashes, e.g. due to
-        # native extension problems.
-        # JSON needs to be at least 2 characters.
-        if !(0..1).cover?(status.exitstatus) || json.to_s.length < 2
-          raise "Error running `rubocop --format json #{args.join " "}`\n#{err}"
-        end
 
-        return RubocopResults.new(JSON.parse(json))
-      else
-        raise "Invalid output_type for check_style_impl: #{output_type}"
+        system cache_env, "rubocop", *args
+        $CHILD_STATUS.success?
+      when :json
+        result = system_command "rubocop", args: ["--format", "json", *args], env: cache_env
+        json = json_result!(result)
+        json["files"]
       end
+    end
 
-      return rubocop_success if files.present?
-
+    def run_shellcheck(files, output_type)
       shellcheck   = which("shellcheck")
       shellcheck ||= which("shellcheck", ENV["HOMEBREW_PATH"])
       shellcheck ||= begin
         ohai "Installing `shellcheck` for shell style checks..."
-        system HOMEBREW_BREW_FILE, "install", "shellcheck"
+        safe_system HOMEBREW_BREW_FILE, "install", "shellcheck"
         which("shellcheck") || which("shellcheck", ENV["HOMEBREW_PATH"])
       end
-      unless shellcheck
-        opoo "Could not find or install `shellcheck`! Not checking shell style."
-        return rubocop_success
-      end
-
-      shell_files = [
-        HOMEBREW_BREW_FILE,
-        *Pathname.glob("#{HOMEBREW_LIBRARY}/Homebrew/*.sh"),
-        *Pathname.glob("#{HOMEBREW_LIBRARY}/Homebrew/cmd/*.sh"),
-        *Pathname.glob("#{HOMEBREW_LIBRARY}/Homebrew/utils/*.sh"),
-      ].select(&:exist?)
-      # TODO: check, fix completions here too.
-      # TODO: consider using ShellCheck JSON output
-      shellcheck_success = system shellcheck, "--shell=bash", *shell_files
-      rubocop_success && shellcheck_success
+
+      if files.empty?
+        files = [
+          HOMEBREW_BREW_FILE,
+          # TODO: HOMEBREW_REPOSITORY/"completions/bash/brew",
+          *Pathname.glob("#{HOMEBREW_LIBRARY}/Homebrew/*.sh"),
+          *Pathname.glob("#{HOMEBREW_LIBRARY}/Homebrew/cmd/*.sh"),
+          *Pathname.glob("#{HOMEBREW_LIBRARY}/Homebrew/utils/*.sh"),
+        ]
+      end
+
+      args = ["--shell=bash", "--", *files] # TODO: Add `--enable=all` to check for more problems.
+
+      case output_type
+      when :print
+        system shellcheck, "--format=tty", *args
+        $CHILD_STATUS.success?
+      when :json
+        result = system_command shellcheck, args: ["--format=json1", *args]
+        json = json_result!(result)
+
+        # Convert to same format as RuboCop offenses.
+        json["comments"].group_by { |v| v["file"] }
+                        .map do |k, v|
+          {
+            "path"     => k,
+            "offenses" => v.map do |o|
+              o.delete("file")
+
+              o["cop_name"] = "SC#{o.delete("code")}"
+
+              level = o.delete("level")
+              o["severity"] = { "style" => "refactor", "info" => "convention" }.fetch(level, level)
+
+              line = o.delete("line")
+              column = o.delete("column")
+
+              o["corrected"] = false
+              o["correctable"] = o.delete("fix").present?
+
+              o["location"] = {
+                "start_line"   => line,
+                "start_column" => column,
+                "last_line"    => o.delete("endLine"),
+                "last_column"  => o.delete("endColumn"),
+                "line"         => line,
+                "column"       => column,
+              }
+
+              o
+            end,
+          }
+        end
+      end
+    end
+
+    def json_result!(result)
+      # An exit status of 1 just means violations were found; other numbers mean
+      # execution errors.
+      # JSON needs to be at least 2 characters.
+      result.assert_success! if !(0..1).cover?(result.status.exitstatus) || result.stdout.length < 2
+
+      JSON.parse(result.stdout)
     end
 
     # Result of a RuboCop run.
     class RubocopResults
-      def initialize(json)
-        @metadata = json["metadata"]
+      def initialize(files)
         @file_offenses = {}
-        json["files"].each do |f|
+        files.each do |f|
           next if f["offenses"].empty?
 
           file = File.realpath(f["path"])