Suggestion on code actions for unsafe rules

[magnuslarsen]

With the introduction of the unsafe auto-fixes in ruff, I feel having a single toggle in pylsp for enabling/disabling them are quite too inefficient or "too safe", as either make all unsafe fixes disappear, or making the Fix all-code action quite unsafe in nature

My idea is instead, if you leave pylsp.plugins.ruff.unsafeFixes as False then the Fix all-code-action should fix all safe rules, but still make the individual unsafe rules available and actionable on the respective diagnostic/location

This strikes the balance of being safe with the Fix all-code-action, and still being able to discover and make a choice on the unsafe fix

(If you instead set pylsp.plugins.ruff.unsafeFixes to True, then the plugin should work exactly as now (because then you signed up for the "unsafety-ness"))

With that being said, the unsafe-code actions should probably indicate they are unsafe somehow

If this isn't too crazy of an idea, I'd happily try to implement it :-)

[jhossbach]

I was also fiddling with the idea of adding (unsafe) to any unsafe code action.
I don't like the idea however of Fix all not actually performing all possible fixes.
Consider also this example:

def f():
    a = t  # F821: Undefined name `t`
    a = 2  # F841: Local variable `a` is assigned but never used

Running Fix all without --unsafe-fixes would not fix any of the errors (which makes sense, since there is no safe fix for F821 and F841. Now you could manually run the code action for F841 which removes a=2 but not F821. With the --unsafe-fixes option, the Fix all option ruff will format the code as

def f():
    pass

My point is that I would expect the Fix all to do exactly what I would expect it to do, namely to do the exact same as ruff check --fix --unsafe-fixes ....

However I am happy to hear the opinion of others on this.

[magnuslarsen]

I was also fiddling with the idea of adding (unsafe) to any unsafe code action.

I think this would be more reasonable; it at least gives the programmer a visual reminder that the action is not-safe

I don't like the idea however of Fix all not actually performing all possible fixes.

My idea was that Fix all in would either fix everything or only fix safe actions, for --unsafe-fixes and without respectively.

Instead, for cases without --unsafe-fixes, we extend the published code-actions to also include unsafe code actions, but to not let them be apart of the Fix all code action (so it only fixes all safe rules, which is expected without the --unsafe-fixes flag)

This way they are visual in the editor, but you have to apply them manually on a rule-by-rule basis (which is why I thought having a visual reminder that the code action is unsafe would be helpful)

Simply put: always run with --unsafe-fixes, making pylsp.plugins.ruff.unsafeFixes only controls whether or not unsafe fixes are included in the Fix all code action

[jhossbach]

I have to reopen this, since in the LSP specification it explicitly says

'Fix all' actions automatically fix errors that have a clear fix that do not require user input.
They should not suppress errors or perform unsafe fixes such as generating new types or classes

So unsafe fixes should never be fixed using the Fix all action.

[jhossbach]

Ah my bad, we already fixed this.