From c976841e7b08fd1e483243112d50d45dd89faaaa Mon Sep 17 00:00:00 2001 From: robmckinnon Date: Fri, 20 Oct 2023 13:00:16 +0200 Subject: [PATCH] Implement extract function code mod --- .../code_mod/refactor_extract_function.ex | 18 -- .../execute_command/extract_function.ex | 28 +- .../code_mod_extract_function.ex | 250 ++++++++++++++++ apps/language_server/mix.exs | 1 + .../code_mod_extract_function_test.exs | 280 ++++++++++++++++++ mix.lock | 1 + 6 files changed, 549 insertions(+), 29 deletions(-) delete mode 100644 apps/language_server/lib/language_server/experimental/code_mod/refactor_extract_function.ex create mode 100644 apps/language_server/lib/language_server/providers/execute_command/extract_function/code_mod_extract_function.ex create mode 100644 apps/language_server/test/providers/execute_command/extract_function/code_mod_extract_function_test.exs diff --git a/apps/language_server/lib/language_server/experimental/code_mod/refactor_extract_function.ex b/apps/language_server/lib/language_server/experimental/code_mod/refactor_extract_function.ex deleted file mode 100644 index 128d7d781..000000000 --- a/apps/language_server/lib/language_server/experimental/code_mod/refactor_extract_function.ex +++ /dev/null @@ -1,18 +0,0 @@ -defmodule ElixirLS.LanguageServer.Experimental.CodeMod.RefactorExtractFunction do - alias ElixirLS.LanguageServer.Experimental.CodeMod.Diff - alias ElixirLS.LanguageServer.Experimental.CodeMod.ExtractFunction - - alias Sourceror.Zipper - - require Logger - - def text_edits(original_text, tree, start_line, end_line, new_function_name) do - result = - tree - |> Zipper.zip() - |> ExtractFunction.extract_function(start_line + 1, end_line + 1, new_function_name) - |> Sourceror.to_string() - - {:ok, Diff.diff(original_text, result)} - end -end diff --git a/apps/language_server/lib/language_server/providers/execute_command/extract_function.ex b/apps/language_server/lib/language_server/providers/execute_command/extract_function.ex index 22d1f6e45..21a897a66 100644 --- a/apps/language_server/lib/language_server/providers/execute_command/extract_function.ex +++ b/apps/language_server/lib/language_server/providers/execute_command/extract_function.ex @@ -5,9 +5,10 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction do """ alias ElixirLS.LanguageServer.JsonRpc + alias ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction.CodeModExtractFunction alias ElixirLS.LanguageServer.Server - alias ElixirLS.LanguageServer.Experimental.CodeMod.Ast - alias ElixirLS.LanguageServer.Experimental.CodeMod.RefactorExtractFunction + + alias VendoredSourceror.Zipper require Logger @@ -16,15 +17,9 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction do @impl ElixirLS.LanguageServer.Providers.ExecuteCommand def execute([uri, start_line, end_line, new_function_name], state) do with source_file <- Server.get_source_file(state, uri), - {:ok, tree} <- Ast.from(source_file.text, include_comments: true), + {:ok, tree} <- VendoredSourceror.parse_string(source_file.text), {:ok, text_edits} <- - RefactorExtractFunction.text_edits( - source_file.text, - tree, - start_line, - end_line, - new_function_name - ) do + text_edits(source_file.text, tree, start_line, end_line, new_function_name) do apply_edits(uri, text_edits) {:ok, nil} end @@ -34,6 +29,17 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction do JsonRpc.send_request("workspace/applyEdit", %{ "label" => "Extract function", "edit" => %{"changes" => %{uri => text_edits}} - }) |> IO.inspect(label: :rpc_response, limit: :infinity) + }) + |> IO.inspect(label: :rpc_response, limit: :infinity) + end + + def text_edits(original_text, tree, start_line, end_line, new_function_name) do + result = + tree + |> Zipper.zip() + |> CodeModExtractFunction.extract_function(start_line + 1, end_line + 1, new_function_name) + |> VendoredSourceror.to_string() + + {:ok, Diff.diff(original_text, result)} end end diff --git a/apps/language_server/lib/language_server/providers/execute_command/extract_function/code_mod_extract_function.ex b/apps/language_server/lib/language_server/providers/execute_command/extract_function/code_mod_extract_function.ex new file mode 100644 index 000000000..6c1b88a0f --- /dev/null +++ b/apps/language_server/lib/language_server/providers/execute_command/extract_function/code_mod_extract_function.ex @@ -0,0 +1,250 @@ +defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction.CodeModExtractFunction do + @moduledoc """ + Elixir refactoring functions. + """ + + alias VendoredSourceror.Zipper, as: Z + + @doc """ + Return zipper containing AST with extracted function. + """ + def extract_function(zipper, start_line, end_line, function_name) + when is_binary(function_name) do + extract_function(zipper, start_line, end_line, String.to_atom(function_name)) + end + + def extract_function(%Z{} = zipper, start_line, end_line, function_name) + when is_integer(start_line) and is_integer(end_line) and is_atom(function_name) do + {quoted_after_extract, acc} = extract_lines(zipper, start_line, end_line, function_name) + + if Enum.empty?(acc.lines) do + {:error, :not_extractable} + else + new_function_zipper = new_function(function_name, [], acc.lines) |> Z.zip() + declared_vars = vars_declared(new_function_zipper) |> Enum.uniq() + used_vars = vars_used(new_function_zipper) |> Enum.uniq() + + args = used_vars -- declared_vars + returns = declared_vars |> Enum.filter(&(&1 in acc.vars)) + + {zipper, extracted} = + add_returned_vars(Z.zip(quoted_after_extract), returns, function_name, args, acc.lines) + + enclosing = acc.def + + zipper + |> top_find(fn + {:def, _meta, [{^enclosing, _, _}, _]} -> true + _ -> false + end) + |> Z.insert_right(extracted) + |> fix_block() + |> Z.root() + end + end + + @doc """ + Return zipper containing AST for lines in the range from-to. + """ + def extract_lines(%Z{} = zipper, start_line, end_line, replace_with \\ nil) do + remove_range(zipper, start_line, end_line, %{ + lines: [], + def: nil, + def_end: nil, + vars: [], + replace_with: replace_with + }) + end + + defp next_remove_range(%Z{} = zipper, from, to, acc) do + next = Z.next(zipper) + + if is_nil(next) || next.node == true do + # return zipper with lines removed + { + Z.top(zipper).node, + %{acc | lines: Enum.reverse(acc.lines), vars: Enum.reverse(acc.vars)} + } + else + remove_range(next, from, to, acc) + end + end + + defp remove_range(%Z{node: {:def, meta, [{marker, _, _}, _]}} = zipper, from, to, acc) do + acc = + if meta[:line] < from do + x = put_in(acc.def, marker) + put_in(x.def_end, meta[:end][:line]) + else + acc + end + + next_remove_range(zipper, from, to, acc) + end + + defp remove_range(%Z{node: {marker, meta, children}} = zipper, from, to, acc) do + if meta[:line] < from || meta[:line] > to || marker == :__block__ do + next_remove_range( + zipper, + from, + to, + if meta[:line] > to && meta[:line] < acc.def_end && is_atom(marker) && is_nil(children) do + put_in(acc.vars, [marker | acc.vars] |> Enum.uniq()) + else + acc + end + ) + else + acc = put_in(acc.lines, [Z.node(zipper) | acc.lines]) + + if is_nil(acc.replace_with) do + zipper + |> Z.remove() + |> next_remove_range(from, to, acc) + else + function_name = acc.replace_with + acc = put_in(acc.replace_with, nil) + + zipper + |> Z.replace({function_name, [], []}) + |> next_remove_range(from, to, acc) + end + end + end + + defp remove_range(%Z{} = zipper, from, to, acc) do + next_remove_range(zipper, from, to, acc) + end + + defp vars_declared(%Z{} = new_function_zipper) do + vars_declared(new_function_zipper, %{vars: []}) + end + + defp vars_declared(nil, acc) do + Enum.reverse(acc.vars) + end + + defp vars_declared(%Z{node: {:=, _, [{var, _, nil}, _]}} = zipper, acc) + when is_atom(var) do + zipper + |> Z.next() + |> vars_declared(put_in(acc.vars, [var | acc.vars])) + end + + defp vars_declared(%Z{} = zipper, acc) do + zipper + |> Z.next() + |> vars_declared(acc) + end + + defp vars_used(%Z{} = new_function_zipper) do + vars_used(new_function_zipper, %{vars: []}) + end + + defp vars_used(nil, acc) do + Enum.reverse(acc.vars) + end + + defp vars_used(%Z{node: {marker, _meta, nil}} = zipper, acc) + when is_atom(marker) do + zipper + |> Z.next() + |> vars_used(put_in(acc.vars, [marker | acc.vars])) + end + + defp vars_used(%Z{} = zipper, acc) do + zipper + |> Z.next() + |> vars_used(acc) + end + + defp add_returned_vars(%Z{} = zipper, _returns = [], function_name, args, lines) do + args = var_ast(args) + + { + replace_function_call(zipper, function_name, {function_name, [], args}), + new_function(function_name, args, lines) + } + end + + defp add_returned_vars(%Z{} = zipper, returns, function_name, args, lines) + when is_list(returns) do + args = var_ast(args) + returned_vars = returned(returns) + + { + replace_function_call( + zipper, + function_name, + {:=, [], [returned_vars, {function_name, [], args}]} + ), + new_function(function_name, args, Enum.concat(lines, [returned_vars])) + } + end + + defp var_ast(vars) when is_list(vars) do + Enum.map(vars, &var_ast/1) + end + + defp var_ast(var) when is_atom(var) do + {var, [], nil} + end + + defp returned([var]) when is_atom(var) do + var_ast(var) + end + + defp returned(vars) when is_list(vars) do + returned = vars |> var_ast() |> List.to_tuple() + {:__block__, [], [returned]} + end + + defp replace_function_call(%Z{} = zipper, function_name, replace_with) do + zipper + |> top_find(fn + {^function_name, [], []} -> true + _ -> false + end) + |> Z.replace(replace_with) + end + + defp new_function(function_name, args, lines) do + {:def, [do: [], end: []], + [ + {function_name, [], args}, + [ + { + {:__block__, [], [:do]}, + {:__block__, [], lines} + } + ] + ]} + end + + defp fix_block(%Z{} = zipper) do + zipper + |> top_find(fn + {:{}, [], _children} -> true + _ -> false + end) + |> case do + nil -> + zipper + + %Z{node: {:{}, [], [block | defs]}, path: meta} -> + %Z{ + node: { + block, + {:__block__, [], defs} + }, + path: meta + } + end + end + + defp top_find(zipper, function) do + zipper + |> Z.top() + |> Z.find(function) + end +end diff --git a/apps/language_server/mix.exs b/apps/language_server/mix.exs index cff0e3af9..ada80e76d 100644 --- a/apps/language_server/mix.exs +++ b/apps/language_server/mix.exs @@ -46,6 +46,7 @@ defmodule ElixirLS.LanguageServer.Mixfile do {:stream_data, "~> 0.5", only: [:dev, :test], runtime: false}, {:path_glob_vendored, github: "elixir-lsp/path_glob", ref: @dep_versions[:path_glob_vendored]}, + {:diff, "~> 1.1"}, {:patch, "~> 0.12.0", only: [:dev, :test], runtime: false}, {:benchee, "~> 1.0", only: :dev, runtime: false} ] diff --git a/apps/language_server/test/providers/execute_command/extract_function/code_mod_extract_function_test.exs b/apps/language_server/test/providers/execute_command/extract_function/code_mod_extract_function_test.exs new file mode 100644 index 000000000..96d7c0160 --- /dev/null +++ b/apps/language_server/test/providers/execute_command/extract_function/code_mod_extract_function_test.exs @@ -0,0 +1,280 @@ +defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction.CodeModExtractFunctionTest do + alias ElixirLS.LanguageServer.Providers.ExecuteCommand.ExtractFunction.CodeModExtractFunction + + use ExUnit.Case + alias VendoredSourceror.Zipper, as: Z + + setup ctx do + if Map.has_key?(ctx, :no_setup) do + {:ok, []} + else + no = Map.get(ctx, :no, "") + + {:ok, + quoted: + """ + defmodule Baz#{no} do + def foo(one, two) do + three = 3 + IO.inspect(one) + IO.inspect(two) + IO.inspect(three) + four = 4 + IO.inspect(three) + + IO.inspect(four: four, + force_format_on_new_line_with_really_long_atom: true + ) + + # comment + end + end + """ + |> VendoredSourceror.parse_string!()} + end + end + + describe "extract_function" do + @tag no: 1 + test "extract one line to function", %{quoted: quoted} do + zipper = CodeModExtractFunction.extract_function(Z.zip(quoted), 3, 3, "bar") + source = VendoredSourceror.to_string(zipper) + + assert [ + "defmodule Baz1 do", + " def foo(one, two) do", + " three = bar()", + " IO.inspect(one)", + " IO.inspect(two)", + " IO.inspect(three)", + " four = 4", + " IO.inspect(three)", + "", + " IO.inspect(", + " four: four,", + " force_format_on_new_line_with_really_long_atom: true", + " )", + "", + " # comment", + " end", + "", + " def bar() do", + " three = 3", + " three", + " end", + "end" + ] == + source |> String.split("\n") + + Code.eval_string(source) + end + + @tag no: 2 + test "extract multiple lines to function", %{quoted: quoted} do + zipper = CodeModExtractFunction.extract_function(Z.zip(quoted), 3, 4, :bar) + source = VendoredSourceror.to_string(zipper) + + assert [ + "defmodule Baz2 do", + " def foo(one, two) do", + " three = bar(one)", + " IO.inspect(two)", + " IO.inspect(three)", + " four = 4", + " IO.inspect(three)", + "", + " IO.inspect(", + " four: four,", + " force_format_on_new_line_with_really_long_atom: true", + " )", + "", + " # comment", + " end", + "", + " def bar(one) do", + " three = 3", + " IO.inspect(one)", + " three", + " end", + "end" + ] == + source |> String.split("\n") + + Code.eval_string(source) + end + + @tag no: 3 + test "extract multiple lines with multiple returns to function", %{quoted: quoted} do + zipper = CodeModExtractFunction.extract_function(Z.zip(quoted), 3, 7, :bar) + source = VendoredSourceror.to_string(zipper) + + assert [ + "defmodule Baz3 do", + " def foo(one, two) do", + " {three, four} = bar(one, two)", + " IO.inspect(three)", + "", + " IO.inspect(", + " four: four,", + " force_format_on_new_line_with_really_long_atom: true", + " )", + "", + " # comment", + " end", + "", + " def bar(one, two) do", + " three = 3", + " IO.inspect(one)", + " IO.inspect(two)", + " IO.inspect(three)", + " four = 4", + " {three, four}", + " end", + "end" + ] == + source |> String.split("\n") + + Code.eval_string(source) + end + + @tag no: 4 + test "extract multiple lines with single return value to function", %{quoted: quoted} do + zipper = CodeModExtractFunction.extract_function(Z.zip(quoted), 3, 8, :bar) + source = VendoredSourceror.to_string(zipper) + + assert [ + "defmodule Baz4 do", + " def foo(one, two) do", + " four = bar(one, two)", + "", + " IO.inspect(", + " four: four,", + " force_format_on_new_line_with_really_long_atom: true", + " )", + "", + " # comment", + " end", + "", + " def bar(one, two) do", + " three = 3", + " IO.inspect(one)", + " IO.inspect(two)", + " IO.inspect(three)", + " four = 4", + " IO.inspect(three)", + "", + " four", + " end", + "end" + ] == + source |> String.split("\n") + + Code.eval_string(source) + end + + @tag no: 5 + test "extracts when extract partial function call", %{quoted: quoted} do + zipper = CodeModExtractFunction.extract_function(Z.zip(quoted), 10, 10, :bar) + source = VendoredSourceror.to_string(zipper) + + assert [ + "defmodule Baz5 do", + " def foo(one, two) do", + " three = 3", + " IO.inspect(one)", + " IO.inspect(two)", + " IO.inspect(three)", + " four = 4", + " IO.inspect(three)", + "", + " bar(four)", + "", + " # comment", + " end", + "", + " def bar(four) do", + " IO.inspect(", + " four: four,", + " force_format_on_new_line_with_really_long_atom: true", + " )", + " end", + "end" + ] == + source |> String.split("\n") + + Code.eval_string(source) + end + + @tag no: 6 + test "errors when extract on second line of multi-line function call", %{quoted: quoted} do + {:error, :not_extractable} = + CodeModExtractFunction.extract_function(Z.zip(quoted), 11, 11, :bar) + end + end + + describe "extract_lines/3" do + @tag no: 20 + test "extract one line to function", %{quoted: quoted} do + {zipper, lines} = CodeModExtractFunction.extract_lines(Z.zip(quoted), 3, 3) + + assert "defmodule Baz20 do\n def foo(one, two) do\n IO.inspect(one)\n IO.inspect(two)\n IO.inspect(three)\n four = 4\n IO.inspect(three)\n\n IO.inspect(\n four: four,\n force_format_on_new_line_with_really_long_atom: true\n )\n\n # comment\n end\nend" == + VendoredSourceror.to_string(zipper) + + assert [ + "{:def, :foo}", + "{:def_end, 15}", + "{:lines, [three = 3]}", + _, + "{:vars, [:one, :two, :three, :four]}" + ] = lines |> Enum.map(&VendoredSourceror.to_string(&1)) + end + + @tag no: 21 + test "extract multiple lines to function", %{quoted: quoted} do + {zipper, lines} = CodeModExtractFunction.extract_lines(Z.zip(quoted), 3, 4) + + assert "defmodule Baz21 do\n def foo(one, two) do\n IO.inspect(two)\n IO.inspect(three)\n four = 4\n IO.inspect(three)\n\n IO.inspect(\n four: four,\n force_format_on_new_line_with_really_long_atom: true\n )\n\n # comment\n end\nend" = + VendoredSourceror.to_string(zipper) + + assert [ + "{:def, :foo}", + "{:def_end, 15}", + "{:lines, [three = 3, IO.inspect(one)]}", + _, + "{:vars, [:two, :three, :four]}" + ] = lines |> Enum.map(&VendoredSourceror.to_string(&1)) + end + + @tag no: 22 + test "extract multi-line function call to function", %{quoted: quoted} do + {zipper, lines} = CodeModExtractFunction.extract_lines(Z.zip(quoted), 10, 10) + + assert "defmodule Baz22 do\n def foo(one, two) do\n three = 3\n IO.inspect(one)\n IO.inspect(two)\n IO.inspect(three)\n four = 4\n IO.inspect(three)\n\n # comment\n end\nend" = + VendoredSourceror.to_string(zipper) + + assert [ + "{:def, :foo}", + "{:def_end, 15}", + "{:lines,\n [\n IO.inspect(\n four: four,\n force_format_on_new_line_with_really_long_atom: true\n )\n ]}", + "{:replace_with, nil}", + "{:vars, []}" + ] = lines |> Enum.map(&VendoredSourceror.to_string(&1)) + end + + @tag no: 23 + test "noop when second line of multi-line function call", %{quoted: quoted} do + {zipper, lines} = CodeModExtractFunction.extract_lines(Z.zip(quoted), 11, 11) + + assert "defmodule Baz23 do\n def foo(one, two) do\n three = 3\n IO.inspect(one)\n IO.inspect(two)\n IO.inspect(three)\n four = 4\n IO.inspect(three)\n\n IO.inspect(\n four: four,\n force_format_on_new_line_with_really_long_atom: true\n )\n\n # comment\n end\nend" = + VendoredSourceror.to_string(zipper) + + assert [ + "{:def, :foo}", + "{:def_end, 15}", + "{:lines, []}", + "{:replace_with, nil}", + "{:vars, []}" + ] = lines |> Enum.map(&VendoredSourceror.to_string(&1)) + end + end +end diff --git a/mix.lock b/mix.lock index 980a8048d..ff73eb620 100644 --- a/mix.lock +++ b/mix.lock @@ -2,6 +2,7 @@ "benchee": {:hex, :benchee, "1.1.0", "f3a43817209a92a1fade36ef36b86e1052627fd8934a8b937ac9ab3a76c43062", [:mix], [{:deep_merge, "~> 1.0", [hex: :deep_merge, repo: "hexpm", optional: false]}, {:statistex, "~> 1.0", [hex: :statistex, repo: "hexpm", optional: false]}], "hexpm", "7da57d545003165a012b587077f6ba90b89210fd88074ce3c60ce239eb5e6d93"}, "deep_merge": {:hex, :deep_merge, "1.0.0", "b4aa1a0d1acac393bdf38b2291af38cb1d4a52806cf7a4906f718e1feb5ee961", [:mix], [], "hexpm", "ce708e5f094b9cd4e8f2be4f00d2f4250c4095be93f8cd6d018c753894885430"}, "dialyxir_vendored": {:git, "https://github.com/elixir-lsp/dialyxir.git", "d50dcd7101c6ebd37b57b7ee4a7888d8cb634782", [ref: "d50dcd7101c6ebd37b57b7ee4a7888d8cb634782"]}, + "diff": {:hex, :diff, "1.1.0", "7b91b77aa5eca71cb361a97d36f52e4e4a0e17b1a0a8045decd4aba3ec657982", [:mix], [], "hexpm", "3364bf94be8b73c9c42464200ebac563023972795d1d6e4adecf4216c9ae685a"}, "elixir_sense": {:git, "https://github.com/elixir-lsp/elixir_sense.git", "497d9b797cb7032fcb9f2c0fc7294c46c88bac32", [ref: "497d9b797cb7032fcb9f2c0fc7294c46c88bac32"]}, "erl2ex_vendored": {:git, "https://github.com/elixir-lsp/erl2ex.git", "073ac6b9a44282e718b6050c7b27cedf9217a12a", [ref: "073ac6b9a44282e718b6050c7b27cedf9217a12a"]}, "erlex_vendored": {:git, "https://github.com/elixir-lsp/erlex.git", "82db0e82ee4896491bc26dec99f5d795f03ab9f4", [ref: "82db0e82ee4896491bc26dec99f5d795f03ab9f4"]},