Don't ignore RuboCop messages when output written to STDERR

If RuboCop is running on a version of Ruby that is older than the
version `parser` gem is designed to parse, warnings such as:

  warning: parser/current is loading parser/ruby21, which recognizes
  warning: 2.1.7-compliant syntax, but you are running 2.1.1.

...are written to STDERR. Unfortunately, a fix for a different issue in
efa40dc causes us to not output any cop messages when these `parser`
gem warnings are emitted.

Fix this by explicitly extracting the messages separately. This required
adding support for mixing `Overcommit::Hook::Message`s with and without
file/line information (previously all messages were assumed to have
file/line information).

Author: sds

CHANGELOG.md

@@ -8,6 +8,8 @@
 * Add `--without-color` flag to `RailsBestPractices` pre-commit hook
   to fix parsing issues due to color escape sequences
 * Improve error message when `gemfile` has not had a dependency installed
+* Fix `RuboCop` pre-commit hook to not swallow cop messages when `parser` gem
+  warnings are output to STDERR
 
 ## 0.30.0
 

lib/overcommit/hook/pre_commit/base.rb

@@ -40,14 +40,17 @@ def extract_messages(output_messages, regex, type_categorizer = nil)
     end
 
     def extract_file(match, message)
-      if match[:file].nil? || match[:file].empty?
+      return unless match.names.include?('file')
+
+      if match[:file].to_s.empty?
         raise "Unexpected output: no file found in '#{message}'"
       end
 
       match[:file]
     end
 
     def extract_line(match, message)
+      return unless match.names.include?('line')
       Integer(match[:line])
     rescue ArgumentError, TypeError
       raise "Unexpected output: invalid line number found in '#{message}'"

lib/overcommit/hook/pre_commit/rubo_cop.rb

@@ -3,20 +3,31 @@ module Overcommit::Hook::PreCommit
   #
   # @see http://batsov.com/rubocop/
   class RuboCop < Base
-    MESSAGE_TYPE_CATEGORIZER = lambda do |type|
+    GENERIC_MESSAGE_TYPE_CATEGORIZER = lambda do |type|
+      type =~ /^warn/ ? :warning : :error
+    end
+
+    COP_MESSAGE_TYPE_CATEGORIZER = lambda do |type|
       type.include?('W') ? :warning : :error
     end
 
     def run
       result = execute(command, args: applicable_files)
       return :pass if result.success?
-      return [:fail, result.stderr] unless result.stderr.empty?
 
-      extract_messages(
+      generic_messages = extract_messages(
+        result.stderr.split("\n"),
+        /^(?<type>[a-z]+)/i,
+        GENERIC_MESSAGE_TYPE_CATEGORIZER,
+      )
+
+      cop_messages = extract_messages(
         result.stdout.split("\n"),
         /^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]+ (?<type>[^ ]+)/,
-        MESSAGE_TYPE_CATEGORIZER,
+        COP_MESSAGE_TYPE_CATEGORIZER,
       )
+
+      generic_messages + cop_messages
     end
   end
 end

lib/overcommit/message_processor.rb

@@ -12,6 +12,8 @@ class MessageProcessor
     WARNINGS_MODIFIED_HEADER = 'Warnings on modified lines:'.freeze
     ERRORS_UNMODIFIED_HEADER = "Errors on lines you didn't modify:".freeze
     WARNINGS_UNMODIFIED_HEADER = "Warnings on lines you didn't modify:".freeze
+    ERRORS_GENERIC_HEADER = 'Errors:'.freeze
+    WARNINGS_GENERIC_HEADER = 'Warnings:'.freeze
 
     # @param hook [Overcommit::Hook::Base]
     # @param unmodified_lines_setting [String] how to treat messages on
@@ -42,10 +44,19 @@ def hook_result(messages)
     def handle_modified_lines(messages, status)
       messages = remove_ignored_messages(messages)
 
-      messages_on_modified_lines, messages_on_unmodified_lines =
-        messages.partition { |message| message_on_modified_line?(message) }
+      messages_with_line, generic_messages = messages.partition(&:line)
 
+      # Always print generic messages first
       output = print_messages(
+        generic_messages,
+        ERRORS_GENERIC_HEADER,
+        WARNINGS_GENERIC_HEADER
+      )
+
+      messages_on_modified_lines, messages_on_unmodified_lines =
+        messages_with_line.partition { |message| message_on_modified_line?(message) }
+
+      output += print_messages(
         messages_on_modified_lines,
         ERRORS_MODIFIED_HEADER,
         WARNINGS_MODIFIED_HEADER
@@ -56,7 +67,7 @@ def handle_modified_lines(messages, status)
         WARNINGS_UNMODIFIED_HEADER
       )
 
-      [transform_status(status, messages_on_modified_lines), output]
+      [transform_status(status, generic_messages + messages_on_modified_lines), output]
     end
 
     def transform_status(status, messages_on_modified_lines)

spec/overcommit/hook/pre_commit/rubo_cop_spec.rb

@@ -10,13 +10,26 @@
   end
 
   context 'when rubocop exits successfully' do
+    let(:result) { double('result') }
+
     before do
-      result = double('result')
-      result.stub(:success?).and_return(true)
+      result.stub(success?: true, stderr: '', stdout: '')
       subject.stub(:execute).and_return(result)
     end
 
     it { should pass }
+
+    context 'and it printed warnings to stderr' do
+      before do
+        result.stub(:stderr).and_return(normalize_indent(<<-MSG))
+          warning: parser/current is loading parser/ruby21, which recognizes
+          warning: 2.1.8-compliant syntax, but you are running 2.1.1.
+          warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
+        MSG
+      end
+
+      it { should pass }
+    end
   end
 
   context 'when rubocop exits unsucessfully' do
@@ -36,6 +49,18 @@
       end
 
       it { should warn }
+
+      context 'and it printed warnings to stderr' do
+        before do
+          result.stub(:stderr).and_return(normalize_indent(<<-MSG))
+            warning: parser/current is loading parser/ruby21, which recognizes
+            warning: 2.1.8-compliant syntax, but you are running 2.1.1.
+            warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
+          MSG
+        end
+
+        it { should warn }
+      end
     end
 
     context 'and it reports an error' do
@@ -47,9 +72,21 @@
       end
 
       it { should fail_hook }
+
+      context 'and it printed warnings to stderr' do
+        before do
+          result.stub(:stderr).and_return(normalize_indent(<<-MSG))
+            warning: parser/current is loading parser/ruby21, which recognizes
+            warning: 2.1.8-compliant syntax, but you are running 2.1.1.
+            warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
+          MSG
+        end
+
+        it { should fail_hook }
+      end
     end
 
-    context 'when there is an error running rubocop' do
+    context 'when a generic error message is written to stderr' do
       before do
         result.stub(:stdout).and_return('')
         result.stub(:stderr).and_return([

spec/overcommit/message_processor_spec.rb

@@ -6,6 +6,8 @@
   WMH = Overcommit::MessageProcessor::WARNINGS_MODIFIED_HEADER + "\n"
   EUH = Overcommit::MessageProcessor::ERRORS_UNMODIFIED_HEADER + "\n"
   WUH = Overcommit::MessageProcessor::WARNINGS_UNMODIFIED_HEADER + "\n"
+  EGH = Overcommit::MessageProcessor::ERRORS_GENERIC_HEADER + "\n"
+  WGH = Overcommit::MessageProcessor::WARNINGS_GENERIC_HEADER + "\n"
 
   let(:config) { double('config') }
   let(:context) { double('context') }
@@ -27,11 +29,11 @@
       end
     end
 
-    def error(file, line)
+    def error(file = nil, line = nil)
       Overcommit::Hook::Message.new(:error, file, line, 'Error')
     end
 
-    def warning(file, line)
+    def warning(file = nil, line = nil)
       Overcommit::Hook::Message.new(:warning, file, line, 'Warning')
     end
 
@@ -209,5 +211,80 @@ def warning(file, line)
         it { should == [:fail, "#{EMH}Error\n#{WMH}Warning\n"] }
       end
     end
+
+    context 'when there are generic errors' do
+      let(:messages) { [error] * 2 }
+      let(:setting) { 'report' }
+
+      it { should == [:fail, "Error\nError\n"] }
+    end
+
+    context 'when there are generic warnings' do
+      let(:messages) { [warning] * 2 }
+      let(:setting) { 'report' }
+
+      it { should == [:warn, "Warning\nWarning\n"] }
+    end
+
+    context 'when there are generic errors and warnings' do
+      let(:messages) { [warning, error] * 2 }
+      let(:setting) { 'report' }
+
+      it { should == [:fail, "Warning\nError\nWarning\nError\n"] }
+    end
+
+    context 'when there are errors and warnings on modified/unmodified lines' do
+      let(:modified_lines) { { 'a.txt' => [2], 'b.txt' => [3, 4] } }
+      let(:setting) { 'report' }
+
+      let(:messages) do
+        [
+          warning('a.txt', 3),
+          warning('b.txt', 3),
+          error('a.txt', 2),
+          error('b.txt', 5),
+        ]
+      end
+
+      context 'and there are generic errors before them' do
+        let(:messages) { [error] * 2 + super() }
+
+        it do
+          should == [:fail, "#{EGH}Error\nError\n" \
+                            "#{EMH}Error\n#{WMH}Warning\n" \
+                            "#{EUH}Error\n#{WUH}Warning\n"]
+        end
+      end
+
+      context 'and there are generic warnings before them' do
+        let(:messages) { [warning] * 2 + super() }
+
+        it do
+          should == [:fail, "#{WGH}Warning\nWarning\n" \
+                            "#{EMH}Error\n#{WMH}Warning\n" \
+                            "#{EUH}Error\n#{WUH}Warning\n"]
+        end
+      end
+
+      context 'and there are generic errors after them' do
+        let(:messages) { super() + [error] * 2 }
+
+        it do
+          should == [:fail, "#{EGH}Error\nError\n" \
+                            "#{EMH}Error\n#{WMH}Warning\n" \
+                            "#{EUH}Error\n#{WUH}Warning\n"]
+        end
+      end
+
+      context 'and there are generic warnings after them' do
+        let(:messages) { super() + [warning] * 2 }
+
+        it do
+          should == [:fail, "#{WGH}Warning\nWarning\n" \
+                            "#{EMH}Error\n#{WMH}Warning\n" \
+                            "#{EUH}Error\n#{WUH}Warning\n"]
+        end
+      end
+    end
   end
 end