Formatting with rubocop should only autocorrect layout offenses

[nevans]

Use case

Rubocop's more aggressive autocorrects are incredibly useful. The "unsafe" autocorrections don't run by default, and I agree that some "safe" autocorrection should probably be re-categorized as "unsafe". But rubocop has many truly safe autocorrections that I don't want to run every time I save or format a file. I do want to be able to run these auto-corrections manually. Marking these in my own rubocop.yml as unsafe or not auto-correctable would be a significant downgrade of the standard rubocop functionality.

Description

Fortunately, rubocop already provides the ability to only run safe formatting auto-corrections, instead of all safe auto-corrections: the -x (or --fix-layout) option. As an added benefit, it's much faster than running all autocorrections. ruby-lsp should use that option when it uses rubocop for formatting.

All auto-correctable offenses (whether safe or unsafe) should be made available as Code Actions.

Implementation

I believe this can be accomplished by simply changing one character on the following line:

ruby-lsp/lib/ruby_lsp/requests/support/rubocop_formatting_runner.rb

Line 20 in 6d63b6e

@runner = T.let(RuboCopRunner.new("-a"), RuboCopRunner)

should be changed to use -x instead of -a.

However, I haven't read through https://github.com/Shopify/ruby-lsp/blob/main/lib/ruby_lsp/requests/support/rubocop_diagnostic.rb (or other relevant files) yet, and I don't know if that would break the auto-correction code actions.

Originally posted by @nevans in #596 (comment)

[andyw8]

@nevans That's an interesting suggestion, but from my understanding it only runs cops within the Layout/ category. It would prevent autocorrections by cops from extensions such as rubocop-rspec or rubocop-sorbet from working.

[nevans]

@andyw8 I was in the middle of editing the description when I saw your comment.

IMO, those cops should not be run automatically on format, because they aren't formatters. The more elaborate auto-corrections are what the Code Actions are for. However... I don't know if updating that one line (to use -x instead of -a) would break any rubocop-related Code Actions. I hope not.

If there are any that should be run on format, we should consider opening a PR with rubocop to allow --fix-layout cops to be contributed from other gems. Although, I suppose there's nothing (beside convention) stopping another gem from simply placing their cops into the Layout category.

[vinistock]

Can you provide an example of a violation you wouldn't want fixed using format on save, but would want to fix manually with a quickfix?

In my opinion, having all violations fixed is a reasonable default to avoid developers accidentally pushing code that will fail linting on CI. We will not change the default, but we might consider exposing some configuration.

[nevans]

Basically, I don't like it when a single code change happens "behind my back", so to speak.

For me personally, I refuse to use auto-format-on-save for literally any changes other than whitespace (indentation, alignment, other spacing). Even automatic line-wrapping drives me nuts (because no formatter ever seems to get it right, by my standards). So rubocop -x is almost exactly what I want an auto-formatter to do.

I only ever run rubocop -a (or -A) when I've already either staged or committed all of my changes: I want to see exactly what changes were made and this makes it much easier for me to either veto changes (rubocop:disable) or make the code acceptable in a different way. E.g: some style rules can be configured to allow multiple styles, e.g: a single space or aligned with the adjacent lines, and the autocorrection doesn't always choose the "correct" spacing for the code in question. Because the "correct" spacing is a semantic distinction which is impossible for rubocop to know.

The original issue #596 gives good examples, IMO: RSpec/Focus and Lint/Debugger are both meant to catch intentionally temporary code alterations and prevent them from accidentally being committed or merged. Although it's certainly debatable whether or not they are "safe" alterations, they do match the definition of "safe" that's been used in the rubocop project for years.

And both of these violations are qualitatively different from most of the "unsafe" autocorrections that rubocop provides. E.g: I can tell at a glance that yes, oops, I forgot to delete those lines and I'd never intentionally push or merge them. But the unsafe ones always require careful thought: e.g: was that really an array or an enumerable or an active record relation, or did rubocop just get tricked into thinking it was?

In other words, I find it very useful to have all three levels of autocorrection: -x, -a, and -A.

I'm personally both more strict but also more permissive with my manual formatting than any ruby auto-formatter I've ever seen. But I've personally configured everything in my company's rubocop.yml, so I have no problem with the actual configuration. That said, some Style/... rules are frequently rubocop:disabled.

Because the examples in #596 don't fit everyone's concept of safe, here's another scenario: some of the style rules that autocorrect to simplify code can unintentionally make the code much harder for me to understand in that moment. E.g: I'm in the middle of working through a complex series of predicates and clauses, and my use of a nested ternary or a one-line if/then or the order of my if/else clauses was very intentional and matches my current thinking. But rubocop knows that it's semantically safe to restructure and reorder them. Maybe the strict application of these rules makes it clearer to someone else reading the code or maybe it doesn't, but in that moment I don't care because the sudden change confuses me and contradicts my immediate thinking about the problem... and I wasn't even finished working through that code yet. I just needed to save this intermediate form to see if it passed the tests. I may have been working towards a final form which would be accepted by rubocop, but now I need to undo those formatting changes so I can backtrack to my intermediate form.

Like I said: I've configured my company's rubocop.yml, so if I don't like a rule I'll just disable it. This is about rules that I do want to keep. But: readability by the team in the long-term and readability by the developer(s) doing the work right now, these aren't identical.

[nevans]

Sorry for typing so much... if I had more time I would have sent a shorter comment!

And, separately, so it doesn't get lost in all of that: I do hope you'll still consider changing the default some day, but a configuration option would be great. 🙂

[vinistock]

Thank you for taking the time to explain.

Based on what you said, it feels to me like you almost want a before commit hook to run formatting and not formatting in the editor. Changing the code as you're writing it is exactly what format on save, format on paste and format on type are intended for.

I'm open to adding a configuration, but it has to be generic and not RuboCop specific since we support Syntax Tree as well. We're also working on the extension system to allow other formatters and linters to hook into the LSP.

Would something like a formatting_level configuration with options of format, format_and_lint and lint be enough?

[leonid-shevtsov]

IMO the options should be layout_only, safe, and unsafe, to match Rubocop semantics.

And it's definitely a worthy configuration option, as I, for one, want the full range of autocorrections - even unsafe - on every save. :)

Or perhaps even better, fix-layout, autocorrect, and autocorrect-all, to use the command-line flags verbatim.

[rreckonerr]

It might be worth checking out how vscode-eslint approaches this feature with codeActionsOnSave settings. I think it'd be cool to align vscode-ruby-lsp with that too.

[ghost]

I would also like a configuration option, but to use --autocorrect-all instead of --autocorrect.
The layout_only/safe/unsafe option sound great to me, but maybe this could just be a string option with --autocorrect as default? Not sure if that is possible.

This would be the most flexible solution. Even an option like --disable-uncorrectable would be possible then.