ruby-lsp icon indicating copy to clipboard operation
ruby-lsp copied to clipboard

Inform users of failing cops

Open vinistock opened this issue 3 years ago • 5 comments

When running RuboCop on files for both diagnostics and formatting, a bug may cause a cop to fail. The default behaviour is for RuboCop to print to stderr that a specific check failed and continue running without that particular cop.

While both diagnostics and formatting will still work (just ignoring the failing cop), we may want to display a message to users to let them know that the cop is failing to process the file they're currently working on.

vinistock avatar Aug 15 '22 14:08 vinistock

Do you know how frequent this happens? And is there a way to reproduce this or would this be for a general catch-all for bugged failing cop cases?

I'm asking because depending on how frequent this failure happens, we may not want to inform users through a pop message (such as the one in this: https://github.com/Shopify/ruby-lsp/issues/249) since it'd be spammy.

wildmaples avatar Aug 29 '22 14:08 wildmaples

It shouldn't be very frequent, since this happening means there's a bug in the cop's implementation. This would be a catch-all just to inform the user.

The default behaviour is for RuboCop to just print to stderr that a cop has failed. You can reproduce by overriding some cop's method and forcing it to raise every time. Something like

class SomeCop
  def on_send(node)
    raise "fake failure!"
  end
end

Some ideas on how to handle it

  • If RuboCop accepts an option to raise when a cop is failing (instead of silently printing to stderr), then let's use that flag and rescue the error. Similar to what we do for other RuboCop errors
  • Look deeper into RuboCop's implementation to understand if we can gather this information programatically (maybe it's stored in an instance variable somewhere)
  • The worst option is capturing stderr and inspecting it. If this is the only alternative, we might as well decide not to pursue this, since we probably don't want to parse stderr messages to figure out which cop failed

vinistock avatar Aug 29 '22 16:08 vinistock

Thanks for the extra context @vinistock, it helped direct my investigation into this.

We got several options on how to implement this:

  1. Use option[:raise_error]. Unfortunately, this option isn't a user-facing option that RuboCop::Options parses. I'm not sure if it is something that's overlooked but I can't find it the list of command line options. I've created an issue on rubocop/Rubocop for this. Even if we did bypass the options validator, we'd only get an StandardError, which doesn't contain the cop information.

  2. Use RuboCop::Runner instance variable errors to create a message for each error in the array.

  3. Monkey patch Commissioner#with_cop_error_handling (or some other method lol) to raise with ErrorWithAnalyzedFileLocation.new(cause: e, node: node, cop: cop). That way, we can rescue it in the Handler and show message with the appropriate cop information. This is not ideal and a workaround to the issue I've described in potential solution 1.

Ideally, the Rubocop team would be open to implementing the feature/option I've proposed. If that happens, all we need to do is turn on the raise-errors option, and then rescue it in the handler, like we did here.

Since that isn't an option (pun intended) right now, I think solution number 2 will do.

The potential drawback is that this error array will contain all the errors for multiple files in the run. But I believe the Ruby LSP only checks one file per run so it isn't a concern. Plus, as Vini mentions, we do not expect this error array to be large so it won't be spammy. (Prototype solution here)

wildmaples avatar Aug 29 '22 23:08 wildmaples

Let's put this on pause and see if they're open to raising a new custom error for this. The current options are brittle or hacky and if we can get first class support for it, it'll be better. Thanks for looking into it!

vinistock avatar Aug 30 '22 13:08 vinistock

With the hopes of moving this issue forward, I've put up a PR on Rubocop for the proposed option: https://github.com/rubocop/rubocop/issues/10977

wildmaples avatar Sep 13 '22 15:09 wildmaples

Update: The latest release of Rubocop v1.38 contains the fix we need to resolve this issue: https://github.com/rubocop/rubocop/pull/11001

wildmaples avatar Nov 03 '22 21:11 wildmaples

I can take a look at this now that the RuboCop fix is in (since v1.38.0).

andyw8 avatar Jan 12 '23 21:01 andyw8