Rubocop hook swallows errors if result.stderr is present

[cank]

I have ruby 2.1.1 installed, which causes Rubocop, via parser, to complain with this:

warning: parser/current is loading parser/ruby21, which recognizes
warning: 2.1.7-compliant syntax, but you are running 2.1.1.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

This is just a warning though, Rubocop works fine.

Since updating overcommit from 0.29.1 to 0.30, I am now seeing that Rubocop messages are no longer being output, likely due to this addition in lib/overcommit/hook/pre_commit/rubo_cop.rb:

    def run
      result = execute(command, args: applicable_files)
      return :pass if result.success?
      return [:fail, result.stderr] unless result.stderr.empty?

      extract_messages(
        result.stdout.split("\n"),
        /^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]+ (?<type>[^ ]+)/,
        MESSAGE_TYPE_CATEGORIZER,
      )
    end

The message block never gets hit because stderr.empty? returns true. This flow makes sense, but unfortunately Rubocop seems to be populating stderr even though it can continue working. This return on fail was not present in 0.29.1. I imagine there are good reasons for adding this return, but could it be moved to after the extract_messages block? Barring that, is there somewhere in the documentation we can add a known issue/workaround?

I can work around this by running ruby -W0 -S rubocop, but this issue likely going to crop up for other people as well. Lots of people complain in the Rubocop project about these parser warnings, for example: rubocop/rubocop#1819

[jawshooah]

To clarify, stderr.empty? actually returns false in this case, which prevents us from parsing RuboCop's own warnings.

Here's a straightforward fix:

    def run
      result = execute(command, args: applicable_files)
      return :pass if result.success?

      messages = extract_messages(
        result.stdout.split("\n"),
        /^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]+ (?<type>[^ ]+)/,
        MESSAGE_TYPE_CATEGORIZER,
      )

      return messages if messages.any?
      return [:fail, result.stderr.chomp] unless result.stderr.empty?
      :pass
    end

The above would work, but with the side effect of ignoring result.stderr if any messages have been parsed from result.stdout.

An alternative would be to check specifically for the parser warning in result.stderr and selectively ignore it. @sds, what do you think?

[sds]

Thanks for the report, @cank!

Sorry for the delay in response here. Yes, this is definitely a problem and it's going to get solved. I've been mulling over just using @jawshooah's suggested solution, but as he already points out this would ignore all output RuboCop sends to STDERR, which will probably bite us in a different way down the road.

Thus I started looking into what would be involved to have Overcommit provide some helpers (similar to extract_messages) that allow you to easily collect output from a stream and return Overcommit::Hook::Message structs without file/line information that can be combined with other Overcommit::Hook::Messages which do contain file/line information.

Right now this can't be done since we assume all Overcommit::Hook::Messages have file/line information, so I hope to address that while at the same time addressing this issue.

[sds]

Fixed in ebb4a53. We still assume that an exit status of zero indicates success (and so we don't show any warnings in that case), but now we show the warnings in addition to any cop messages, which should be useful.