Suggestion on code actions for unsafe rules
magnuslarsen opened this issue · 4 comments
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 :-)
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.
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
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.
Ah my bad, we already fixed this.