feat: add remove debugger code action

[NJichev]

A Code Action that removes the following warning checks from Credo:

• Credo.Check.Warning.Dbg
• Credo.Check.Warning.IExPry
• Credo.Check.Warning.IoInspect
• Credo.Check.Warning.IoPuts
• Credo.Check.Warning.MixEnv

[NJichev]

Note this is a quick and dirty version, this would delete the whole line in the following scenarios:

IO.inspect(arg); foo

list |> Enum.map(...) |> IO.inspect() |> Enum.join()

I think it's fine as a starting point but I could rework it to get the start and end ranges of the expression from spitfire in this PR.

[mhanberg]

Couple things to think about.

I believe we want the text edit to be as precise as possible, meaning only reflects the code we are changing.

This is due to the code possibly having errors and when we reformat it, it will introduce error artifacts.

So I think we want to only format the node we are affecting, and create a text edit with that range.

Also an edge case, having two debugger statements on the same line, but you only want to remove one of the. Consider something like foo(dbg(arg1), IO.inspect(arg2))

[NJichev]

The problem with affecting only the debugger node is that it will break in the semicolon and the piping cases, i.e.:
IO.inspect(arg, label: "foo"); expr
You'll be left with ; expr instead of expr - with a bigger scope the formatter deals with this. Also you'll be stuck most likely with empty lines of indentation as well that way(which could be okay with the client).

[mhanberg]

I also realized that we should only remove the function call but not the argument.

So in this case removing the semicolon is not a problem, because the argument will still be there.

[mhanberg]

At least for pass through functions like IO.inspect and dbg

[mhanberg]

I think you might be able to assume the expression is terminated with a semicolon if it has the newlines key in the metadata is equal to 0 (meaning, not >=1 or not present at all)

[NJichev]

I also realized that we should only remove the function call but not the argument.

So in this case removing the semicolon is not a problem, because the argument will still be there.

Hmm but that would break cases such as this one:

def f(bar) do
  IO.inspect(bar)
  other_f(bar) |> IO.inspect()
  ... code
 end

In this use case applying the code action for the first inspect and removing the argument would be the correct thing to do imo.
And if you don't want to remove the argument you can just use the pipe variant.

[mhanberg]

How does that break? I think I'm missing it haha

[NJichev]

I mean not break it 😅 But leave the variable there hanging.

[mhanberg]

I mean not break it 😅 But leave the variable there hanging.

I see, I think think this is preferable, as if you have them inside a function call, it would just straight remove the argument and change the semantics of the code.i frequently do this myself.

I think you also mentioned that you should just use the pipe operator to avoid this, but with regard to dbg, this actually changes the sematnics of the code if you do that, so i frequently wrap it on purpose, and that's where i'd like to have the code action, but its tedious to remove the wrapping call to dbg

[mhanberg]

Seems good, just left a couple comments. I still need to test it out locally.

[mhanberg]

This file should go in the test/support directory.

Also probably should consolidate with the other test helper module

[NJichev]

My thought process was other extension devs might use it to test their code actions/workspace commands

[mhanberg]

not a bad idea, but lets cross that road when we get to it

[mhanberg]

Test name doesn't seem accurate

[mhanberg]

CleanShot.2024-04-23.at.08.53.29.mp4

found an edge case

bundle_base |> dbg() |> NextLS.Runtime.BundledElixir.install(IO.inspect(logger), mix_home: mixhome) |> IO.inspect()

with this original code, 3 code actions appear, but they all seem to be for the piped IO.inspect call at the end. once you activate one of those, they turn into unique code actions.

when activated, they all worked, tho

[mhanberg]

That might be a credo issue tho, i see that the diagnostics for the two IO.inspects have the same range

[mhanberg]

Lets make this

"Remove `#{function}` #{line}:#{col}"

so that it looks like

"Remove `dbg` 76:12"

I believe will require creating a lookup table based on the check

[mhanberg]

I think we can probably get this in for the 0.21 release, but i hope to do that before the end of the week. no pressure tho if we can't get it over the line.

[mhanberg]

last thing i think.

i've patched the bug where IO.inspect and dbg warnings didn't have the correct columns (it previously backfilled column by just doing a string find on the line, i changed it to use the actual column metadata in the ast).

so these lines need to also account for column, not just line.

i'll get my PR up for credo and then you can test against that branch to confirm

[mhanberg]

rrrene/credo#1126

[mhanberg]

looks like we need to patch the Mix.env, IO.puts, and IEx.pry rules as well. i can do it later unless you get to it first

but we can merge this once the above comment is merged, theres a chance credo doesn't merge and release these patches for a while

[NJichev]

I think this is fixed now

[mhanberg]

can you rebase and then i'll try this out?

[mhanberg]

pushed a change to use credo from rrrene/credo rather than my fork. i think i deleted that branch when he merged it

also pushed a change that adds an "empty fallback" to the min_by function. it would raise if you triggered the code actions in the editor again after fixing one of them, because it would still have the diagnostic that was just fixed.

i think in a followup PR we can make code actions that fix a diagnostic, remove the diagnostic as well, as its presumably fixed

[mhanberg]

this code action is going to drastically speed up my workflow (i'm a heavy puts debugger), thank you so much for grinding this one out!

another follow up, maybe include a code action for "remove all debugger statements"