Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add remove debugger code action #426

Merged
merged 11 commits into from
May 1, 2024

Conversation

NJichev
Copy link
Collaborator

@NJichev NJichev commented Apr 20, 2024

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
Copy link
Collaborator Author

NJichev commented Apr 20, 2024

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.

@NJichev NJichev force-pushed the add-remove-debugger-code-action branch from 4135f6f to be3cdaa Compare April 22, 2024 07:49
@mhanberg
Copy link
Collaborator

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
Copy link
Collaborator Author

NJichev commented Apr 22, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@mhanberg
Copy link
Collaborator

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
Copy link
Collaborator Author

NJichev commented Apr 22, 2024

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
Copy link
Collaborator

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

@NJichev
Copy link
Collaborator Author

NJichev commented Apr 22, 2024

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

@mhanberg
Copy link
Collaborator

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

Copy link
Collaborator

@mhanberg mhanberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -0,0 +1,38 @@
defmodule NextLS.Test do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should go in the test/support directory.

Also probably should consolidate with the other test helper module

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert_is_text_edit(text, edit, expected)
end

test "handles empty function bodies" do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test name doesn't seem accurate

@mhanberg
Copy link
Collaborator

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
Copy link
Collaborator

That might be a credo issue tho, i see that the diagnostics for the two IO.inspects have the same range
CleanShot 2024-04-23 at 09 00 39@2x

defp remove_debugger({:dbg, _, [arg | _]}), do: arg
defp remove_debugger(_node), do: {:__block__, [], []}

defp make_title({_, ctx, _} = node), do: "Remove #{Macro.to_string(node)} column(#{ctx[:column]})"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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.

Comment on lines 57 to 75
if !acc &&
(matches_debug?(node, pos) || matches_pipe?(node, pos)) &&
Sourceror.compare_positions(range.start, pos) in [:lt, :eq] &&
Sourceror.compare_positions(range.end, pos) in [:gt, :eq] do
{tree, node}
else
{tree, acc}
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fixed now

@mhanberg
Copy link
Collaborator

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

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
Adds a macro for testing text edits.
Fixes edge-cases when piping debug expr.
Fixes edge-cases when we have a single block with the pipe expr.
This refactor takes in only the AST of the debugging node and replaces
it. For IO.inspect/1 and dbg/1 the first argument is preserved.
@NJichev NJichev force-pushed the add-remove-debugger-code-action branch from 3a65ecb to f6f09dc Compare April 30, 2024 22:45
Saw an error in local when i tried to fetch the code actions again
without saving, not really sure if its a good fix but it should stop the
LSP from crashing
@mhanberg
Copy link
Collaborator

mhanberg commented May 1, 2024

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 mhanberg enabled auto-merge (squash) May 1, 2024 12:28
@mhanberg
Copy link
Collaborator

mhanberg commented May 1, 2024

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"

@mhanberg mhanberg merged commit 7f2f4f4 into elixir-tools:main May 1, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants