From 8b9a57c7ccc1bfd3f530d1b9e2ef1e5ebb6b7047 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 22 Feb 2024 01:06:52 +0200 Subject: [PATCH] feat: unused variable code action (#349) --- lib/next_ls.ex | 30 ++++++++ lib/next_ls/code_actionable.ex | 29 +++++++ lib/next_ls/extensions/credo_extension.ex | 2 +- .../extensions/credo_extension/code_action.ex | 10 +++ lib/next_ls/extensions/elixir_extension.ex | 16 +++- .../elixir_extension/code_action.ex | 19 +++++ .../code_action/unused_variable.ex | 33 ++++++++ test/next_ls/diagnostics_test.exs | 3 +- .../code_action/unused_variable_test.exs | 52 +++++++++++++ .../elixir_extension/code_action_test.exs | 77 +++++++++++++++++++ .../extensions/elixir_extension_test.exs | 4 + 11 files changed, 272 insertions(+), 3 deletions(-) create mode 100644 lib/next_ls/code_actionable.ex create mode 100644 lib/next_ls/extensions/credo_extension/code_action.ex create mode 100644 lib/next_ls/extensions/elixir_extension/code_action.ex create mode 100644 lib/next_ls/extensions/elixir_extension/code_action/unused_variable.ex create mode 100644 test/next_ls/extensions/elixir_extension/code_action/unused_variable_test.exs create mode 100644 test/next_ls/extensions/elixir_extension/code_action_test.exs diff --git a/lib/next_ls.ex b/lib/next_ls.ex index 7ba9440d..5eaa13f7 100644 --- a/lib/next_ls.ex +++ b/lib/next_ls.ex @@ -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 @@ -16,6 +17,7 @@ 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 @@ -23,6 +25,10 @@ defmodule NextLS do 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 @@ -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 @@ -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{ @@ -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 -> diff --git a/lib/next_ls/code_actionable.ex b/lib/next_ls/code_actionable.ex new file mode 100644 index 00000000..f5c32bf6 --- /dev/null +++ b/lib/next_ls/code_actionable.ex @@ -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 + @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 diff --git a/lib/next_ls/extensions/credo_extension.ex b/lib/next_ls/extensions/credo_extension.ex index 4370a2e4..46423c04 100644 --- a/lib/next_ls/extensions/credo_extension.ex +++ b/lib/next_ls/extensions/credo_extension.ex @@ -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{ diff --git a/lib/next_ls/extensions/credo_extension/code_action.ex b/lib/next_ls/extensions/credo_extension/code_action.ex new file mode 100644 index 00000000..0a7e6303 --- /dev/null +++ b/lib/next_ls/extensions/credo_extension/code_action.ex @@ -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 diff --git a/lib/next_ls/extensions/elixir_extension.ex b/lib/next_ls/extensions/elixir_extension.ex index d5dd2a75..5d369f2e 100644 --- a/lib/next_ls/extensions/elixir_extension.ex +++ b/lib/next_ls/extensions/elixir_extension.ex @@ -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 @@ -111,4 +112,17 @@ defmodule NextLS.ElixirExtension do end def clamp(line), do: max(line, 0) + + @unused_variable ~r/variable\s\"[^\"]+\"\sis\sunused/ + 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 diff --git a/lib/next_ls/extensions/elixir_extension/code_action.ex b/lib/next_ls/extensions/elixir_extension/code_action.ex new file mode 100644 index 00000000..1722b2b9 --- /dev/null +++ b/lib/next_ls/extensions/elixir_extension/code_action.ex @@ -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 diff --git a/lib/next_ls/extensions/elixir_extension/code_action/unused_variable.ex b/lib/next_ls/extensions/elixir_extension/code_action/unused_variable.ex new file mode 100644 index 00000000..c244c5e0 --- /dev/null +++ b/lib/next_ls/extensions/elixir_extension/code_action/unused_variable.ex @@ -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 diff --git a/test/next_ls/diagnostics_test.exs b/test/next_ls/diagnostics_test.exs index 09589e97..64900ba0 100644 --- a/test/next_ls/diagnostics_test.exs +++ b/test/next_ls/diagnostics_test.exs @@ -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"} } ] } diff --git a/test/next_ls/extensions/elixir_extension/code_action/unused_variable_test.exs b/test/next_ls/extensions/elixir_extension/code_action/unused_variable_test.exs new file mode 100644 index 00000000..070e50d7 --- /dev/null +++ b/test/next_ls/extensions/elixir_extension/code_action/unused_variable_test.exs @@ -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 diff --git a/test/next_ls/extensions/elixir_extension/code_action_test.exs b/test/next_ls/extensions/elixir_extension/code_action_test.exs new file mode 100644 index 00000000..945e0769 --- /dev/null +++ b/test/next_ls/extensions/elixir_extension/code_action_test.exs @@ -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 diff --git a/test/next_ls/extensions/elixir_extension_test.exs b/test/next_ls/extensions/elixir_extension_test.exs index bb212211..8bfd96f6 100644 --- a/test/next_ls/extensions/elixir_extension_test.exs +++ b/test/next_ls/extensions/elixir_extension_test.exs @@ -65,6 +65,7 @@ defmodule NextLS.ElixirExtensionTest do assert %{ with_iodata.file => [ %GenLSP.Structures.Diagnostic{ + data: %{"namespace" => "elixir"}, severity: 2, message: "ElixirExtension.foo/0" <> @@ -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", @@ -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", @@ -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",