From ebf6760f8759e0065c220cfd8cf6b3626a358f03 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 22 Feb 2024 16:40:47 +0200 Subject: [PATCH 1/5] feat: add require code action This adds a require code action that adds `require Module` to your module whenever a macro is used without requiring it beforehand. It tries to insert the require after all the top level Elixir macros(moduledoc, alias, require, import). --- lib/next_ls/extensions/elixir_extension.ex | 4 + .../elixir_extension/code_action.ex | 4 + .../elixir_extension/code_action/require.ex | 132 ++++++++++++ .../code_action/require_test.exs | 202 ++++++++++++++++++ .../code_action/unused_variable_test.exs | 20 +- .../elixir_extension/code_action_test.exs | 48 +++++ 6 files changed, 402 insertions(+), 8 deletions(-) create mode 100644 lib/next_ls/extensions/elixir_extension/code_action/require.ex create mode 100644 test/next_ls/extensions/elixir_extension/code_action/require_test.exs diff --git a/lib/next_ls/extensions/elixir_extension.ex b/lib/next_ls/extensions/elixir_extension.ex index 5d369f2e..a6c7166a 100644 --- a/lib/next_ls/extensions/elixir_extension.ex +++ b/lib/next_ls/extensions/elixir_extension.ex @@ -114,6 +114,7 @@ defmodule NextLS.ElixirExtension do def clamp(line), do: max(line, 0) @unused_variable ~r/variable\s\"[^\"]+\"\sis\sunused/ + @require_module ~r/you\smust\srequire/ defp metadata(diagnostic) do base = %{"namespace" => "elixir"} @@ -121,6 +122,9 @@ defmodule NextLS.ElixirExtension do is_binary(diagnostic.message) and diagnostic.message =~ @unused_variable -> Map.put(base, "type", "unused_variable") + is_binary(diagnostic.message) and diagnostic.message =~ @require_module -> + Map.put(base, "type", "require") + true -> base end diff --git a/lib/next_ls/extensions/elixir_extension/code_action.ex b/lib/next_ls/extensions/elixir_extension/code_action.ex index 1722b2b9..ea1310b7 100644 --- a/lib/next_ls/extensions/elixir_extension/code_action.ex +++ b/lib/next_ls/extensions/elixir_extension/code_action.ex @@ -4,6 +4,7 @@ defmodule NextLS.ElixirExtension.CodeAction do @behaviour NextLS.CodeActionable alias NextLS.CodeActionable.Data + alias NextLS.ElixirExtension.CodeAction.Require alias NextLS.ElixirExtension.CodeAction.UnusedVariable @impl true @@ -12,6 +13,9 @@ defmodule NextLS.ElixirExtension.CodeAction do %{"type" => "unused_variable"} -> UnusedVariable.new(data.diagnostic, data.document, data.uri) + %{"type" => "require"} -> + Require.new(data.diagnostic, data.document, data.uri) + _ -> [] end diff --git a/lib/next_ls/extensions/elixir_extension/code_action/require.ex b/lib/next_ls/extensions/elixir_extension/code_action/require.ex new file mode 100644 index 00000000..4c119794 --- /dev/null +++ b/lib/next_ls/extensions/elixir_extension/code_action/require.ex @@ -0,0 +1,132 @@ +defmodule NextLS.ElixirExtension.CodeAction.Require do + @moduledoc false + + alias GenLSP.Structures.CodeAction + alias GenLSP.Structures.Diagnostic + alias GenLSP.Structures.Position + alias GenLSP.Structures.Range + alias GenLSP.Structures.TextEdit + alias GenLSP.Structures.WorkspaceEdit + + @one_indentation_level " " + def new(%Diagnostic{} = diagnostic, text, uri) do + range = diagnostic.range + + with {:ok, require_module} <- get_edit(diagnostic.message), + {:ok, ast} <- parse_ast(text), + {:ok, defm} <- nearest_defmodule(ast, range), + {:ok, indentation} <- get_indent(text, defm), + {:ok, module_name} <- get_module_name(defm), + nearest <- find_nearest_node_for_require(defm), + range <- get_edit_range(nearest) do + [ + %CodeAction{ + title: "Add missing require in #{module_name}", + diagnostics: [diagnostic], + edit: %WorkspaceEdit{ + changes: %{ + uri => [ + %TextEdit{ + new_text: indentation <> require_module, + range: range + } + ] + } + } + } + ] + else + _error -> + [] + end + end + + defp parse_ast(text) do + text + |> Enum.join("\n") + |> Spitfire.parse() + end + + defp nearest_defmodule(ast, range) do + defmodules = + ast + |> Macro.prewalker() + |> Enum.filter(fn + {:defmodule, _, _} -> true + _ -> false + end) + + if defmodules != [] do + defm = + Enum.min_by(defmodules, fn {_, ctx, _} -> + range.start.character - ctx[:line] + 1 + end) + + {:ok, defm} + else + {:error, "no defmodule definition"} + end + end + + @module_name ~r/require\s+([^\s]+)\s+before/ + defp get_edit(message) do + case Regex.run(@module_name, message) do + [_, module] -> {:ok, "require #{module}\n"} + _ -> {:error, "unable to find require"} + end + end + + # Context starts from 1 while LSP starts from 0 + # which works for us since we want to insert the require on the next line + defp get_edit_range(context) do + %Range{ + start: %Position{line: context[:line], character: 0}, + end: %Position{line: context[:line], character: 0} + } + end + + defp get_indent(text, {_, defm_context, _}) do + line = defm_context[:line] - 1 + + indent = + text + |> Enum.at(line) + |> then(&Regex.run(~r/^(\s*).*/, &1)) + |> List.last() + + {:ok, indent <> @one_indentation_level} + end + + @top_level_macros [:import, :alias, :require] + defp find_nearest_node_for_require({:defmodule, context, _} = ast) do + top_level_macros = + ast + |> Macro.prewalker() + |> Enum.filter(fn + {:@, _, [{:moduledoc, _, _}]} -> true + {macro, _, _} when macro in @top_level_macros -> true + _ -> false + end) + + case top_level_macros do + [] -> + context + + _ -> + {_, context, _} = Enum.max_by(top_level_macros, fn {_, ctx, _} -> ctx[:line] end) + context + end + end + + defp get_module_name({:defmodule, _, [{:__aliases__, _, alias} | _rest]}) do + name = + alias + |> Module.concat() + |> to_string() + |> String.replace_prefix("Elixir.", "") + + {:ok, name} + end + + defp get_module_name(_), do: {:error, "expected defmodule ast"} +end diff --git a/test/next_ls/extensions/elixir_extension/code_action/require_test.exs b/test/next_ls/extensions/elixir_extension/code_action/require_test.exs new file mode 100644 index 00000000..cdc9861f --- /dev/null +++ b/test/next_ls/extensions/elixir_extension/code_action/require_test.exs @@ -0,0 +1,202 @@ +defmodule NextLS.ElixirExtension.RequireTest 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.Require + + test "adds require to module" do + text = + String.split( + """ + defmodule Test.Require do + def hello() do + Logger.info("foo") + end + end + """, + "\n" + ) + + start = %Position{character: 0, line: 1} + + diagnostic = %GenLSP.Structures.Diagnostic{ + data: %{"namespace" => "elixir", "type" => "require"}, + message: "you must require Logger before invoking the macro Logger.info/1", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: start, + end: %{start | character: 999} + } + } + + uri = "file:///home/owner/my_project/hello.ex" + + assert [code_action] = Require.new(diagnostic, text, uri) + assert is_struct(code_action, CodeAction) + assert [diagnostic] == code_action.diagnostics + assert code_action.title =~ "Test.Require" + + assert %WorkspaceEdit{ + changes: %{ + ^uri => [ + %TextEdit{ + new_text: " require Logger\n", + range: %Range{start: ^start, end: ^start} + } + ] + } + } = code_action.edit + end + + test "adds require after moduledoc" do + text = + String.split( + """ + defmodule Test.Require do + @moduledoc + def hello() do + Logger.info("foo") + end + end + """, + "\n" + ) + + start = %Position{character: 0, line: 2} + + diagnostic = %GenLSP.Structures.Diagnostic{ + data: %{"namespace" => "elixir", "type" => "require"}, + message: "you must require Logger before invoking the macro Logger.info/1", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: start, + end: %{start | character: 999} + } + } + + uri = "file:///home/owner/my_project/hello.ex" + + assert [code_action] = Require.new(diagnostic, text, uri) + assert is_struct(code_action, CodeAction) + assert [diagnostic] == code_action.diagnostics + assert code_action.title =~ "Test.Require" + + assert %WorkspaceEdit{ + changes: %{ + ^uri => [ + %TextEdit{ + new_text: " require Logger\n", + range: %Range{start: ^start, end: ^start} + } + ] + } + } = code_action.edit + end + + test "adds require after alias" do + text = + String.split( + """ + defmodule Test.Require do + @moduledoc + import Test.Foo + alias Test.Bar + def hello() do + Logger.info("foo") + end + end + """, + "\n" + ) + + start = %Position{character: 0, line: 4} + + diagnostic = %GenLSP.Structures.Diagnostic{ + data: %{"namespace" => "elixir", "type" => "require"}, + message: "you must require Logger before invoking the macro Logger.info/1", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: start, + end: %{start | character: 999} + } + } + + uri = "file:///home/owner/my_project/hello.ex" + + assert [code_action] = Require.new(diagnostic, text, uri) + assert is_struct(code_action, CodeAction) + assert [diagnostic] == code_action.diagnostics + assert code_action.title =~ "Test.Require" + + assert %WorkspaceEdit{ + changes: %{ + ^uri => [ + %TextEdit{ + new_text: " require Logger\n", + range: %Range{start: ^start, end: ^start} + } + ] + } + } = code_action.edit + end + + test "figures out the correct module" do + text = + String.split( + """ + defmodule Test do + defmodule Foo do + def hello() do + IO.inspect("foo") + end + end + + defmodule Require do + @moduledoc + import Test.Foo + alias Test.Bar + + def hello() do + Logger.info("foo") + end + end + end + """, + "\n" + ) + + start = %Position{character: 0, line: 11} + + diagnostic = %GenLSP.Structures.Diagnostic{ + data: %{"namespace" => "elixir", "type" => "require"}, + message: "you must require Logger before invoking the macro Logger.info/1", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: start, + end: %{start | character: 999} + } + } + + uri = "file:///home/owner/my_project/hello.ex" + + assert [code_action] = Require.new(diagnostic, text, uri) + assert is_struct(code_action, CodeAction) + assert [diagnostic] == code_action.diagnostics + assert code_action.title =~ "Require" + + assert %WorkspaceEdit{ + changes: %{ + ^uri => [ + %TextEdit{ + new_text: " require Logger\n", + range: %Range{start: ^start, end: ^start} + } + ] + } + } = code_action.edit + end +end 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 index 070e50d7..9252fc5c 100644 --- 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 @@ -9,14 +9,18 @@ defmodule NextLS.ElixirExtension.UnusedVariableTest do 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 - """ + text = + String.split( + """ + defmodule Test.Unused do + def hello() do + foo = 3 + :world + end + end + """, + "\n" + ) start = %Position{character: 4, line: 3} diff --git a/test/next_ls/extensions/elixir_extension/code_action_test.exs b/test/next_ls/extensions/elixir_extension/code_action_test.exs index 945e0769..09fd4846 100644 --- a/test/next_ls/extensions/elixir_extension/code_action_test.exs +++ b/test/next_ls/extensions/elixir_extension/code_action_test.exs @@ -21,6 +21,10 @@ defmodule NextLS.Extensions.ElixirExtension.CodeActionTest do foo = :bar :world end + + def world() do + Logger.info("no require") + end end """ @@ -74,4 +78,48 @@ defmodule NextLS.Extensions.ElixirExtension.CodeActionTest do }, 500 end + + test "can send more than one code action", %{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" + }, + %{ + "data" => %{"namespace" => "elixir", "type" => "require"}, + "message" => "you must require Logger before invoking the macro Logger.info/1", + "range" => %{"end" => %{"character" => 999, "line" => 7}, "start" => %{"character" => 0, "line" => 7}}, + "severity" => 2, + "source" => "Elixir" + } + ] + }, + range: %{start: %{line: 2, character: 0}, end: %{line: 7, character: 999}}, + textDocument: %{uri: foo_uri} + } + } + + assert_receive %{ + "jsonrpc" => "2.0", + "id" => 1, + "result" => [ + %{"edit" => %{"changes" => %{^foo_uri => [%{"newText" => "_"}]}}}, + %{"edit" => %{"changes" => %{^foo_uri => [%{"newText" => " require Logger\n"}]}}} + ] + }, + 500 + end end From 2f6873d5d6b96dfcf2916049f6d9e0d0009cd21a Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 22 Feb 2024 18:16:46 +0200 Subject: [PATCH 2/5] Refactor indent clause --- .../extensions/elixir_extension/code_action/require.ex | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/next_ls/extensions/elixir_extension/code_action/require.ex b/lib/next_ls/extensions/elixir_extension/code_action/require.ex index 4c119794..c725e2af 100644 --- a/lib/next_ls/extensions/elixir_extension/code_action/require.ex +++ b/lib/next_ls/extensions/elixir_extension/code_action/require.ex @@ -9,14 +9,15 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do alias GenLSP.Structures.WorkspaceEdit @one_indentation_level " " - def new(%Diagnostic{} = diagnostic, text, uri) do + @spec new(diagnostic :: Diagnostic.t(), [text :: String.t()], uri :: String.t()) :: [CodeAction.t()] + def new(diagnostic = %Diagnostic{}, text, uri) do range = diagnostic.range with {:ok, require_module} <- get_edit(diagnostic.message), {:ok, ast} <- parse_ast(text), {:ok, defm} <- nearest_defmodule(ast, range), - {:ok, indentation} <- get_indent(text, defm), {:ok, module_name} <- get_module_name(defm), + indentation <- get_indent(text, defm), nearest <- find_nearest_node_for_require(defm), range <- get_edit_range(nearest) do [ @@ -85,16 +86,17 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do } end + @indent ~r/^(\s*).*/ defp get_indent(text, {_, defm_context, _}) do line = defm_context[:line] - 1 indent = text |> Enum.at(line) - |> then(&Regex.run(~r/^(\s*).*/, &1)) + |> then(&Regex.run(@indent, &1)) |> List.last() - {:ok, indent <> @one_indentation_level} + indent <> @one_indentation_level end @top_level_macros [:import, :alias, :require] From bb137c6aabd6e0e739a5347a7968a0208923b172 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 22 Feb 2024 18:20:27 +0200 Subject: [PATCH 3/5] Fix formatting --- lib/next_ls/extensions/elixir_extension/code_action/require.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/next_ls/extensions/elixir_extension/code_action/require.ex b/lib/next_ls/extensions/elixir_extension/code_action/require.ex index c725e2af..a5525ce2 100644 --- a/lib/next_ls/extensions/elixir_extension/code_action/require.ex +++ b/lib/next_ls/extensions/elixir_extension/code_action/require.ex @@ -10,7 +10,7 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do @one_indentation_level " " @spec new(diagnostic :: Diagnostic.t(), [text :: String.t()], uri :: String.t()) :: [CodeAction.t()] - def new(diagnostic = %Diagnostic{}, text, uri) do + def new(%Diagnostic{} = diagnostic, text, uri) do range = diagnostic.range with {:ok, require_module} <- get_edit(diagnostic.message), From 5bedeb02e261db04345f3b6809ec17930efdd576 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 22 Feb 2024 20:36:44 +0200 Subject: [PATCH 4/5] Refactor module name with &Macro.to_string/1 --- lib/next_ls/extensions/elixir_extension/code_action/require.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/next_ls/extensions/elixir_extension/code_action/require.ex b/lib/next_ls/extensions/elixir_extension/code_action/require.ex index a5525ce2..8ae34351 100644 --- a/lib/next_ls/extensions/elixir_extension/code_action/require.ex +++ b/lib/next_ls/extensions/elixir_extension/code_action/require.ex @@ -124,8 +124,7 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do name = alias |> Module.concat() - |> to_string() - |> String.replace_prefix("Elixir.", "") + |> Macro.to_string() {:ok, name} end From 191dde09166424cfcf48931f6b300758895ac831 Mon Sep 17 00:00:00 2001 From: Mitchell Hanberg Date: Sat, 24 Feb 2024 17:13:53 -0500 Subject: [PATCH 5/5] reword title --- .../elixir_extension/code_action/require.ex | 18 +++--------------- .../code_action/require_test.exs | 8 ++++---- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/lib/next_ls/extensions/elixir_extension/code_action/require.ex b/lib/next_ls/extensions/elixir_extension/code_action/require.ex index 8ae34351..ea638847 100644 --- a/lib/next_ls/extensions/elixir_extension/code_action/require.ex +++ b/lib/next_ls/extensions/elixir_extension/code_action/require.ex @@ -16,19 +16,18 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do with {:ok, require_module} <- get_edit(diagnostic.message), {:ok, ast} <- parse_ast(text), {:ok, defm} <- nearest_defmodule(ast, range), - {:ok, module_name} <- get_module_name(defm), indentation <- get_indent(text, defm), nearest <- find_nearest_node_for_require(defm), range <- get_edit_range(nearest) do [ %CodeAction{ - title: "Add missing require in #{module_name}", + title: "Add missing require for #{require_module}", diagnostics: [diagnostic], edit: %WorkspaceEdit{ changes: %{ uri => [ %TextEdit{ - new_text: indentation <> require_module, + new_text: indentation <> "require #{require_module}\n", range: range } ] @@ -72,7 +71,7 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do @module_name ~r/require\s+([^\s]+)\s+before/ defp get_edit(message) do case Regex.run(@module_name, message) do - [_, module] -> {:ok, "require #{module}\n"} + [_, module] -> {:ok, module} _ -> {:error, "unable to find require"} end end @@ -119,15 +118,4 @@ defmodule NextLS.ElixirExtension.CodeAction.Require do context end end - - defp get_module_name({:defmodule, _, [{:__aliases__, _, alias} | _rest]}) do - name = - alias - |> Module.concat() - |> Macro.to_string() - - {:ok, name} - end - - defp get_module_name(_), do: {:error, "expected defmodule ast"} end diff --git a/test/next_ls/extensions/elixir_extension/code_action/require_test.exs b/test/next_ls/extensions/elixir_extension/code_action/require_test.exs index cdc9861f..39e5942b 100644 --- a/test/next_ls/extensions/elixir_extension/code_action/require_test.exs +++ b/test/next_ls/extensions/elixir_extension/code_action/require_test.exs @@ -38,7 +38,7 @@ defmodule NextLS.ElixirExtension.RequireTest do assert [code_action] = Require.new(diagnostic, text, uri) assert is_struct(code_action, CodeAction) assert [diagnostic] == code_action.diagnostics - assert code_action.title =~ "Test.Require" + assert code_action.title == "Add missing require for Logger" assert %WorkspaceEdit{ changes: %{ @@ -83,7 +83,7 @@ defmodule NextLS.ElixirExtension.RequireTest do assert [code_action] = Require.new(diagnostic, text, uri) assert is_struct(code_action, CodeAction) assert [diagnostic] == code_action.diagnostics - assert code_action.title =~ "Test.Require" + assert code_action.title == "Add missing require for Logger" assert %WorkspaceEdit{ changes: %{ @@ -130,7 +130,7 @@ defmodule NextLS.ElixirExtension.RequireTest do assert [code_action] = Require.new(diagnostic, text, uri) assert is_struct(code_action, CodeAction) assert [diagnostic] == code_action.diagnostics - assert code_action.title =~ "Test.Require" + assert code_action.title == "Add missing require for Logger" assert %WorkspaceEdit{ changes: %{ @@ -186,7 +186,7 @@ defmodule NextLS.ElixirExtension.RequireTest do assert [code_action] = Require.new(diagnostic, text, uri) assert is_struct(code_action, CodeAction) assert [diagnostic] == code_action.diagnostics - assert code_action.title =~ "Require" + assert code_action.title == "Add missing require for Logger" assert %WorkspaceEdit{ changes: %{