Autofix Design Proposal

[sladyn98]

This discussion is in reference to this comment #4361 (comment) and this issue in general #4361. We would like to scope out the work invovled in setting up autofix for ruff. I think we can execute this in a series of steps

1. Run ruff on the entire set of files or the file given.
2. Prompt the user for the fix. (Once the fix has been applied we can either let the user apply all fixes or apply a fix and then relint
3. Relint all the files and then prompt the user again if they want to make any more changes

User Prompting

The modification or variant of apply_fixes should include the following features:

• Display the rule that has been violated.
• Show the current line with the error.
• Display the proposed fix.
• Offer the user an option to accept or decline the fix.
• Provide an option to apply all fixes at once or individually.
• In case of a declined fix, it should retain the original code and move to the next error.

We already have apply_fixes which applies a set of fixes to the ones reported. We would need to modify or write a variant that would prompt the user that this is the fix that is being applied and this is how the new line will look.

Applying fix for rule: UnusedImport
-- 'import math
'
++ ''
Applying fix for rule: TrueFalseComparison
-- 'num == True'
++ 'num is True'
Applying fix for rule: UselessSemicolon
-- ';'
++ ''
Applying fix for rule: UnusedImport
-- 'import os
'
++ ''
Ready to print diagnostics
Found 4 errors (4 fixed, 0 remaining).

I tried hacking on some part of it and you can see it in a draft PR here, it is more of a proof of concept right now. One of the things I noticed right off the bat is we would like to print the line that is being modified along with the ones that it is going to be replaced with.

Draft PR: #4936
This is something that I hacked quickly and need to modify to get some better results.

Relinting the files once a fix has been applied.

This should be fairly straightforward. One thing we need to make sure is that the fix does not introduce any more errors. If so we abort the autofix.

[sladyn98]

CC @charliermarsh @MichaReiser @evanrittenhouse @madkinsz

Would love some initial thoughts, ideas

[MichaReiser]

Thanks for writting this up and start tackling it. I left a few questions.

[evanrittenhouse]

+1, thanks for this, super helpful. Also left some questions

[MichaReiser]

Offer the user an option to accept or decline the fix.

What happens if the user declines the fix? Does it get suppressed or just ignored? Should we add an option for adding a suppression comment?

[jfmengels]

In elm-review this error (identified by IIRC the rule name + error message + file name + position in the file) gets ignored, and is reported at the end without the fix proposal.
I think it's the right thing to do because often I go through fixes and I'm like "I don't want to fix this one right now, I'll do it right after".

I think having the option to suppress it would be interesting, and for that you'd need to be able to support a multiple choice prompt. But also, suppression comments are evil and you don't want to make those too easy I'd say?

[sladyn98]

I think its fairly simple to support a multiple choice prompt and we could just present the error at the end to the user as @jfmengels suggested. IMO ignoring the fix makes sense since it could be a choice of the user at that very moment to ignore it, and might not want to ignore it in the future. The case for the user suppressing a error is a completely different aspect and might be out of scope for this implementation ?

[MichaReiser]

Display the proposed fix.

I think it would be good to tackle this as a project on its own first. The current implementation is very naive and only really intended for testing purposes. For example, the diff doesn't properly display differences that are white spaces only (changing from spaces to tabs could be hard to see), or it renders the full range, possibly throwing a 100 line diff at you rather than collapsing the change at some point.

[sladyn98]

So do we want to have this included in this scope ? Or just have a simple implementation of the fix that is displayed right now. Or do you view this as something that needs to be tackled first for this autofix to work ?

[jfmengels]

One potential way of implementing this would be to implement fix proposals with the naive/simple diff, then improve the diff, and THEN show the flag in the tool's help/documentation.

If you cut a release in between the steps, then people could try it out in a deteriorated state, but it could be okay because it'd be a preview feature?

[MichaReiser]

Provide an option to apply all fixes at once or individually.

Would this apply all fixes for a single file or for all files? How does the user know which fixes would be applied?

[jfmengels]

elm-review --fix-all does it for all files, by computing every diff in a representation of the project in memory, and then doing a diff between the original and new project. The diffs are displayed per file one file after another, with a separator.
(Reminder that you can try it out following these instructions: #3642 (comment))

Since you don't have multi-file analysis, it makes a bit more sense for Ruff to do it file-by-file, but I'd imagine this could be several flags: one for fix one by one, one for fix file by file, and one for fix all. I don't know how often the fix file by file would be used in practice 🤔
(Also, it becomes weird once you have multifile analysis?).

[sladyn98]

I think we could have a flag that displays the fixes one by one and a flag for file by file. And by one by one I mean it could go display the name of the file the change is being made to and then the diff. So when a particular file is done it could then move on to the next file. In contrast the fix-all would just apply all the fixes without prompting like it does now.

[MichaReiser]

Relinting the files once a fix has been applied.

Do you propose to review all files first and then re-lint or re-lint on a per-file level (applied all fixes for a file, re-lint that file)?

[jfmengels]

Probably relevant article of mine: https://jfmengels.net/much-faster-fixes/

If you're fixing one-by-one, then you can/should stop as soon as you find your first applicable fix. The user could say no to that one, so you might want to finish analyzing the entire file, otherwise you'd have to analyze it from scratch. If you do that, you can have a backlog of fixable errors to prompt for, and apply the first one the user says yes to. If they say no to all, then you just continue the analysis from the next file on. If you review files in parallel you can probably have a slightly bigger backlog, or even review the rest of the project while the user is being prompted (that'd be pretty dope).

[sladyn98]

I think @jfmengels makes a lot of sense here, I was thinking along the lines of once the user finishes applying all the possible fixes he wants to apply and then once we are done with the prompting, we relint the file and show him the errors once again. Ignoring the once the user decided to skip. In this case we would only be relinting once when the user has finished applying all the fixes. In this case we avoid linting from scratch for each fix and also ignore those fixes that the user does not want to apply in the relinting phase (I am not sure how hard it will be to remember the errors that the user does not want to apply, we definitely do not want to show them those errors when we relint the file at the end)

[MichaReiser]

#4183 introduced three different applicability levels. How would you propose that the review mode works for these different levels?

[sladyn98]

I think we would need to prompt even the safe fixes to the user even though the fix is safe, maybe we could add an additional prompt that this fix is safe to apply and would not change anything in the working of the program, and for the suggested ones we could write that this fix is suggested and might be safe to apply make sure you check your projects requirements before applying this fix.

[jfmengels]

One thing we need to make sure is that the fix does not introduce any more errors. If so we abort the autofix.

On the contrary, you absolutely want the fix to introduce more errors! (Not syntax errors though!)
For instance if there is a rule to report unused variables and you start with

a = 1
b = 2
c = a + b

then c will be removed. That introduces a and b as new errors. And those you now want to fix.

[sladyn98]

Oh yeah correct ! i meant syntax errors thanks

[jfmengels]

Yeah, elm-review automatically ignores fixes that it deems invalid, so when:

• the file becomes syntactically incorrect
• The fix leaves the file as it as before (that would probably result in an infinite loop)

and maybe one or two other cases but that's probably more implementation details

[jfmengels]

Btw, when we run in non-fix mode, fixable errors have a (FIX) prefix in elm-review written in blue, and in fix mode if they then turn out to be invalid (because pre-computing that is expensive), then we prefix with (FIX FAILED) written in red.

[evanrittenhouse]

We would need to modify or write a variant that would prompt the user that this is the fix that is being applied and this is how the new line will look.

The proposed display is solid, I think, but I think we may want to add the filename and source code location (i.e., row and column) for each fix.

Maybe filename persists at the top and each fix shows its row + column (and optionally more context around it, similar to git diff)?

[sladyn98]

Yeah this makes sense, IMO this could be simple to do

[evanrittenhouse]

Prompt the user for the fix. (Once the fix has been applied we can either let the user apply all fixes or apply a fix and then relint

Are you proposing relinting after every user rule acceptance or after the user has gone through all errors from a specific lint run? For example, the proposed input display shows 4 errors (presumably in the same file). Do we re-lint that file 4 times or once after the user has checked all 4 errors?

[sladyn98]

Just once at the end of the user applied fix.

[evanrittenhouse]

We may also want to add some sort of fix accepted/fix rejected state to the display. We may also want to expose the notion of a fix "session" - i.e., being able to accept/deny all fixes from a given command run

[jfmengels]

How/when would you show a accepted fix? I'm not sure I follow the idea 🤔

[sladyn98]

I think I get what you mean. I guess you want each fix to have a state that either this fix has been accepeted or rejected once we run the command. I am thinking this would mean we would need to track the fix session. Some of the questions that pop up are

a) Would this be tracked only if the command is run just once ? Like what if the user runs the command again, does this same state persist ?
b) Storing this would mean we would need to track this session inside ruff. I am just writing my thoughts out here but what if the binary is recompiled how would we track the session

We could have a rather simple approach to this and just have a state for just that command run and make it all vanish once the command has exited or returned zero, this could be when the user finishes applying the fix.

[jfmengels]

I might be misunderstanding, but in my experience (and I don't think it would differ for Ruff), the only thing you need to store internally is a list of things that identify ignored errors.
(That and a copy of the project/files in memory if you want to do a fix-all diff).

I don't think this state should be persisted across runs, because that would make un-ignoring a fix painful UX-wise 🤔

[sladyn98]

Do we want to break this down into smaller issues, or do some more hacking in this space