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: unused variable code action #349

Merged
merged 10 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lib/next_ls.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ defmodule NextLS do

import NextLS.DB.Query

alias GenLSP.Enumerations.CodeActionKind
alias GenLSP.Enumerations.ErrorCodes
alias GenLSP.Enumerations.TextDocumentSyncKind
alias GenLSP.ErrorResponse
Expand All @@ -16,13 +17,18 @@ defmodule NextLS do
alias GenLSP.Notifications.WorkspaceDidChangeWorkspaceFolders
alias GenLSP.Requests.Initialize
alias GenLSP.Requests.Shutdown
alias GenLSP.Requests.TextDocumentCodeAction
alias GenLSP.Requests.TextDocumentCompletion
alias GenLSP.Requests.TextDocumentDefinition
alias GenLSP.Requests.TextDocumentDocumentSymbol
alias GenLSP.Requests.TextDocumentFormatting
alias GenLSP.Requests.TextDocumentHover
alias GenLSP.Requests.TextDocumentReferences
alias GenLSP.Requests.WorkspaceSymbol
alias GenLSP.Structures.CodeActionContext
alias GenLSP.Structures.CodeActionOptions
alias GenLSP.Structures.CodeActionParams
alias GenLSP.Structures.Diagnostic
alias GenLSP.Structures.DidChangeWatchedFilesParams
alias GenLSP.Structures.DidChangeWorkspaceFoldersParams
alias GenLSP.Structures.DidOpenTextDocumentParams
Expand All @@ -34,6 +40,7 @@ defmodule NextLS do
alias GenLSP.Structures.SaveOptions
alias GenLSP.Structures.ServerCapabilities
alias GenLSP.Structures.SymbolInformation
alias GenLSP.Structures.TextDocumentIdentifier
alias GenLSP.Structures.TextDocumentItem
alias GenLSP.Structures.TextDocumentSyncOptions
alias GenLSP.Structures.TextEdit
Expand Down Expand Up @@ -118,6 +125,9 @@ defmodule NextLS do
save: %SaveOptions{include_text: true},
change: TextDocumentSyncKind.full()
},
code_action_provider: %CodeActionOptions{
code_action_kinds: [CodeActionKind.quick_fix()]
},
completion_provider:
if init_opts.experimental.completions.enable do
%GenLSP.Structures.CompletionOptions{
Expand Down Expand Up @@ -149,6 +159,26 @@ defmodule NextLS do
)}
end

def handle_request(
%TextDocumentCodeAction{
params: %CodeActionParams{
context: %CodeActionContext{diagnostics: diagnostics},
text_document: %TextDocumentIdentifier{uri: uri}
}
},
lsp
) do
code_actions =
for %Diagnostic{} = diagnostic <- diagnostics,
data = %NextLS.CodeActionable.Data{diagnostic: diagnostic, uri: uri, document: lsp.assigns.documents[uri]},
namespace = diagnostic.data["namespace"],
action <- NextLS.CodeActionable.from(namespace, data) do
action
end

{:reply, code_actions, lsp}
end

def handle_request(%TextDocumentDefinition{params: %{text_document: %{uri: uri}, position: position}}, lsp) do
result =
dispatch(lsp.assigns.registry, :databases, fn entries ->
Expand Down
29 changes: 29 additions & 0 deletions lib/next_ls/code_actionable.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
defmodule NextLS.CodeActionable do
@moduledoc false
# A diagnostic can produce 1 or more code actions hence we return a list

alias GenLSP.Structures.CodeAction
alias GenLSP.Structures.Diagnostic

defmodule Data do
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 I can remove this in favour of just using schematic

@moduledoc false
defstruct [:diagnostic, :uri, :document]

@type t :: %__MODULE__{
diagnostic: Diagnostic.t(),
uri: String.t(),
document: String.t()
}
end

@callback from(diagnostic :: Data.t()) :: [CodeAction.t()]

# TODO: Add support for third party extensions
def from("elixir", diagnostic_data) do
NextLS.ElixirExtension.CodeAction.from(diagnostic_data)
end

def from("credo", diagnostic_data) do
NextLS.CredoExtension.CodeAction.from(diagnostic_data)
end
end
2 changes: 1 addition & 1 deletion lib/next_ls/extensions/credo_extension.ex
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ defmodule NextLS.CredoExtension do
}
},
severity: category_to_severity(issue.category),
data: %{check: issue.check, file: issue.filename},
data: %{check: issue.check, file: issue.filename, namespace: :credo},
source: "credo",
code: Macro.to_string(issue.check),
code_description: %CodeDescription{
Expand Down
10 changes: 10 additions & 0 deletions lib/next_ls/extensions/credo_extension/code_action.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
defmodule NextLS.CredoExtension.CodeAction do
@moduledoc false

@behaviour NextLS.CodeActionable

alias NextLS.CodeActionable.Data

@impl true
def from(%Data{} = _data), do: []
end
16 changes: 15 additions & 1 deletion lib/next_ls/extensions/elixir_extension.ex
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ defmodule NextLS.ElixirExtension do
severity: severity(d.severity),
message: IO.iodata_to_binary(d.message),
source: d.compiler_name,
range: range(d.position, Map.get(d, :span))
range: range(d.position, Map.get(d, :span)),
data: metadata(d)
})
end

Expand Down Expand Up @@ -111,4 +112,17 @@ defmodule NextLS.ElixirExtension do
end

def clamp(line), do: max(line, 0)

@unused_variable ~r/variable\s\"[^\"]+\"\sis\sunused/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if elixir core would consider tagging diagnostics with IDs, like EX1 means "unused variable" 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be dope

defp metadata(diagnostic) do
base = %{"namespace" => "elixir"}

cond do
is_binary(diagnostic.message) and diagnostic.message =~ @unused_variable ->
Map.put(base, "type", "unused_variable")

true ->
base
end
end
end
19 changes: 19 additions & 0 deletions lib/next_ls/extensions/elixir_extension/code_action.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
defmodule NextLS.ElixirExtension.CodeAction do
@moduledoc false

@behaviour NextLS.CodeActionable

alias NextLS.CodeActionable.Data
alias NextLS.ElixirExtension.CodeAction.UnusedVariable

@impl true
def from(%Data{} = data) do
case data.diagnostic.data do
%{"type" => "unused_variable"} ->
UnusedVariable.new(data.diagnostic, data.document, data.uri)

_ ->
[]
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
defmodule NextLS.ElixirExtension.CodeAction.UnusedVariable do
@moduledoc false

alias GenLSP.Structures.CodeAction
alias GenLSP.Structures.Diagnostic
alias GenLSP.Structures.Range
alias GenLSP.Structures.TextEdit
alias GenLSP.Structures.WorkspaceEdit

def new(diagnostic, _text, uri) do
%Diagnostic{range: %{start: start}} = diagnostic

[
%CodeAction{
title: "Underscore unused variable",
diagnostics: [diagnostic],
edit: %WorkspaceEdit{
changes: %{
uri => [
%TextEdit{
new_text: "_",
range: %Range{
start: start,
end: start
}
}
]
}
}
}
]
end
end
3 changes: 2 additions & 1 deletion test/next_ls/diagnostics_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ defmodule NextLS.DiagnosticsTest do
"range" => %{
"start" => %{"line" => 3, "character" => ^char},
"end" => %{"line" => 3, "character" => 14}
}
},
"data" => %{"type" => "unused_variable", "namespace" => "elixir"}
}
]
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
defmodule NextLS.ElixirExtension.UnusedVariableTest do
use ExUnit.Case, async: true

alias GenLSP.Structures.CodeAction
alias GenLSP.Structures.Position
alias GenLSP.Structures.Range
alias GenLSP.Structures.TextEdit
alias GenLSP.Structures.WorkspaceEdit
alias NextLS.ElixirExtension.CodeAction.UnusedVariable

test "adds an underscore to unused variables" do
text = """
defmodule Test.Unused do
def hello() do
foo = 3
:world
end
end
"""

start = %Position{character: 4, line: 3}

diagnostic = %GenLSP.Structures.Diagnostic{
data: %{"namespace" => "elixir", "type" => "unused_variable"},
message: "variable \"foo\" is unused (if the variable is not meant to be used, prefix it with an underscore)",
source: "Elixir",
range: %GenLSP.Structures.Range{
start: start,
end: %{start | character: 999}
}
}

uri = "file:///home/owner/my_project/hello.ex"

assert [code_action] = UnusedVariable.new(diagnostic, text, uri)
assert is_struct(code_action, CodeAction)
assert [diagnostic] == code_action.diagnostics

# We insert a single underscore character at the start position of the unused variable
# Hence the start and end positions are matching the original start position in the diagnostic
assert %WorkspaceEdit{
changes: %{
^uri => [
%TextEdit{
new_text: "_",
range: %Range{start: ^start, end: ^start}
}
]
}
} = code_action.edit
end
end
77 changes: 77 additions & 0 deletions test/next_ls/extensions/elixir_extension/code_action_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
defmodule NextLS.Extensions.ElixirExtension.CodeActionTest do
use ExUnit.Case, async: true

import GenLSP.Test
import NextLS.Support.Utils

@moduletag :tmp_dir
@moduletag root_paths: ["my_proj"]

setup %{tmp_dir: tmp_dir} do
File.mkdir_p!(Path.join(tmp_dir, "my_proj/lib"))
File.write!(Path.join(tmp_dir, "my_proj/mix.exs"), mix_exs())

cwd = Path.join(tmp_dir, "my_proj")

foo_path = Path.join(cwd, "lib/foo.ex")

foo = """
defmodule MyProj.Foo do
def hello() do
foo = :bar
:world
end
end
"""

File.write!(foo_path, foo)

[foo: foo, foo_path: foo_path]
end

setup :with_lsp

setup context do
assert :ok == notify(context.client, %{method: "initialized", jsonrpc: "2.0", params: %{}})
assert_is_ready(context, "my_proj")
assert_compiled(context, "my_proj")
assert_notification "$/progress", %{"value" => %{"kind" => "end", "message" => "Finished indexing!"}}

did_open(context.client, context.foo_path, context.foo)
context
end

test "sends back a list of code actions", %{client: client, foo_path: foo} do
foo_uri = uri(foo)
id = 1

request client, %{
method: "textDocument/codeAction",
id: id,
jsonrpc: "2.0",
params: %{
context: %{
"diagnostics" => [
%{
"data" => %{"namespace" => "elixir", "type" => "unused_variable"},
"message" =>
"variable \"foo\" is unused (if the variable is not meant to be used, prefix it with an underscore)",
"range" => %{"end" => %{"character" => 999, "line" => 2}, "start" => %{"character" => 4, "line" => 2}},
"severity" => 2,
"source" => "Elixir"
}
]
},
range: %{start: %{line: 2, character: 4}, end: %{line: 2, character: 999}},
textDocument: %{uri: foo_uri}
}
}

assert_receive %{
"jsonrpc" => "2.0",
"id" => 1,
"result" => [%{"edit" => %{"changes" => %{^foo_uri => [%{"newText" => "_"}]}}}]
},
500
end
end
4 changes: 4 additions & 0 deletions test/next_ls/extensions/elixir_extension_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ defmodule NextLS.ElixirExtensionTest do
assert %{
with_iodata.file => [
%GenLSP.Structures.Diagnostic{
data: %{"namespace" => "elixir"},
severity: 2,
message:
"ElixirExtension.foo/0" <>
Expand All @@ -84,6 +85,7 @@ defmodule NextLS.ElixirExtensionTest do
],
only_line.file => [
%GenLSP.Structures.Diagnostic{
data: %{"namespace" => "elixir"},
severity: 2,
message: "kind of bad",
source: "Elixir",
Expand All @@ -101,6 +103,7 @@ defmodule NextLS.ElixirExtensionTest do
],
line_and_col.file => [
%GenLSP.Structures.Diagnostic{
data: %{"namespace" => "elixir"},
severity: 1,
message: "nothing works",
source: "Elixir",
Expand All @@ -118,6 +121,7 @@ defmodule NextLS.ElixirExtensionTest do
],
start_and_end.file => [
%GenLSP.Structures.Diagnostic{
data: %{"namespace" => "elixir"},
severity: 4,
message: "here's a hint",
source: "Elixir",
Expand Down
Loading