From fac4e30997300c20875797ae1fc8e492d660ad97 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Tue, 19 Mar 2024 14:37:34 +0800 Subject: [PATCH 01/12] Rename the file while renaming the module. It first determines whether the module meets certain hard constraints, such as having no parent, then performs some conventional checks. If both are satisfied, it proceeds to return the file renaming. --- .../lexical/remote_control/code_mod/rename.ex | 4 +- .../code_mod/rename/document_changes.ex | 12 ++ .../remote_control/code_mod/rename/file.ex | 100 +++++++++++++ .../remote_control/code_mod/rename/module.ex | 23 ++- .../remote_control/code_mod/rename_test.exs | 131 +++++++++++++++--- .../server/provider/handlers/rename.ex | 40 +++++- .../server/provider/handlers/rename_test.exs | 43 +++--- 7 files changed, 302 insertions(+), 51 deletions(-) create mode 100644 apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex create mode 100644 apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index 506f199c7..2d235f536 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -1,8 +1,8 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do alias Lexical.Ast.Analysis - alias Lexical.Document.Edit alias Lexical.Document.Position alias Lexical.Document.Range + alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias __MODULE__ @spec prepare(Analysis.t(), Position.t()) :: @@ -12,7 +12,7 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do @rename_mapping %{module: Rename.Module} @spec rename(Analysis.t(), Position.t(), String.t()) :: - {:ok, %{Lexical.uri() => [Edit.t()]}} | {:error, term()} + {:ok, [DocumentChanges.t()]} | {:error, term()} def rename(%Analysis{} = analysis, %Position{} = position, new_name) do with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do rename_module = @rename_mapping[renamable] diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex new file mode 100644 index 000000000..a3ef92ffd --- /dev/null +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex @@ -0,0 +1,12 @@ +defmodule Lexical.RemoteControl.CodeMod.Rename.DocumentChanges do + defstruct [:uri, :edits, :rename_file] + + @type t :: %__MODULE__{ + uri: Lexical.uri(), + edits: [Lexical.Document.Edit.t()], + rename_file: {Lexical.uri(), Lexical.uri()} | nil + } + def new(uri, edits, rename_file \\ nil) do + %__MODULE__{uri: uri, edits: edits, rename_file: rename_file} + end +end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex new file mode 100644 index 000000000..96a94328a --- /dev/null +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex @@ -0,0 +1,100 @@ +defmodule Lexical.RemoteControl.CodeMod.Rename.File do + alias Lexical.Ast + alias Lexical.Document + alias Lexical.RemoteControl + alias Lexical.RemoteControl.Search.Store + + def maybe_rename(entry, new_suffix) do + with false <- has_parent?(entry), + false <- has_any_siblings?(entry) do + rename_file(entry, new_suffix) + else + _ -> nil + end + end + + defp has_parent?(entry) do + case Store.parent(entry) do + {:ok, _} -> true + _ -> false + end + end + + defp has_any_siblings?(entry) do + case Store.siblings(entry) do + {:ok, [_]} -> false + {:ok, [_ | _]} -> true + _ -> false + end + end + + defp rename_file(entry, new_suffix) do + root_path = root_path() + relative_path = relative_path(entry.path, root_path) + + with {:ok, prefix} <- fetch_conventional_prefix(relative_path), + {:ok, new_name} <- fetch_new_name(entry, new_suffix) do + extname = Path.extname(entry.path) + suffix = Macro.underscore(new_name) + new_path = Path.join([root_path, prefix, "#{suffix}#{extname}"]) + + {Document.Path.ensure_uri(entry.path), Document.Path.ensure_uri(new_path)} + else + _ -> nil + end + end + + defp relative_path(path, root_path) do + Path.relative_to(path, root_path) + end + + defp root_path do + RemoteControl.get_project() + |> Map.get(:root_uri) + |> Document.Path.ensure_path() + end + + defp fetch_new_name(entry, new_suffix) do + uri = Document.Path.ensure_uri(entry.path) + text_edits = [Document.Edit.new(new_suffix, entry.range)] + + with {:ok, document} <- Document.Store.open_temporary(uri), + {:ok, edited_document} = + Document.apply_content_changes(document, document.version + 1, text_edits), + {:ok, %{context: {:alias, alias}}} <- + Ast.surround_context(edited_document, entry.range.start) do + {:ok, to_string(alias)} + else + _ -> :error + end + end + + defp fetch_conventional_prefix(path) do + result = + path + |> Path.split() + |> Enum.chunk_every(2, 2) + |> Enum.reduce({[], []}, fn + ["apps", app_name], _ -> + {[], [app_name, "apps"]} + + ["lib", follow_element], {elements, prefix} -> + {[follow_element | elements], ["lib" | prefix]} + + ["test", follow_element], {elements, prefix} -> + {[follow_element | elements], ["test" | prefix]} + + remain, {elements, prefix} -> + {remain ++ elements, prefix} + end) + + case result do + {_, []} -> + :error + + {_module_path, prefix} -> + prefix = prefix |> Enum.reverse() |> Enum.join("/") + {:ok, prefix} + end + end +end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex index 616e7d8a9..8ea35c2b3 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex @@ -6,21 +6,25 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.Module do alias Lexical.Document.Position alias Lexical.Document.Range alias Lexical.RemoteControl.CodeIntelligence.Entity + alias Lexical.RemoteControl.CodeMod.Rename + alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias Lexical.RemoteControl.Search.Store require Logger import Line - @spec rename(Range.t(), String.t(), atom()) :: %{Lexical.uri() => [Edit.t()]} + @spec rename(Range.t(), String.t(), atom()) :: [DocumentChanges.t()] def rename(%Range{} = old_range, new_name, entity) do {old_suffix, new_suffix} = old_range |> range_text() |> diff(new_name) results = exacts(entity, old_suffix) ++ descendants(entity, old_suffix) - Enum.group_by( - results, - &Document.Path.ensure_uri(&1.path), - &Edit.new(new_suffix, &1.range) - ) + results + |> Enum.group_by(&Document.Path.ensure_uri(&1.path)) + |> Enum.map(fn {uri, entries} -> + rename_file = maybe_rename_file(entries, new_suffix) + edits = Enum.map(entries, &Edit.new(new_suffix, &1.range)) + DocumentChanges.new(uri, edits, rename_file) + end) end @spec resolve(Analysis.t() | Lexical.path(), Position.t()) :: @@ -77,6 +81,13 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.Module do |> adjust_range_for_descendants(entity, old_suffix) end + defp maybe_rename_file(entries, new_suffix) do + entries + |> Enum.map(&Rename.File.maybe_rename(&1, new_suffix)) + # every group should have only one `rename_file` + |> Enum.find(&(not is_nil(&1))) + end + defp entry_matching?(entry, old_suffix) do entry.range |> range_text() |> String.contains?(old_suffix) end diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs index 98af234d0..9eca2714f 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs @@ -1,6 +1,5 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do alias Lexical.Document - alias Lexical.Project alias Lexical.RemoteControl alias Lexical.RemoteControl.CodeMod.Rename alias Lexical.RemoteControl.Search @@ -40,16 +39,6 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do {:ok, project: project} end - setup %{project: project} do - uri = subject_uri(project) - - on_exit(fn -> - Document.Store.close(uri) - end) - - %{uri: uri} - end - describe "prepare/2" do test "returns the module name" do {:ok, result, _} = @@ -71,7 +60,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do assert result == "TopLevel.Foo" end - test "returns the whole module name even if the cusor is not at the end" do + test "returns the whole module name even if the cursor is not at the end" do {:ok, result, _} = ~q[ defmodule Top|Level.Foo do @@ -217,7 +206,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end end - describe "rename descendants" do + describe "rename module descendants" do test "rename the descendants" do {:ok, result} = ~q[ defmodule TopLevel.|Module do @@ -341,8 +330,86 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end end - defp rename(%Project{} = project \\ project(), source, new_name) do - uri = subject_uri(project) + describe "rename file" do + test "it shouldn't rename file if the module has parent module within that file" do + {:ok, {_applied, nil}} = + ~q[ + defmodule FooServer do + defmodule |State do + end + end + ] |> rename("Renamed", "lib/foo_server.ex") + end + + test "it shouldn't rename file if the module has any siblings within that file" do + assert {:ok, {_applied, nil}} = + ~q[ + defmodule |Foo do + end + + defmodule Bar do + end + ] |> rename("Renamed", "lib/foo.ex") + end + + test "it shouldn't rename file if the path doesn't match the any convensions" do + assert {:ok, {_applied, nil}} = + ~q[ + defmodule |Foo.Mix do + end + ] |> rename("Renamed", "mix.ex") + end + + test "succeeds when the path matching the `lib/*` convension", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |Foo do + end + ] |> rename("Renamed", "lib/foo.ex") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "lib/renamed.ex") + end + + test "succeeds when the path matching the `apps/*` convension", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |FooApp.Bar do + end + ] |> rename("FooApp.Renamed", "apps/foo_app/lib/foo_app/bar.ex") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "apps/foo_app/lib/foo_app/renamed.ex") + end + + test "succeeds when the path matching the `apps/*` convension with nested folders", %{ + project: project + } do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |Lexical.RemoteControl do + end + ] |> rename("Lexical.RemoteChaos", "apps/remote_control/lib/lexical/remote_control.ex") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "apps/remote_control/lib/lexical/remote_chaos.ex") + end + + test "succeeds when the path matching the `test/*` convension", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |FooTest do + end + ] |> rename("RenamedTest", "test/foo_test.exs") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "test/renamed_test.exs") + end + end + + defp rename(source, new_name, path \\ nil) do + project = project() + uri = subject_uri(project, path) with {position, text} <- pop_cursor(source), {:ok, document} <- open_document(uri, text), @@ -350,12 +417,23 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do :ok <- Search.Store.replace(entries), analysis = Lexical.Ast.analyze(document), {:ok, uri_with_changes} <- Rename.rename(analysis, position, new_name) do - changes = uri_with_changes |> Map.values() |> List.flatten() - {:ok, apply_edits(document, changes)} + changes = uri_with_changes |> Enum.map(& &1.edits) |> List.flatten() + applied = apply_edits(document, changes) + + result = + if path do + rename_file = uri_with_changes |> Enum.map(& &1.rename_file) |> List.first() + {applied, rename_file} + else + applied + end + + {:ok, result} end end - defp prepare(project \\ project(), code) do + defp prepare(code) do + project = project() uri = subject_uri(project) with {position, text} <- pop_cursor(code), @@ -368,10 +446,19 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end end - defp subject_uri(project) do - project - |> file_path(Path.join("lib", "project.ex")) - |> Document.Path.ensure_uri() + defp subject_uri(project, path \\ nil) do + path = path || Path.join("wont_rename_file_folder", "project.ex") + + uri = + project + |> file_path(path) + |> Document.Path.ensure_uri() + + on_exit(fn -> + Document.Store.close(uri) + end) + + uri end defp open_document(uri, content) do diff --git a/apps/server/lib/lexical/server/provider/handlers/rename.ex b/apps/server/lib/lexical/server/provider/handlers/rename.ex index 31abbaced..765b455b7 100644 --- a/apps/server/lib/lexical/server/provider/handlers/rename.ex +++ b/apps/server/lib/lexical/server/provider/handlers/rename.ex @@ -3,8 +3,11 @@ defmodule Lexical.Server.Provider.Handlers.Rename do alias Lexical.Document alias Lexical.Protocol.Requests.Rename alias Lexical.Protocol.Responses - alias Lexical.Protocol.Types.Workspace.Edit + alias Lexical.Protocol.Types.RenameFile + alias Lexical.Protocol.Types.TextDocument + alias Lexical.Protocol.Types.Workspace alias Lexical.RemoteControl.Api + alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias Lexical.Server.Provider.Env require Logger @@ -21,12 +24,25 @@ defmodule Lexical.Server.Provider.Handlers.Rename do defp rename(project, analysis, position, new_name, id) do case Api.rename(project, analysis, position, new_name) do - {:ok, results} when results == %{} -> + {:ok, []} -> {:reply, nil} {:ok, results} -> - edit = Edit.new(changes: results) - {:reply, Responses.Rename.new(id, edit)} + text_document_edits = + Enum.map(results, fn %DocumentChanges{edits: edits, uri: uri} -> + new_text_document_edit(uri, edits) + end) + + rename_files = + results + |> Stream.map(& &1.rename_file) + |> Stream.reject(&(&1 == nil)) + |> Enum.map(&new_rename_file/1) + + workspace_edit = + Workspace.Edit.new(document_changes: text_document_edits ++ rename_files) + + {:reply, Responses.Rename.new(id, workspace_edit)} {:error, {:unsupported_entity, entity}} -> Logger.info("Unrenameable entity: #{inspect(entity)}") @@ -36,4 +52,20 @@ defmodule Lexical.Server.Provider.Handlers.Rename do {:reply, Responses.Rename.error(id, :request_failed, inspect(reason))} end end + + defp new_text_document_edit(uri, edits) do + text_document = TextDocument.OptionalVersioned.Identifier.new(uri: uri, version: 0) + TextDocument.Edit.new(edits: edits, text_document: text_document) + end + + defp new_rename_file({from_uri, to_uri}) do + options = RenameFile.Options.new(overwrite: true) + + RenameFile.new( + kind: "rename", + new_uri: to_uri, + old_uri: from_uri, + options: options + ) + end end diff --git a/apps/server/test/lexical/server/provider/handlers/rename_test.exs b/apps/server/test/lexical/server/provider/handlers/rename_test.exs index f4cd62703..ffe160056 100644 --- a/apps/server/test/lexical/server/provider/handlers/rename_test.exs +++ b/apps/server/test/lexical/server/provider/handlers/rename_test.exs @@ -4,6 +4,7 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do alias Lexical.Proto.Convert alias Lexical.Protocol.Requests.Rename alias Lexical.RemoteControl + alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias Lexical.Server alias Lexical.Server.Provider.Env @@ -79,28 +80,36 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name -> {:ok, - %{ - "file:///path/to/file.ex" => [ - %{ - new_text: "new_text", - range: %{start: %{line: 1, character: 5}, end: %{line: 1, character: 10}} - } - ] - }} + [ + DocumentChanges.new( + "file:///path/to/file.ex", + [ + %{ + new_text: "new_text", + range: %{start: %{line: 1, character: 5}, end: %{line: 1, character: 10}} + } + ], + {"file:///path/to/file.ex", "file:///path/to/new_text.ex"} + ) + ]} end) {:ok, request} = build_request(uri, 1, 5) assert {:reply, response} = handle(request, project) - - assert response.result.changes == %{ - "file:///path/to/file.ex" => [ - %{ - new_text: "new_text", - range: %{end: %{character: 10, line: 1}, start: %{character: 5, line: 1}} - } - ] - } + [edit, rename_file] = response.result.document_changes + + assert edit.edits == [ + %{ + new_text: "new_text", + range: %{end: %{character: 10, line: 1}, start: %{character: 5, line: 1}} + } + ] + + assert edit.text_document.uri == "file:///path/to/file.ex" + assert edit.text_document.version == 0 + assert rename_file.old_uri == "file:///path/to/file.ex" + assert rename_file.new_uri == "file:///path/to/new_text.ex" end end end From 795f7a6da18ee567fc5eee370ab17e3b264f4835 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Thu, 21 Mar 2024 15:58:35 +0800 Subject: [PATCH 02/12] Maybe insert special folder for PhoenixWeb's module --- .../remote_control/code_mod/rename/file.ex | 73 ++++++++++++++++++- .../remote_control/code_mod/rename_test.exs | 69 ++++++++++++++++++ 2 files changed, 138 insertions(+), 4 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex index 96a94328a..d03790d67 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex @@ -32,10 +32,15 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do root_path = root_path() relative_path = relative_path(entry.path, root_path) - with {:ok, prefix} <- fetch_conventional_prefix(relative_path), + with {:ok, prefix, old_module_paths} <- fetch_conventional_prefix(relative_path), {:ok, new_name} <- fetch_new_name(entry, new_suffix) do extname = Path.extname(entry.path) - suffix = Macro.underscore(new_name) + + suffix = + new_name + |> Macro.underscore() + |> maybe_insert_special_phoenix_folder(old_module_paths, new_name) + new_path = Path.join([root_path, prefix, "#{suffix}#{extname}"]) {Document.Path.ensure_uri(entry.path), Document.Path.ensure_uri(new_path)} @@ -92,9 +97,69 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do {_, []} -> :error - {_module_path, prefix} -> + {module_paths, prefix} -> prefix = prefix |> Enum.reverse() |> Enum.join("/") - {:ok, prefix} + {:ok, prefix, module_paths} end end + + defp maybe_insert_special_phoenix_folder(suffix, old_module_paths, new_name) do + web_app? = new_name |> String.split(".") |> hd() |> String.ends_with?("Web") + + insertions = + cond do + not web_app? -> + "" + + phoenix_component?(old_module_paths, new_name) -> + "components" + + phoenix_controller?(old_module_paths, new_name) -> + "controllers" + + phoenix_live_view?(old_module_paths, new_name) -> + "live" + + true -> + "" + end + + suffix + |> String.split("/") + |> List.insert_at(1, insertions) + |> Enum.reject(&(&1 == "")) + |> Enum.join("/") + end + + defp phoenix_component?(old_module_paths, new_name) do + under_components? = "components" in old_module_paths + component? = String.ends_with?(new_name, ["Components", "Layouts", "Component"]) + + under_components? and component? + end + + defp phoenix_controller?(old_module_paths, new_name) do + under_controllers? = "controllers" in old_module_paths + controller? = String.ends_with?(new_name, ["Controller", "JSON", "HTML"]) + + under_controllers? and controller? + end + + defp phoenix_live_view?(old_module_paths, new_name) do + under_live_views? = "live" in old_module_paths + new_name_list = String.split(new_name, ".") + + live_view? = + if match?([_, _ | _], new_name_list) do + parent = Enum.at(new_name_list, -2) + local_module = Enum.at(new_name_list, -1) + + # `LiveDemoWeb.SomeLive` or `LiveDemoWeb.SomeLive.Index` + String.ends_with?(parent, "Live") or String.ends_with?(local_module, "Live") + else + false + end + + under_live_views? and live_view? + end end diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs index 9eca2714f..b23d066b1 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs @@ -405,6 +405,75 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do assert {_, to_uri} = rename_file assert to_uri == subject_uri(project, "test/renamed_test.exs") end + + test "leaves the `components` folder as is when renaming the live view", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule DemoWeb.|FooComponents do + end + ] |> rename("DemoWeb.RenamedComponent", "lib/demo_web/components/foo_component.ex") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "lib/demo_web/components/renamed_component.ex") + end + + test "leaves the `components` folder as is when renaming a component", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule DemoWeb.SomeContext.|FooComponent do + end + ] + |> rename( + "DemoWeb.SomeContext.RenamedComponent", + "lib/demo_web/components/some_context/foo_component.ex" + ) + + assert {_, to_uri} = rename_file + + assert to_uri == + subject_uri(project, "lib/demo_web/components/some_context/renamed_component.ex") + end + + test "leaves the `controllers` folder as is when renaming the controller", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule DemoWeb.|FooController do + end + ] |> rename("DemoWeb.RenamedController", "lib/demo_web/controllers/foo_controller.ex") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "lib/demo_web/controllers/renamed_controller.ex") + end + + test "leaves the `controller` folder as is when renaming the `JSON` module", %{ + project: project + } do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule DemoWeb.FooController.|JSON do + end + ] + |> rename( + "DemoWeb.FooController.RenamedJSON", + "lib/demo_web/controllers/foo_controller/json.ex" + ) + + assert {_, to_uri} = rename_file + + assert to_uri == + subject_uri(project, "lib/demo_web/controllers/foo_controller/renamed_json.ex") + end + + test "leaves the `live` folder as is when renaming the live view", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule DemoWeb.|FooLive do + end + ] |> rename("DemoWeb.RenamedLive", "lib/demo_web/live/foo_live.ex") + + assert {_, to_uri} = rename_file + assert to_uri == subject_uri(project, "lib/demo_web/live/renamed_live.ex") + end end defp rename(source, new_name, path \\ nil) do From 62b6993d0727829083921d209303ed738d33626f Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Fri, 22 Mar 2024 14:48:45 +0800 Subject: [PATCH 03/12] Apply some code review changes for using `Document.Changes` Use `Document.Changes` instead of `CodeMod.Rename.DocumentChanges` Make `Document.Changes.RenameFile` a struct and use `root_module?` instead of `has_silibings?` and `has_parent?` --- .../lexical/remote_control/code_mod/rename.ex | 4 +- .../code_mod/rename/document_changes.ex | 12 ----- .../remote_control/code_mod/rename/file.ex | 47 +++++++++++-------- .../remote_control/code_mod/rename/module.ex | 25 ++++++---- .../remote_control/code_mod/rename_test.exs | 38 ++++++--------- .../server/provider/handlers/rename.ex | 11 ++--- .../server/provider/handlers/rename_test.exs | 18 ++++--- .../lib/lexical/document/changes.ex | 22 ++++++++- 8 files changed, 96 insertions(+), 81 deletions(-) delete mode 100644 apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index 2d235f536..8a6adade8 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -1,8 +1,8 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do alias Lexical.Ast.Analysis + alias Lexical.Document alias Lexical.Document.Position alias Lexical.Document.Range - alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias __MODULE__ @spec prepare(Analysis.t(), Position.t()) :: @@ -12,7 +12,7 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do @rename_mapping %{module: Rename.Module} @spec rename(Analysis.t(), Position.t(), String.t()) :: - {:ok, [DocumentChanges.t()]} | {:error, term()} + {:ok, [Document.Changes.t()]} | {:error, term()} def rename(%Analysis{} = analysis, %Position{} = position, new_name) do with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do rename_module = @rename_mapping[renamable] diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex deleted file mode 100644 index a3ef92ffd..000000000 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/document_changes.ex +++ /dev/null @@ -1,12 +0,0 @@ -defmodule Lexical.RemoteControl.CodeMod.Rename.DocumentChanges do - defstruct [:uri, :edits, :rename_file] - - @type t :: %__MODULE__{ - uri: Lexical.uri(), - edits: [Lexical.Document.Edit.t()], - rename_file: {Lexical.uri(), Lexical.uri()} | nil - } - def new(uri, edits, rename_file \\ nil) do - %__MODULE__{uri: uri, edits: edits, rename_file: rename_file} - end -end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex index d03790d67..6fed38a78 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex @@ -1,30 +1,37 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do alias Lexical.Ast alias Lexical.Document + alias Lexical.ProcessCache + alias Lexical.Project alias Lexical.RemoteControl - alias Lexical.RemoteControl.Search.Store + alias Lexical.RemoteControl.Search.Indexer + alias Lexical.RemoteControl.Search.Indexer.Entry + @spec maybe_rename(Entry.t(), String.t()) :: Document.Changes.rename_file() def maybe_rename(entry, new_suffix) do - with false <- has_parent?(entry), - false <- has_any_siblings?(entry) do + if root_module?(entry) do rename_file(entry, new_suffix) - else - _ -> nil end end - defp has_parent?(entry) do - case Store.parent(entry) do - {:ok, _} -> true - _ -> false - end - end + defp root_module?(entry) do + uri = Document.Path.ensure_uri(entry.path) - defp has_any_siblings?(entry) do - case Store.siblings(entry) do - {:ok, [_]} -> false - {:ok, [_ | _]} -> true - _ -> false + entries = + ProcessCache.trans("#{uri}-entries", 50, fn -> + with {:ok, document} <- Document.Store.open_temporary(uri), + {:ok, entries} <- + Indexer.Source.index_document(document, [Indexer.Extractors.Module]) do + entries + end + end) + + case Enum.filter(entries, &(&1.block_id == :root)) do + [root_module] -> + root_module.subject == entry.subject and root_module.block_range == entry.block_range + + _ -> + false end end @@ -43,7 +50,9 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do new_path = Path.join([root_path, prefix, "#{suffix}#{extname}"]) - {Document.Path.ensure_uri(entry.path), Document.Path.ensure_uri(new_path)} + old_uri = Document.Path.ensure_uri(entry.path) + new_uri = Document.Path.ensure_uri(new_path) + Document.Changes.RenameFile.new(old_uri, new_uri) else _ -> nil end @@ -54,9 +63,7 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do end defp root_path do - RemoteControl.get_project() - |> Map.get(:root_uri) - |> Document.Path.ensure_path() + Project.root_path(RemoteControl.get_project()) end defp fetch_new_name(entry, new_suffix) do diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex index 8ea35c2b3..06b9298a9 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex @@ -7,24 +7,22 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.Module do alias Lexical.Document.Range alias Lexical.RemoteControl.CodeIntelligence.Entity alias Lexical.RemoteControl.CodeMod.Rename - alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias Lexical.RemoteControl.Search.Store require Logger import Line - @spec rename(Range.t(), String.t(), atom()) :: [DocumentChanges.t()] + @spec rename(Range.t(), String.t(), atom()) :: [Document.Changes.t()] def rename(%Range{} = old_range, new_name, entity) do {old_suffix, new_suffix} = old_range |> range_text() |> diff(new_name) results = exacts(entity, old_suffix) ++ descendants(entity, old_suffix) - results - |> Enum.group_by(&Document.Path.ensure_uri(&1.path)) - |> Enum.map(fn {uri, entries} -> - rename_file = maybe_rename_file(entries, new_suffix) - edits = Enum.map(entries, &Edit.new(new_suffix, &1.range)) - DocumentChanges.new(uri, edits, rename_file) - end) + for {uri, entries} <- Enum.group_by(results, &Document.Path.ensure_uri(&1.path)), + result = to_document_changes(uri, entries, new_suffix), + match?({:ok, _}, result) do + {:ok, document_changes} = result + document_changes + end end @spec resolve(Analysis.t() | Lexical.path(), Position.t()) :: @@ -172,4 +170,13 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.Module do |> put_in([:start, :character], start_character) |> put_in([:end, :character], end_character) end + + defp to_document_changes(uri, entries, new_suffix) do + edits = Enum.map(entries, &Edit.new(new_suffix, &1.range)) + rename_file = maybe_rename_file(entries, new_suffix) + + with {:ok, document} <- Document.Store.open_temporary(uri) do + {:ok, Document.Changes.new(document, edits, rename_file)} + end + end end diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs index b23d066b1..f9539b316 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs @@ -367,8 +367,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("Renamed", "lib/foo.ex") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "lib/renamed.ex") + assert rename_file.new_uri == subject_uri(project, "lib/renamed.ex") end test "succeeds when the path matching the `apps/*` convension", %{project: project} do @@ -378,8 +377,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("FooApp.Renamed", "apps/foo_app/lib/foo_app/bar.ex") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "apps/foo_app/lib/foo_app/renamed.ex") + assert rename_file.new_uri == subject_uri(project, "apps/foo_app/lib/foo_app/renamed.ex") end test "succeeds when the path matching the `apps/*` convension with nested folders", %{ @@ -391,8 +389,8 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("Lexical.RemoteChaos", "apps/remote_control/lib/lexical/remote_control.ex") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "apps/remote_control/lib/lexical/remote_chaos.ex") + assert rename_file.new_uri == + subject_uri(project, "apps/remote_control/lib/lexical/remote_chaos.ex") end test "succeeds when the path matching the `test/*` convension", %{project: project} do @@ -402,8 +400,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("RenamedTest", "test/foo_test.exs") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "test/renamed_test.exs") + assert rename_file.new_uri == subject_uri(project, "test/renamed_test.exs") end test "leaves the `components` folder as is when renaming the live view", %{project: project} do @@ -413,8 +410,8 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("DemoWeb.RenamedComponent", "lib/demo_web/components/foo_component.ex") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "lib/demo_web/components/renamed_component.ex") + assert rename_file.new_uri == + subject_uri(project, "lib/demo_web/components/renamed_component.ex") end test "leaves the `components` folder as is when renaming a component", %{project: project} do @@ -428,9 +425,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do "lib/demo_web/components/some_context/foo_component.ex" ) - assert {_, to_uri} = rename_file - - assert to_uri == + assert rename_file.new_uri == subject_uri(project, "lib/demo_web/components/some_context/renamed_component.ex") end @@ -441,8 +436,8 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("DemoWeb.RenamedController", "lib/demo_web/controllers/foo_controller.ex") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "lib/demo_web/controllers/renamed_controller.ex") + assert rename_file.new_uri == + subject_uri(project, "lib/demo_web/controllers/renamed_controller.ex") end test "leaves the `controller` folder as is when renaming the `JSON` module", %{ @@ -458,9 +453,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do "lib/demo_web/controllers/foo_controller/json.ex" ) - assert {_, to_uri} = rename_file - - assert to_uri == + assert rename_file.new_uri == subject_uri(project, "lib/demo_web/controllers/foo_controller/renamed_json.ex") end @@ -471,8 +464,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end ] |> rename("DemoWeb.RenamedLive", "lib/demo_web/live/foo_live.ex") - assert {_, to_uri} = rename_file - assert to_uri == subject_uri(project, "lib/demo_web/live/renamed_live.ex") + assert rename_file.new_uri == subject_uri(project, "lib/demo_web/live/renamed_live.ex") end end @@ -485,13 +477,13 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do {:ok, entries} <- Search.Indexer.Source.index(document.path, text), :ok <- Search.Store.replace(entries), analysis = Lexical.Ast.analyze(document), - {:ok, uri_with_changes} <- Rename.rename(analysis, position, new_name) do - changes = uri_with_changes |> Enum.map(& &1.edits) |> List.flatten() + {:ok, document_changes} <- Rename.rename(analysis, position, new_name) do + changes = document_changes |> Enum.map(& &1.edits) |> List.flatten() applied = apply_edits(document, changes) result = if path do - rename_file = uri_with_changes |> Enum.map(& &1.rename_file) |> List.first() + rename_file = document_changes |> Enum.map(& &1.rename_file) |> List.first() {applied, rename_file} else applied diff --git a/apps/server/lib/lexical/server/provider/handlers/rename.ex b/apps/server/lib/lexical/server/provider/handlers/rename.ex index 765b455b7..60b756b5e 100644 --- a/apps/server/lib/lexical/server/provider/handlers/rename.ex +++ b/apps/server/lib/lexical/server/provider/handlers/rename.ex @@ -7,7 +7,6 @@ defmodule Lexical.Server.Provider.Handlers.Rename do alias Lexical.Protocol.Types.TextDocument alias Lexical.Protocol.Types.Workspace alias Lexical.RemoteControl.Api - alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias Lexical.Server.Provider.Env require Logger @@ -29,8 +28,8 @@ defmodule Lexical.Server.Provider.Handlers.Rename do {:ok, results} -> text_document_edits = - Enum.map(results, fn %DocumentChanges{edits: edits, uri: uri} -> - new_text_document_edit(uri, edits) + Enum.map(results, fn %Document.Changes{edits: edits, document: document} -> + new_text_document_edit(document.uri, edits) end) rename_files = @@ -58,13 +57,13 @@ defmodule Lexical.Server.Provider.Handlers.Rename do TextDocument.Edit.new(edits: edits, text_document: text_document) end - defp new_rename_file({from_uri, to_uri}) do + defp new_rename_file(%Document.Changes.RenameFile{} = rename_file) do options = RenameFile.Options.new(overwrite: true) RenameFile.new( kind: "rename", - new_uri: to_uri, - old_uri: from_uri, + new_uri: rename_file.new_uri, + old_uri: rename_file.old_uri, options: options ) end diff --git a/apps/server/test/lexical/server/provider/handlers/rename_test.exs b/apps/server/test/lexical/server/provider/handlers/rename_test.exs index ffe160056..2d7e29f74 100644 --- a/apps/server/test/lexical/server/provider/handlers/rename_test.exs +++ b/apps/server/test/lexical/server/provider/handlers/rename_test.exs @@ -4,7 +4,6 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do alias Lexical.Proto.Convert alias Lexical.Protocol.Requests.Rename alias Lexical.RemoteControl - alias Lexical.RemoteControl.CodeMod.Rename.DocumentChanges alias Lexical.Server alias Lexical.Server.Provider.Env @@ -64,7 +63,7 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do end) patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name -> - {:ok, %{}} + {:ok, []} end) {:ok, request} = build_request(uri, 1, 5) @@ -74,6 +73,8 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do end test "returns edit when there are changes", %{project: project, uri: uri} do + document = %Document{uri: uri, version: 0} + patch(Document.Store, :fetch, fn ^uri, :analysis -> {:ok, nil, %Ast.Analysis{valid?: true}} end) @@ -81,15 +82,18 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name -> {:ok, [ - DocumentChanges.new( - "file:///path/to/file.ex", + Document.Changes.new( + document, [ %{ new_text: "new_text", range: %{start: %{line: 1, character: 5}, end: %{line: 1, character: 10}} } ], - {"file:///path/to/file.ex", "file:///path/to/new_text.ex"} + Document.Changes.RenameFile.new( + document.uri, + "file:///path/to/new_text.ex" + ) ) ]} end) @@ -106,9 +110,9 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do } ] - assert edit.text_document.uri == "file:///path/to/file.ex" + assert edit.text_document.uri == document.uri assert edit.text_document.version == 0 - assert rename_file.old_uri == "file:///path/to/file.ex" + assert rename_file.old_uri == document.uri assert rename_file.new_uri == "file:///path/to/new_text.ex" end end diff --git a/projects/lexical_shared/lib/lexical/document/changes.ex b/projects/lexical_shared/lib/lexical/document/changes.ex index fc848452f..f6ea4c6fa 100644 --- a/projects/lexical_shared/lib/lexical/document/changes.ex +++ b/projects/lexical_shared/lib/lexical/document/changes.ex @@ -9,15 +9,28 @@ defmodule Lexical.Document.Changes do Using this struct allows efficient conversions at the language server border, as the document doesn't have to be looked up (and possibly read off the filesystem) by the language server. """ - defstruct [:document, :edits] + defmodule RenameFile do + @type t :: %__MODULE__{old_uri: Lexical.uri(), new_uri: Lexical.uri()} + + defstruct [:old_uri, :new_uri] + + @spec new(Lexical.uri(), Lexical.uri()) :: t() + def new(old_uri, new_uri) do + %__MODULE__{old_uri: old_uri, new_uri: new_uri} + end + end + + defstruct [:document, :edits, :rename_file] alias Lexical.Document use Lexical.StructAccess @type edits :: Document.Edit.t() | [Document.Edit.t()] + @type rename_file :: nil | RenameFile.t() @type t :: %__MODULE__{ document: Document.t(), - edits: edits + edits: edits, + rename_file: rename_file() } @doc """ @@ -28,4 +41,9 @@ defmodule Lexical.Document.Changes do def new(document, edits) do %__MODULE__{document: document, edits: edits} end + + @spec new(Document.t(), edits(), rename_file()) :: t() + def new(document, edits, rename_file) do + %__MODULE__{document: document, edits: edits, rename_file: rename_file} + end end From ece113c3866c3b47a4ce00a5290df2c7f6072409 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sat, 6 Apr 2024 15:44:05 +0800 Subject: [PATCH 04/12] Use `rename progress marker` instead of suspending build --- .../lib/lexical/remote_control/api.ex | 31 +++++ .../lib/lexical/remote_control/application.ex | 1 + .../lexical/remote_control/code_mod/format.ex | 12 +- .../lexical/remote_control/code_mod/rename.ex | 20 ++- .../lexical/remote_control/commands/rename.ex | 117 ++++++++++++++++++ .../remote_control/code_mod/format_test.exs | 2 + .../commands/rename/state_test.exs | 37 ++++++ .../remote_control/commands/rename_test.exs | 27 ++++ .../server/lib/lexical/server/provider/env.ex | 7 +- apps/server/lib/lexical/server/state.ex | 5 +- 10 files changed, 252 insertions(+), 7 deletions(-) create mode 100644 apps/remote_control/lib/lexical/remote_control/commands/rename.ex create mode 100644 apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs create mode 100644 apps/remote_control/test/lexical/remote_control/commands/rename_test.exs diff --git a/apps/remote_control/lib/lexical/remote_control/api.ex b/apps/remote_control/lib/lexical/remote_control/api.ex index ebdf336ce..55d7e3d12 100644 --- a/apps/remote_control/lib/lexical/remote_control/api.ex +++ b/apps/remote_control/lib/lexical/remote_control/api.ex @@ -13,6 +13,37 @@ defmodule Lexical.RemoteControl.Api do require Logger + @spec maybe_schedule_compile(Project.t(), Lexical.uri()) :: :ok + def maybe_schedule_compile(project, triggered_file_uri) do + if in_rename_progress?(project) do + RemoteControl.call(project, Commands.Rename, :mark_saved, [triggered_file_uri]) + else + schedule_compile(project, false) + end + end + + @spec maybe_compile_document(Project.t(), Lexical.uri()) :: :ok + def maybe_compile_document(project, document) do + if in_rename_progress?(project) do + RemoteControl.call(project, Commands.Rename, :mark_changed, [document.uri]) + else + compile_document(project, document) + end + end + + @spec maybe_mark_closed(Project.t(), Lexical.uri()) :: :ok + def maybe_mark_closed(%Project{} = project, uri) do + if in_rename_progress?(project) do + RemoteControl.call(project, Commands.Rename, :mark_closed, [uri]) + else + :ok + end + end + + defp in_rename_progress?(%Project{} = project) do + RemoteControl.call(project, Commands.Rename, :in_progress?, []) + end + defdelegate schedule_compile(project, force?), to: Build defdelegate compile_document(project, document), to: Build diff --git a/apps/remote_control/lib/lexical/remote_control/application.ex b/apps/remote_control/lib/lexical/remote_control/application.ex index b7e364f64..7ca4f2f12 100644 --- a/apps/remote_control/lib/lexical/remote_control/application.ex +++ b/apps/remote_control/lib/lexical/remote_control/application.ex @@ -14,6 +14,7 @@ defmodule Lexical.RemoteControl.Application do if RemoteControl.project_node?() do [ {RemoteControl.Commands.Reindex, nil}, + RemoteControl.Commands.Rename, RemoteControl.Module.Loader, {RemoteControl.Dispatch, progress: true}, RemoteControl.ModuleMappings, diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex index dcaabd7dc..078d32620 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex @@ -4,6 +4,7 @@ defmodule Lexical.RemoteControl.CodeMod.Format do alias Lexical.Project alias Lexical.RemoteControl alias Lexical.RemoteControl.Build + alias Lexical.RemoteControl.Commands.Rename alias Lexical.RemoteControl.CodeMod.Diff require Logger @@ -12,13 +13,22 @@ defmodule Lexical.RemoteControl.CodeMod.Format do @spec edits(Project.t(), Document.t()) :: {:ok, Changes.t()} | {:error, any} def edits(%Project{} = project, %Document{} = document) do - with :ok <- Build.compile_document(project, document), + with :ok <- ensure_not_in_rename_process(), + :ok <- Build.compile_document(project, document), {:ok, formatted} <- do_format(project, document) do edits = Diff.diff(document, formatted) {:ok, Changes.new(document, edits)} end end + defp ensure_not_in_rename_process do + if Rename.in_progress?() do + {:error, "Cannot format while renaming"} + else + :ok + end + end + defp do_format(%Project{} = project, %Document{} = document) do project_path = Project.project_path(project) diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index 8a6adade8..e482d02bf 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -3,6 +3,7 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do alias Lexical.Document alias Lexical.Document.Position alias Lexical.Document.Range + alias Lexical.RemoteControl.Commands alias __MODULE__ @spec prepare(Analysis.t(), Position.t()) :: @@ -16,7 +17,24 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do def rename(%Analysis{} = analysis, %Position{} = position, new_name) do with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do rename_module = @rename_mapping[renamable] - {:ok, rename_module.rename(range, new_name, entity)} + results = rename_module.rename(range, new_name, entity) + set_rename_progress(results) + {:ok, results} end end + + defp set_rename_progress(document_changes_list) do + uri_with_operation_counts = + document_changes_list + |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> + if rename_file do + [{rename_file.old_uri, 2}, {rename_file.new_uri, 1}] + else + [{document.uri, 3}] + end + end) + |> Map.new() + + Commands.Rename.set_rename_progress(uri_with_operation_counts) + end end diff --git a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex new file mode 100644 index 000000000..3a3445192 --- /dev/null +++ b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex @@ -0,0 +1,117 @@ +defmodule Lexical.RemoteControl.Commands.Rename do + @moduledoc """ + We are unable to accurately determine the process of renaming, + because after the rename, there will be a series of operations such as + `DidChange`, `DidSave`, etc., which will trigger expensive actions like compiling the entire project. + Therefore, we need this module to make some markings to determine whether it is currently in the process of renaming. + """ + defmodule State do + defstruct uri_with_operation_counts: %{} + + def new(uri_with_operation_counts) do + %__MODULE__{uri_with_operation_counts: uri_with_operation_counts} + end + + def mark_changed(%__MODULE__{} = state, uri) do + new_uri_with_operation_counts = + delete_key_or_reduce_counts(state.uri_with_operation_counts, uri) + + %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} + end + + def mark_saved(%__MODULE__{} = state, uri) do + new_uri_with_operation_counts = + delete_key_or_reduce_counts(state.uri_with_operation_counts, uri) + + %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} + end + + def mark_closed(%__MODULE__{} = state, uri) do + new_uri_with_operation_counts = + delete_key_or_reduce_counts(state.uri_with_operation_counts, uri) + + %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} + end + + def in_progress?(%__MODULE__{} = state) do + state.uri_with_operation_counts != %{} + end + + defp delete_key_or_reduce_counts(uri_with_operation_counts, uri) do + {_, new_map} = + Map.get_and_update(uri_with_operation_counts, uri, fn + nil -> :pop + current_counts when current_counts <= 1 -> :pop + current_counts -> {current_counts, current_counts - 1} + end) + + new_map + end + end + + use GenServer + + def start_link(_) do + GenServer.start_link(__MODULE__, %State{}, name: __MODULE__) + end + + @impl true + def init(state) do + {:ok, state} + end + + def set_rename_progress(uri_with_operation_counts) do + GenServer.cast(__MODULE__, {:set_rename_progress, uri_with_operation_counts}) + end + + def mark_changed(uri) do + GenServer.cast(__MODULE__, {:mark_changed, uri}) + end + + @doc """ + When a rename is completed, the old file will be closed. + """ + def mark_closed(uri) do + GenServer.cast(__MODULE__, {:mark_closed, uri}) + end + + @doc """ + When a rename is completed, the new file will be saved. + """ + def mark_saved(uri) do + GenServer.cast(__MODULE__, {:mark_saved, uri}) + end + + def in_progress? do + GenServer.call(__MODULE__, :in_progress?) + end + + @impl true + def handle_call(:in_progress?, _from, state) do + {:reply, State.in_progress?(state), state} + end + + @impl true + def handle_cast({:set_rename_progress, uri_with_operation_counts}, _state) do + new_state = State.new(uri_with_operation_counts) + {:noreply, new_state} + end + + @impl true + def handle_cast({:mark_changed, uri}, %State{} = state) do + new_state = State.mark_changed(state, uri) + {:noreply, new_state} + end + + @impl true + def handle_cast({:mark_saved, uri}, %State{} = state) do + new_state = State.mark_saved(state, uri) + {:noreply, new_state} + end + + @impl true + def handle_cast({:mark_closed, uri}, %State{} = state) do + new_state = State.mark_closed(state, uri) + {:noreply, new_state} + end +end diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/format_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/format_test.exs index 7eb0f369e..bfaa170d0 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/format_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/format_test.exs @@ -6,6 +6,7 @@ defmodule Lexical.RemoteControl.CodeMod.FormatTest do alias Lexical.RemoteControl.Api.Messages alias Lexical.RemoteControl.Build alias Lexical.RemoteControl.CodeMod.Format + alias Lexical.RemoteControl.Commands use Lexical.Test.CodeMod.Case, enable_ast_conversion: false use Patch @@ -66,6 +67,7 @@ defmodule Lexical.RemoteControl.CodeMod.FormatTest do end setup do + start_supervised!(Commands.Rename) project = project() {:ok, project: project} end diff --git a/apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs b/apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs new file mode 100644 index 000000000..be479bfae --- /dev/null +++ b/apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs @@ -0,0 +1,37 @@ +defmodule Lexical.RemoteControl.Commands.Rename.StateTest do + alias Lexical.RemoteControl.Commands.Rename.State + use ExUnit.Case + use Patch + + test "it should decrease the change count once marked as changed." do + file_uri = "file://file.ex" + initial_state = State.new(%{file_uri => 2}) + state = State.mark_changed(initial_state, file_uri) + assert state.uri_with_operation_counts == %{file_uri => 1} + end + + test "it should remove the uri from the state once marked as saved." do + file_uri = "file://file.ex" + initial_state = State.new(%{file_uri => 1}) + state = State.mark_saved(initial_state, file_uri) + assert state.uri_with_operation_counts == %{} + end + + test "it should not changed the `in_progress` state when the `uri` not in the state." do + initial_state = State.new(%{}) + state = State.mark_changed(initial_state, "file://file.ex") + refute State.in_progress?(state) + end + + test "it should return true if there are uris with change counts." do + initial_state = State.new(%{"file://file.ex" => 1}) + + assert State.in_progress?(initial_state) + end + + test "it should return false if there are no uris with change counts." do + initial_state = State.new(%{}) + + refute State.in_progress?(initial_state) + end +end diff --git a/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs b/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs new file mode 100644 index 000000000..3056b74df --- /dev/null +++ b/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs @@ -0,0 +1,27 @@ +defmodule Lexical.RemoteControl.Commands.RenameTest do + alias Lexical.RemoteControl.Commands.Rename + + use ExUnit.Case + use Patch + + setup do + start_supervised!(Rename) + :ok + end + + test "it should mark the `in_progress` as `true` when a rename is in progress." do + assert :ok = Rename.set_rename_progress(%{"file://file.ex" => 3}) + assert Rename.in_progress?() + end + + test "it should mark the `in_progress` as false when a rename is done" do + file_uri = "file://file.ex" + Rename.set_rename_progress(%{file_uri => 3}) + + Rename.mark_changed(file_uri) + Rename.mark_saved(file_uri) + Rename.mark_closed(file_uri) + + refute Rename.in_progress?() + end +end diff --git a/apps/server/lib/lexical/server/provider/env.ex b/apps/server/lib/lexical/server/provider/env.ex index f728c6ba7..6960bb363 100644 --- a/apps/server/lib/lexical/server/provider/env.ex +++ b/apps/server/lib/lexical/server/provider/env.ex @@ -8,10 +8,11 @@ defmodule Lexical.Server.Provider.Env do alias Lexical.Project alias Lexical.Server.Configuration - defstruct [:project] + defstruct [:project, :client_name] @type t :: %__MODULE__{ - project: Project.t() + project: Project.t(), + client_name: String.t() | nil } def new do @@ -19,7 +20,7 @@ defmodule Lexical.Server.Provider.Env do end def from_configuration(%Configuration{} = config) do - %__MODULE__{project: config.project} + %__MODULE__{project: config.project, client_name: config.client_name} end def project_name(%__MODULE__{} = env) do diff --git a/apps/server/lib/lexical/server/state.ex b/apps/server/lib/lexical/server/state.ex index 95b8a8b39..e8cb28669 100644 --- a/apps/server/lib/lexical/server/state.ex +++ b/apps/server/lib/lexical/server/state.ex @@ -162,7 +162,7 @@ defmodule Lexical.Server.State do ) Api.broadcast(project, updated_message) - Api.compile_document(state.configuration.project, updated_source) + Api.maybe_compile_document(state.configuration.project, updated_source) {:ok, state} error -> @@ -194,6 +194,7 @@ defmodule Lexical.Server.State do case Document.Store.close(uri) do :ok -> + Api.maybe_mark_closed(state.configuration.project, uri) {:ok, state} error -> @@ -210,7 +211,7 @@ defmodule Lexical.Server.State do case Document.Store.save(uri) do :ok -> - Api.schedule_compile(state.configuration.project, false) + Api.maybe_schedule_compile(state.configuration.project, uri) {:ok, state} error -> From 16d11680a5c20abf04b80ef6a6aed22d8a661e35 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sat, 6 Apr 2024 19:37:48 +0800 Subject: [PATCH 05/12] Use `file_changed` instead of `file_compile_requested` for reindexing --- .../lexical/remote_control/api/messages.ex | 2 ++ .../remote_control/commands/reindex.ex | 19 +++++++++++++++++-- .../dispatch/handlers/indexing.ex | 15 +++++++++++++-- apps/server/lib/lexical/server/state.ex | 2 ++ 4 files changed, 34 insertions(+), 4 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/api/messages.ex b/apps/remote_control/lib/lexical/remote_control/api/messages.ex index fa1baaad7..aee2b5984 100644 --- a/apps/remote_control/lib/lexical/remote_control/api/messages.ex +++ b/apps/remote_control/lib/lexical/remote_control/api/messages.ex @@ -15,6 +15,8 @@ defmodule Lexical.RemoteControl.Api.Messages do defrecord :file_changed, uri: nil, from_version: nil, to_version: nil, open?: false + defrecord :file_opened, uri: nil, version: nil + defrecord :file_compile_requested, project: nil, build_number: 0, uri: nil defrecord :file_compiled, diff --git a/apps/remote_control/lib/lexical/remote_control/commands/reindex.ex b/apps/remote_control/lib/lexical/remote_control/commands/reindex.ex index d9763ffde..0f2f29d43 100644 --- a/apps/remote_control/lib/lexical/remote_control/commands/reindex.ex +++ b/apps/remote_control/lib/lexical/remote_control/commands/reindex.ex @@ -1,5 +1,6 @@ defmodule Lexical.RemoteControl.Commands.Reindex do defmodule State do + alias Lexical.Ast alias Lexical.Ast.Analysis alias Lexical.Document alias Lexical.ProcessCache @@ -54,8 +55,7 @@ defmodule Lexical.RemoteControl.Commands.Reindex do end defp entries_for_uri(uri) do - with {:ok, %Document{} = document, %Analysis{} = analysis} <- - Document.Store.fetch(uri, :analysis), + with {:ok, %Document{} = document, %Analysis{} = analysis} <- ensure_document_open(uri), {:ok, entries} <- Indexer.Quoted.index_with_cleanup(analysis) do {:ok, document.path, entries} else @@ -64,6 +64,21 @@ defmodule Lexical.RemoteControl.Commands.Reindex do error end end + + defp ensure_document_open(uri) do + case Document.Store.fetch(uri, :analysis) do + {:ok, %Document{} = document, analysis} -> + {:ok, document, analysis} + + {:error, :not_open} -> + # Sometimes, some operations are received long after the reindex is triggered, + # such as new files after a batch rename + with {:ok, document} <- Document.Store.open_temporary(uri) do + analysis = Ast.analyze(document) + {:ok, document, analysis} + end + end + end end @moduledoc """ diff --git a/apps/remote_control/lib/lexical/remote_control/dispatch/handlers/indexing.ex b/apps/remote_control/lib/lexical/remote_control/dispatch/handlers/indexing.ex index f20ec4bd3..b7c569168 100644 --- a/apps/remote_control/lib/lexical/remote_control/dispatch/handlers/indexing.ex +++ b/apps/remote_control/lib/lexical/remote_control/dispatch/handlers/indexing.ex @@ -8,9 +8,20 @@ defmodule Lexical.RemoteControl.Dispatch.Handlers.Indexing do require Logger import Messages - use Dispatch.Handler, [file_compile_requested(), filesystem_event()] + use Dispatch.Handler, [filesystem_event(), file_changed(), file_opened()] - def on_event(file_compile_requested(uri: uri), state) do + def on_event(file_changed(uri: uri, open?: true), state) do + reindex(uri) + {:ok, state} + end + + def on_event(file_changed(), state) do + {:ok, state} + end + + def on_event(file_opened(uri: uri), state) do + # When renaming occurs, new files are only *opened* and *closed*, + # so we need to index the new files. reindex(uri) {:ok, state} end diff --git a/apps/server/lib/lexical/server/state.ex b/apps/server/lib/lexical/server/state.ex index e8cb28669..c290d9c82 100644 --- a/apps/server/lib/lexical/server/state.ex +++ b/apps/server/lib/lexical/server/state.ex @@ -181,6 +181,8 @@ defmodule Lexical.Server.State do case Document.Store.open(uri, text, version, language_id) do :ok -> Logger.info("opened #{uri}") + project = state.configuration.project + Api.broadcast(project, file_opened(uri: uri, version: version)) {:ok, state} error -> From 523e5e6fbd4e0382d0a05aa9d24c97c57305d144 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sun, 7 Apr 2024 20:04:13 +0800 Subject: [PATCH 06/12] `uri_with_operations_counts` by client name --- .../lib/lexical/remote_control/api.ex | 11 +++- .../lexical/remote_control/code_mod/rename.ex | 52 +++++++++++++------ .../server/provider/handlers/rename.ex | 13 +++-- 3 files changed, 56 insertions(+), 20 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/api.ex b/apps/remote_control/lib/lexical/remote_control/api.ex index 55d7e3d12..4082b0a74 100644 --- a/apps/remote_control/lib/lexical/remote_control/api.ex +++ b/apps/remote_control/lib/lexical/remote_control/api.ex @@ -86,11 +86,18 @@ defmodule Lexical.RemoteControl.Api do RemoteControl.call(project, CodeMod.Rename, :prepare, [analysis, position]) end - def rename(%Project{} = project, %Analysis{} = analysis, %Position{} = position, new_name) do + def rename( + %Project{} = project, + %Analysis{} = analysis, + %Position{} = position, + new_name, + client_name + ) do RemoteControl.call(project, CodeMod.Rename, :rename, [ analysis, position, - new_name + new_name, + client_name ]) end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index e482d02bf..b1a8182e8 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -12,29 +12,51 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do @rename_mapping %{module: Rename.Module} - @spec rename(Analysis.t(), Position.t(), String.t()) :: + @spec rename(Analysis.t(), Position.t(), String.t(), String.t() | nil) :: {:ok, [Document.Changes.t()]} | {:error, term()} - def rename(%Analysis{} = analysis, %Position{} = position, new_name) do + def rename(%Analysis{} = analysis, %Position{} = position, new_name, client_name) do with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do rename_module = @rename_mapping[renamable] results = rename_module.rename(range, new_name, entity) - set_rename_progress(results) + set_rename_progress(results, client_name) {:ok, results} end end - defp set_rename_progress(document_changes_list) do - uri_with_operation_counts = - document_changes_list - |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> - if rename_file do - [{rename_file.old_uri, 2}, {rename_file.new_uri, 1}] - else - [{document.uri, 3}] - end - end) - |> Map.new() - + defp set_rename_progress(document_changes_list, client_name) do + uri_with_operation_counts = uri_with_operation_counts(client_name, document_changes_list) Commands.Rename.set_rename_progress(uri_with_operation_counts) end + + defp uri_with_operation_counts("Visual Studio Code", document_changes_list) do + document_changes_list + |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> + if rename_file do + # first operation is for `DidChange` in the old file, + # second operation is for `DidClose` in the old file + # Note: `DidSave` won't be received for the old file + # third operation is for `DidSave` in the new file + [{rename_file.old_uri, 2}, {rename_file.new_uri, 1}] + else + [{document.uri, 3}] + end + end) + |> Map.new() + end + + defp uri_with_operation_counts(_, document_changes_list) do + document_changes_list + |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> + if rename_file do + # first operation is for `DidChange` + # second operation is for `DidSave` + # third operation is for `DidClose` + [{document.uri, 3}] + else + # when the file is not renamed, we'll only received `DidChange` for the old file + [{document.uri, 1}] + end + end) + |> Map.new() + end end diff --git a/apps/server/lib/lexical/server/provider/handlers/rename.ex b/apps/server/lib/lexical/server/provider/handlers/rename.ex index 60b756b5e..f211240ad 100644 --- a/apps/server/lib/lexical/server/provider/handlers/rename.ex +++ b/apps/server/lib/lexical/server/provider/handlers/rename.ex @@ -13,7 +13,14 @@ defmodule Lexical.Server.Provider.Handlers.Rename do def handle(%Rename{} = request, %Env{} = env) do case Document.Store.fetch(request.document.uri, :analysis) do {:ok, _document, %Ast.Analysis{valid?: true} = analysis} -> - rename(env.project, analysis, request.position, request.new_name, request.id) + rename( + env.project, + analysis, + request.position, + request.new_name, + request.id, + env.client_name + ) _ -> {:reply, @@ -21,8 +28,8 @@ defmodule Lexical.Server.Provider.Handlers.Rename do end end - defp rename(project, analysis, position, new_name, id) do - case Api.rename(project, analysis, position, new_name) do + defp rename(project, analysis, position, new_name, id, client_name) do + case Api.rename(project, analysis, position, new_name, client_name) do {:ok, []} -> {:reply, nil} From 2ada97d1ca94bb4136a48c7f727c483bd34809fc Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Tue, 9 Apr 2024 15:49:35 +0800 Subject: [PATCH 07/12] Apply some code review suggestions for `rename/file` --- .../code_intelligence/entity.ex | 24 ++- .../remote_control/code_mod/rename/file.ex | 141 ++++++++---------- .../remote_control/code_mod/rename/module.ex | 6 +- .../remote_control/code_mod/rename_test.exs | 37 ++++- 4 files changed, 114 insertions(+), 94 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/code_intelligence/entity.ex b/apps/remote_control/lib/lexical/remote_control/code_intelligence/entity.ex index 1b5a09f94..470b58e89 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_intelligence/entity.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_intelligence/entity.ex @@ -52,6 +52,22 @@ defmodule Lexical.RemoteControl.CodeIntelligence.Entity do ) end + @spec phoenix_controller_module?(module()) :: boolean() + def phoenix_controller_module?(module) do + function_exists?(module, :call, 2) and function_exists?(module, :action, 2) + end + + @spec phoenix_liveview_module?(module()) :: boolean() + def phoenix_liveview_module?(module) do + function_exists?(module, :mount, 3) and function_exists?(module, :render, 1) + end + + @spec phoenix_component_module?(module()) :: boolean() + def phoenix_component_module?(module) do + function_exists?(module, :__components__, 0) and + function_exists?(module, :__phoenix_component_verify__, 1) + end + defp check_commented(%Analysis{} = analysis, %Position{} = position) do if Analysis.commented?(analysis, position) do :error @@ -247,14 +263,6 @@ defmodule Lexical.RemoteControl.CodeIntelligence.Entity do end end - defp phoenix_controller_module?(module) do - function_exists?(module, :call, 2) and function_exists?(module, :action, 2) - end - - defp phoenix_liveview_module?(module) do - function_exists?(module, :mount, 3) and function_exists?(module, :render, 1) - end - # Take only the segments at and before the cursor, e.g. # Foo|.Bar.Baz -> Foo # Foo.|Bar.Baz -> Foo.Bar diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex index 6fed38a78..47a0fcea0 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex @@ -4,23 +4,21 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do alias Lexical.ProcessCache alias Lexical.Project alias Lexical.RemoteControl + alias Lexical.RemoteControl.CodeIntelligence.Entity alias Lexical.RemoteControl.Search.Indexer alias Lexical.RemoteControl.Search.Indexer.Entry - @spec maybe_rename(Entry.t(), String.t()) :: Document.Changes.rename_file() - def maybe_rename(entry, new_suffix) do - if root_module?(entry) do - rename_file(entry, new_suffix) + @spec maybe_rename(Document.t(), Entry.t(), String.t()) :: Document.Changes.rename_file() + def maybe_rename(%Document{} = document, %Entry{} = entry, new_suffix) do + if root_module?(entry, document) do + rename_file(document, entry, new_suffix) end end - defp root_module?(entry) do - uri = Document.Path.ensure_uri(entry.path) - + defp root_module?(entry, document) do entries = - ProcessCache.trans("#{uri}-entries", 50, fn -> - with {:ok, document} <- Document.Store.open_temporary(uri), - {:ok, entries} <- + ProcessCache.trans("#{document.uri}-entries", 50, fn -> + with {:ok, entries} <- Indexer.Source.index_document(document, [Indexer.Extractors.Module]) do entries end @@ -35,24 +33,22 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do end end - defp rename_file(entry, new_suffix) do + defp rename_file(document, entry, new_suffix) do root_path = root_path() relative_path = relative_path(entry.path, root_path) - with {:ok, prefix, old_module_paths} <- fetch_conventional_prefix(relative_path), - {:ok, new_name} <- fetch_new_name(entry, new_suffix) do + with {:ok, prefix} <- fetch_conventional_prefix(relative_path), + {:ok, new_name} <- fetch_new_name(document, entry, new_suffix) do extname = Path.extname(entry.path) suffix = new_name |> Macro.underscore() - |> maybe_insert_special_phoenix_folder(old_module_paths, new_name) + |> maybe_insert_special_phoenix_folder(entry.subject, relative_path) new_path = Path.join([root_path, prefix, "#{suffix}#{extname}"]) - - old_uri = Document.Path.ensure_uri(entry.path) new_uri = Document.Path.ensure_uri(new_path) - Document.Changes.RenameFile.new(old_uri, new_uri) + Document.Changes.RenameFile.new(document.uri, new_uri) else _ -> nil end @@ -66,12 +62,10 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do Project.root_path(RemoteControl.get_project()) end - defp fetch_new_name(entry, new_suffix) do - uri = Document.Path.ensure_uri(entry.path) + defp fetch_new_name(document, entry, new_suffix) do text_edits = [Document.Edit.new(new_suffix, entry.range)] - with {:ok, document} <- Document.Store.open_temporary(uri), - {:ok, edited_document} = + with {:ok, edited_document} <- Document.apply_content_changes(document, document.version + 1, text_edits), {:ok, %{context: {:alias, alias}}} <- Ast.surround_context(edited_document, entry.range.start) do @@ -82,91 +76,76 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do end defp fetch_conventional_prefix(path) do + # To obtain the new relative path, we can't directly convert from the *new module* name. + # We also need a part of the prefix, and Elixir has some conventions in this regard, + # For example: + # + # in umbrella projects, the prefix is `Path.join(["apps", app_name, "lib"])` + # in non-umbrella projects, most file's prefix is `"lib"` + # + # ## Examples + # + # iex> fetch_conventional_prefix("apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex") + # {:ok, "apps/remote_control/lib"} result = path |> Path.split() |> Enum.chunk_every(2, 2) - |> Enum.reduce({[], []}, fn + |> Enum.reduce([], fn ["apps", app_name], _ -> - {[], [app_name, "apps"]} + [app_name, "apps"] - ["lib", follow_element], {elements, prefix} -> - {[follow_element | elements], ["lib" | prefix]} + ["lib", _follow_element], prefix -> + ["lib" | prefix] - ["test", follow_element], {elements, prefix} -> - {[follow_element | elements], ["test" | prefix]} + ["test", _follow_element], prefix -> + ["test" | prefix] - remain, {elements, prefix} -> - {remain ++ elements, prefix} + _remain, prefix -> + prefix end) case result do - {_, []} -> + [] -> :error - {module_paths, prefix} -> - prefix = prefix |> Enum.reverse() |> Enum.join("/") - {:ok, prefix, module_paths} + prefix -> + prefix = prefix |> Enum.reverse() |> Path.join() + {:ok, prefix} end end - defp maybe_insert_special_phoenix_folder(suffix, old_module_paths, new_name) do - web_app? = new_name |> String.split(".") |> hd() |> String.ends_with?("Web") - + defp maybe_insert_special_phoenix_folder(suffix, subject, relative_path) do insertions = cond do - not web_app? -> - "" - - phoenix_component?(old_module_paths, new_name) -> - "components" - - phoenix_controller?(old_module_paths, new_name) -> + Entity.phoenix_controller_module?(subject) -> "controllers" - phoenix_live_view?(old_module_paths, new_name) -> + Entity.phoenix_liveview_module?(subject) -> "live" + Entity.phoenix_component_module?(subject) -> + "components" + true -> - "" + nil end - suffix - |> String.split("/") - |> List.insert_at(1, insertions) - |> Enum.reject(&(&1 == "")) - |> Enum.join("/") - end - - defp phoenix_component?(old_module_paths, new_name) do - under_components? = "components" in old_module_paths - component? = String.ends_with?(new_name, ["Components", "Layouts", "Component"]) - - under_components? and component? - end + # In some cases, users prefer to include the `insertions` in the module name, + # such as `DemoWeb.Components.Icons`. + # In this case, we should not insert the prefix in a nested manner. + prefer_to_include_insertions? = insertions && insertions in Path.split(suffix) + old_path_contains_insertions? = insertions in Path.split(relative_path) - defp phoenix_controller?(old_module_paths, new_name) do - under_controllers? = "controllers" in old_module_paths - controller? = String.ends_with?(new_name, ["Controller", "JSON", "HTML"]) - - under_controllers? and controller? - end - - defp phoenix_live_view?(old_module_paths, new_name) do - under_live_views? = "live" in old_module_paths - new_name_list = String.split(new_name, ".") - - live_view? = - if match?([_, _ | _], new_name_list) do - parent = Enum.at(new_name_list, -2) - local_module = Enum.at(new_name_list, -1) - - # `LiveDemoWeb.SomeLive` or `LiveDemoWeb.SomeLive.Index` - String.ends_with?(parent, "Live") or String.ends_with?(local_module, "Live") - else - false - end - - under_live_views? and live_view? + if not is_nil(insertions) and old_path_contains_insertions? and + not prefer_to_include_insertions? do + suffix + |> Path.split() + |> List.insert_at(1, insertions) + |> Enum.reject(&(&1 == "")) + |> Path.join() + else + suffix + end end end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex index 06b9298a9..79f4866be 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex @@ -79,9 +79,9 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.Module do |> adjust_range_for_descendants(entity, old_suffix) end - defp maybe_rename_file(entries, new_suffix) do + defp maybe_rename_file(document, entries, new_suffix) do entries - |> Enum.map(&Rename.File.maybe_rename(&1, new_suffix)) + |> Enum.map(&Rename.File.maybe_rename(document, &1, new_suffix)) # every group should have only one `rename_file` |> Enum.find(&(not is_nil(&1))) end @@ -173,9 +173,9 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.Module do defp to_document_changes(uri, entries, new_suffix) do edits = Enum.map(entries, &Edit.new(new_suffix, &1.range)) - rename_file = maybe_rename_file(entries, new_suffix) with {:ok, document} <- Document.Store.open_temporary(uri) do + rename_file = maybe_rename_file(document, entries, new_suffix) {:ok, Document.Changes.new(document, edits, rename_file)} end end diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs index f9539b316..69a98034a 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs @@ -4,6 +4,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do alias Lexical.RemoteControl.CodeMod.Rename alias Lexical.RemoteControl.Search alias Lexical.RemoteControl.Search.Store.Backends + alias Lexical.RemoteControl.CodeIntelligence.Entity alias Lexical.Test.CodeSigil alias Lexical.Test.CursorSupport alias Lexical.Test.Fixtures @@ -14,6 +15,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do import Fixtures use ExUnit.Case + use Patch setup_all do project = project() @@ -331,6 +333,11 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end describe "rename file" do + setup do + patch(Entity, :function_exists?, false) + :ok + end + test "it shouldn't rename file if the module has parent module within that file" do {:ok, {_applied, nil}} = ~q[ @@ -404,9 +411,11 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end test "leaves the `components` folder as is when renaming the live view", %{project: project} do + patch(Entity, :phoenix_component_module?, fn DemoWeb.FooComponent -> true end) + {:ok, {_applied, rename_file}} = ~q[ - defmodule DemoWeb.|FooComponents do + defmodule DemoWeb.|FooComponent do end ] |> rename("DemoWeb.RenamedComponent", "lib/demo_web/components/foo_component.ex") @@ -415,6 +424,8 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end test "leaves the `components` folder as is when renaming a component", %{project: project} do + patch(Entity, :phoenix_component_module?, fn DemoWeb.SomeContext.FooComponent -> true end) + {:ok, {_applied, rename_file}} = ~q[ defmodule DemoWeb.SomeContext.|FooComponent do @@ -429,7 +440,25 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do subject_uri(project, "lib/demo_web/components/some_context/renamed_component.ex") end + test "leaves the `components` folder as is when the user prefers to include the `Components` in the module name", + %{ + project: project + } do + patch(Entity, :phoenix_component_module?, fn DemoWeb.Components.Icons -> true end) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule DemoWeb.Components.|Icons do + end + ] |> rename("DemoWeb.Components.RenamedIcons", "lib/demo_web/components/icons.ex") + + assert rename_file.new_uri == + subject_uri(project, "lib/demo_web/components/renamed_icons.ex") + end + test "leaves the `controllers` folder as is when renaming the controller", %{project: project} do + patch(Entity, :phoenix_controller_module?, fn DemoWeb.FooController -> true end) + {:ok, {_applied, rename_file}} = ~q[ defmodule DemoWeb.|FooController do @@ -443,6 +472,8 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do test "leaves the `controller` folder as is when renaming the `JSON` module", %{ project: project } do + patch(Entity, :phoenix_controller_module?, fn DemoWeb.FooController.JSON -> true end) + {:ok, {_applied, rename_file}} = ~q[ defmodule DemoWeb.FooController.|JSON do @@ -458,6 +489,8 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do end test "leaves the `live` folder as is when renaming the live view", %{project: project} do + patch(Entity, :phoenix_liveview_module?, fn DemoWeb.FooLive -> true end) + {:ok, {_applied, rename_file}} = ~q[ defmodule DemoWeb.|FooLive do @@ -477,7 +510,7 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do {:ok, entries} <- Search.Indexer.Source.index(document.path, text), :ok <- Search.Store.replace(entries), analysis = Lexical.Ast.analyze(document), - {:ok, document_changes} <- Rename.rename(analysis, position, new_name) do + {:ok, document_changes} <- Rename.rename(analysis, position, new_name, nil) do changes = document_changes |> Enum.map(& &1.edits) |> List.flatten() applied = apply_edits(document, changes) From d4ff5b01ebccf18b2636fdcaf09720ce6d0b9caa Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Thu, 11 Apr 2024 20:44:07 +0800 Subject: [PATCH 08/12] Apply suggestions from code review Co-authored-by: Steve Cohen --- .../lib/lexical/remote_control/api.ex | 14 ++++++-------- .../lib/lexical/remote_control/code_mod/format.ex | 6 +++--- .../lib/lexical/remote_control/code_mod/rename.ex | 5 +++-- .../remote_control/code_mod/rename_test.exs | 2 +- apps/server/lib/lexical/server/state.ex | 2 +- .../server/provider/handlers/rename_test.exs | 4 ++-- 6 files changed, 16 insertions(+), 17 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/api.ex b/apps/remote_control/lib/lexical/remote_control/api.ex index 4082b0a74..b6921f7c8 100644 --- a/apps/remote_control/lib/lexical/remote_control/api.ex +++ b/apps/remote_control/lib/lexical/remote_control/api.ex @@ -13,34 +13,32 @@ defmodule Lexical.RemoteControl.Api do require Logger - @spec maybe_schedule_compile(Project.t(), Lexical.uri()) :: :ok def maybe_schedule_compile(project, triggered_file_uri) do - if in_rename_progress?(project) do + if rename_running?(project) do RemoteControl.call(project, Commands.Rename, :mark_saved, [triggered_file_uri]) else schedule_compile(project, false) end end - @spec maybe_compile_document(Project.t(), Lexical.uri()) :: :ok def maybe_compile_document(project, document) do - if in_rename_progress?(project) do + if rename_running?(project) do RemoteControl.call(project, Commands.Rename, :mark_changed, [document.uri]) else compile_document(project, document) end end - @spec maybe_mark_closed(Project.t(), Lexical.uri()) :: :ok - def maybe_mark_closed(%Project{} = project, uri) do - if in_rename_progress?(project) do + @spec mark_rename_file_closed(Project.t(), Lexical.uri()) :: :ok + def mark_rename_file_closed(%Project{} = project, uri) do + if rename_running?(project) do RemoteControl.call(project, Commands.Rename, :mark_closed, [uri]) else :ok end end - defp in_rename_progress?(%Project{} = project) do + defp rename_running?(%Project{} = project) do RemoteControl.call(project, Commands.Rename, :in_progress?, []) end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex index 078d32620..ea29d6ebc 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex @@ -4,8 +4,8 @@ defmodule Lexical.RemoteControl.CodeMod.Format do alias Lexical.Project alias Lexical.RemoteControl alias Lexical.RemoteControl.Build - alias Lexical.RemoteControl.Commands.Rename alias Lexical.RemoteControl.CodeMod.Diff + alias Lexical.RemoteControl.Commands.Rename require Logger @@ -13,7 +13,7 @@ defmodule Lexical.RemoteControl.CodeMod.Format do @spec edits(Project.t(), Document.t()) :: {:ok, Changes.t()} | {:error, any} def edits(%Project{} = project, %Document{} = document) do - with :ok <- ensure_not_in_rename_process(), + with :ok <- ensure_not_renaming(), :ok <- Build.compile_document(project, document), {:ok, formatted} <- do_format(project, document) do edits = Diff.diff(document, formatted) @@ -21,7 +21,7 @@ defmodule Lexical.RemoteControl.CodeMod.Format do end end - defp ensure_not_in_rename_process do + defp ensure_not_renaming do if Rename.in_progress?() do {:error, "Cannot format while renaming"} else diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index b1a8182e8..0736ab8aa 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -24,8 +24,9 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do end defp set_rename_progress(document_changes_list, client_name) do - uri_with_operation_counts = uri_with_operation_counts(client_name, document_changes_list) - Commands.Rename.set_rename_progress(uri_with_operation_counts) + client_name + |> uri_with_operation_counts(document_changes_list) + |> Commands.Rename.set_rename_progress() end defp uri_with_operation_counts("Visual Studio Code", document_changes_list) do diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs index 69a98034a..182d39af7 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs @@ -1,10 +1,10 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do alias Lexical.Document alias Lexical.RemoteControl + alias Lexical.RemoteControl.CodeIntelligence.Entity alias Lexical.RemoteControl.CodeMod.Rename alias Lexical.RemoteControl.Search alias Lexical.RemoteControl.Search.Store.Backends - alias Lexical.RemoteControl.CodeIntelligence.Entity alias Lexical.Test.CodeSigil alias Lexical.Test.CursorSupport alias Lexical.Test.Fixtures diff --git a/apps/server/lib/lexical/server/state.ex b/apps/server/lib/lexical/server/state.ex index c290d9c82..dbdface82 100644 --- a/apps/server/lib/lexical/server/state.ex +++ b/apps/server/lib/lexical/server/state.ex @@ -196,7 +196,7 @@ defmodule Lexical.Server.State do case Document.Store.close(uri) do :ok -> - Api.maybe_mark_closed(state.configuration.project, uri) + Api.mark_rename_file_closed(state.configuration.project, uri) {:ok, state} error -> diff --git a/apps/server/test/lexical/server/provider/handlers/rename_test.exs b/apps/server/test/lexical/server/provider/handlers/rename_test.exs index 2d7e29f74..6160f6759 100644 --- a/apps/server/test/lexical/server/provider/handlers/rename_test.exs +++ b/apps/server/test/lexical/server/provider/handlers/rename_test.exs @@ -62,7 +62,7 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do {:ok, nil, %Ast.Analysis{valid?: true}} end) - patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name -> + patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name, _ -> {:ok, []} end) @@ -79,7 +79,7 @@ defmodule Lexical.Server.Provider.Handlers.RenameTest do {:ok, nil, %Ast.Analysis{valid?: true}} end) - patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name -> + patch(RemoteControl.Api, :rename, fn ^project, _analysis, _position, _new_name, _ -> {:ok, [ Document.Changes.new( From 8fa028d2133f63081af4a147e393716ee043426e Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sat, 13 Apr 2024 16:57:32 +0800 Subject: [PATCH 09/12] Do not need to care about the `DidClose` message --- .../lib/lexical/remote_control/api.ex | 9 --------- .../lib/lexical/remote_control/code_mod/rename.ex | 10 ++++------ .../lib/lexical/remote_control/commands/rename.ex | 14 -------------- .../remote_control/commands/rename_test.exs | 3 +-- apps/server/lib/lexical/server/state.ex | 1 - 5 files changed, 5 insertions(+), 32 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/api.ex b/apps/remote_control/lib/lexical/remote_control/api.ex index b6921f7c8..1ecd13757 100644 --- a/apps/remote_control/lib/lexical/remote_control/api.ex +++ b/apps/remote_control/lib/lexical/remote_control/api.ex @@ -29,15 +29,6 @@ defmodule Lexical.RemoteControl.Api do end end - @spec mark_rename_file_closed(Project.t(), Lexical.uri()) :: :ok - def mark_rename_file_closed(%Project{} = project, uri) do - if rename_running?(project) do - RemoteControl.call(project, Commands.Rename, :mark_closed, [uri]) - else - :ok - end - end - defp rename_running?(%Project{} = project) do RemoteControl.call(project, Commands.Rename, :in_progress?, []) end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index 0736ab8aa..1c6954c35 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -34,12 +34,11 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> if rename_file do # first operation is for `DidChange` in the old file, - # second operation is for `DidClose` in the old file # Note: `DidSave` won't be received for the old file - # third operation is for `DidSave` in the new file - [{rename_file.old_uri, 2}, {rename_file.new_uri, 1}] + # second operation is for `DidSave` in the new file + [{rename_file.old_uri, 1}, {rename_file.new_uri, 1}] else - [{document.uri, 3}] + [{document.uri, 2}] end end) |> Map.new() @@ -51,8 +50,7 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do if rename_file do # first operation is for `DidChange` # second operation is for `DidSave` - # third operation is for `DidClose` - [{document.uri, 3}] + [{document.uri, 2}] else # when the file is not renamed, we'll only received `DidChange` for the old file [{document.uri, 1}] diff --git a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex index 3a3445192..394e17f81 100644 --- a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex @@ -26,13 +26,6 @@ defmodule Lexical.RemoteControl.Commands.Rename do %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} end - def mark_closed(%__MODULE__{} = state, uri) do - new_uri_with_operation_counts = - delete_key_or_reduce_counts(state.uri_with_operation_counts, uri) - - %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} - end - def in_progress?(%__MODULE__{} = state) do state.uri_with_operation_counts != %{} end @@ -68,13 +61,6 @@ defmodule Lexical.RemoteControl.Commands.Rename do GenServer.cast(__MODULE__, {:mark_changed, uri}) end - @doc """ - When a rename is completed, the old file will be closed. - """ - def mark_closed(uri) do - GenServer.cast(__MODULE__, {:mark_closed, uri}) - end - @doc """ When a rename is completed, the new file will be saved. """ diff --git a/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs b/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs index 3056b74df..f14307d4c 100644 --- a/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs @@ -16,11 +16,10 @@ defmodule Lexical.RemoteControl.Commands.RenameTest do test "it should mark the `in_progress` as false when a rename is done" do file_uri = "file://file.ex" - Rename.set_rename_progress(%{file_uri => 3}) + Rename.set_rename_progress(%{file_uri => 2}) Rename.mark_changed(file_uri) Rename.mark_saved(file_uri) - Rename.mark_closed(file_uri) refute Rename.in_progress?() end diff --git a/apps/server/lib/lexical/server/state.ex b/apps/server/lib/lexical/server/state.ex index dbdface82..00042a45c 100644 --- a/apps/server/lib/lexical/server/state.ex +++ b/apps/server/lib/lexical/server/state.ex @@ -196,7 +196,6 @@ defmodule Lexical.Server.State do case Document.Store.close(uri) do :ok -> - Api.mark_rename_file_closed(state.configuration.project, uri) {:ok, state} error -> From 78f71e6f2e0c783e2bbca5bd3dfa271cbc265dbe Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Wed, 17 Apr 2024 21:41:59 +0800 Subject: [PATCH 10/12] Apply some code review suggestions 1. Move the `if-else` logic to the `Commands.Rename` module 2. Add `file_opened` message type 3. modify the format error message --- .../lib/lexical/remote_control/api.ex | 22 +++++-------------- .../lexical/remote_control/api/messages.ex | 6 +++++ .../lib/lexical/remote_control/build.ex | 17 ++++++++++++++ .../lexical/remote_control/code_mod/format.ex | 2 +- .../lexical/remote_control/commands/rename.ex | 4 +--- 5 files changed, 30 insertions(+), 21 deletions(-) diff --git a/apps/remote_control/lib/lexical/remote_control/api.ex b/apps/remote_control/lib/lexical/remote_control/api.ex index 1ecd13757..d6f74bc1d 100644 --- a/apps/remote_control/lib/lexical/remote_control/api.ex +++ b/apps/remote_control/lib/lexical/remote_control/api.ex @@ -13,29 +13,17 @@ defmodule Lexical.RemoteControl.Api do require Logger + defdelegate schedule_compile(project, force?), to: Build + defdelegate compile_document(project, document), to: Build + def maybe_schedule_compile(project, triggered_file_uri) do - if rename_running?(project) do - RemoteControl.call(project, Commands.Rename, :mark_saved, [triggered_file_uri]) - else - schedule_compile(project, false) - end + RemoteControl.call(project, Build, :maybe_schedule_compile, [triggered_file_uri]) end def maybe_compile_document(project, document) do - if rename_running?(project) do - RemoteControl.call(project, Commands.Rename, :mark_changed, [document.uri]) - else - compile_document(project, document) - end - end - - defp rename_running?(%Project{} = project) do - RemoteControl.call(project, Commands.Rename, :in_progress?, []) + RemoteControl.call(project, Build, :maybe_compile_document, [project, document]) end - defdelegate schedule_compile(project, force?), to: Build - defdelegate compile_document(project, document), to: Build - def expand_alias( %Project{} = project, segments_or_module, diff --git a/apps/remote_control/lib/lexical/remote_control/api/messages.ex b/apps/remote_control/lib/lexical/remote_control/api/messages.ex index aee2b5984..861b2d7d0 100644 --- a/apps/remote_control/lib/lexical/remote_control/api/messages.ex +++ b/apps/remote_control/lib/lexical/remote_control/api/messages.ex @@ -81,6 +81,12 @@ defmodule Lexical.RemoteControl.Api.Messages do open?: boolean() ) + @type file_opened :: + record(:file_opened, + uri: Lexical.uri(), + version: non_neg_integer() + ) + @type file_compile_requested :: record(:file_compile_requested, project: Lexical.Project.t(), diff --git a/apps/remote_control/lib/lexical/remote_control/build.ex b/apps/remote_control/lib/lexical/remote_control/build.ex index 2d01d61fb..02a4ed61f 100644 --- a/apps/remote_control/lib/lexical/remote_control/build.ex +++ b/apps/remote_control/lib/lexical/remote_control/build.ex @@ -4,6 +4,7 @@ defmodule Lexical.RemoteControl.Build do alias Lexical.RemoteControl alias Lexical.RemoteControl.Build.Document.Compilers.HEEx alias Lexical.RemoteControl.Build.State + alias Lexical.RemoteControl.Commands.Rename alias Lexical.VM.Versions require Logger @@ -35,6 +36,22 @@ defmodule Lexical.RemoteControl.Build do :ok end + def maybe_schedule_compile(triggered_file_uri) do + if Rename.in_progress?() do + Rename.mark_saved(triggered_file_uri) + else + GenServer.cast(__MODULE__, {:compile, false}) + end + end + + def maybe_compile_document(%Project{} = project, %Document{} = document) do + if Rename.in_progress?() do + Rename.mark_changed(document.uri) + else + compile_document(project, document) + end + end + # this is for testing def force_compile_document(%Project{} = project, %Document{} = document) do with false <- Path.absname(document.path) == "mix.exs", diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex index ea29d6ebc..3aeb07032 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/format.ex @@ -23,7 +23,7 @@ defmodule Lexical.RemoteControl.CodeMod.Format do defp ensure_not_renaming do if Rename.in_progress?() do - {:error, "Cannot format while renaming"} + {:error, :rename_in_progress} else :ok end diff --git a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex index 394e17f81..512ebc81d 100644 --- a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex @@ -53,6 +53,7 @@ defmodule Lexical.RemoteControl.Commands.Rename do {:ok, state} end + @spec set_rename_progress(%{Lexical.uri() => integer()}) :: :ok def set_rename_progress(uri_with_operation_counts) do GenServer.cast(__MODULE__, {:set_rename_progress, uri_with_operation_counts}) end @@ -61,9 +62,6 @@ defmodule Lexical.RemoteControl.Commands.Rename do GenServer.cast(__MODULE__, {:mark_changed, uri}) end - @doc """ - When a rename is completed, the new file will be saved. - """ def mark_saved(uri) do GenServer.cast(__MODULE__, {:mark_saved, uri}) end From 5e895b9617f9ac74af8074b6b55796270ea1db57 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Thu, 18 Apr 2024 21:00:19 +0800 Subject: [PATCH 11/12] Shouldn't returns `RenameFile` if the file name not changed --- .../lib/lexical/remote_control/code_mod/rename/file.ex | 5 ++++- .../test/lexical/remote_control/code_mod/rename_test.exs | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex index 47a0fcea0..f0c5023d1 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex @@ -48,7 +48,10 @@ defmodule Lexical.RemoteControl.CodeMod.Rename.File do new_path = Path.join([root_path, prefix, "#{suffix}#{extname}"]) new_uri = Document.Path.ensure_uri(new_path) - Document.Changes.RenameFile.new(document.uri, new_uri) + + if document.uri != new_uri do + Document.Changes.RenameFile.new(document.uri, new_uri) + end else _ -> nil end diff --git a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs index 182d39af7..2a5e66923 100644 --- a/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/code_mod/rename_test.exs @@ -377,6 +377,14 @@ defmodule Lexical.RemoteControl.CodeMod.RenameTest do assert rename_file.new_uri == subject_uri(project, "lib/renamed.ex") end + test "it shouldn't rename file if just uppercased the module name" do + assert {:ok, {_applied, nil}} = + ~q[ + defmodule |Foo do + end + ] |> rename("FOO", "lib/foo.ex") + end + test "succeeds when the path matching the `apps/*` convension", %{project: project} do {:ok, {_applied, rename_file}} = ~q[ From a36ad788675d782d925ad0baccfb40dd75aa3eae Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sat, 20 Apr 2024 18:01:00 +0800 Subject: [PATCH 12/12] Change `uri_with_operation_counts` to `uri_with_expected_operation` and special some message type for emacs --- .../lib/lexical/remote_control/api.ex | 15 ++-- .../lib/lexical/remote_control/build.ex | 8 +-- .../lexical/remote_control/code_mod/rename.ex | 25 +++---- .../lexical/remote_control/commands/rename.ex | 71 ++++++------------- .../commands/rename/state_test.exs | 37 ---------- .../remote_control/commands/rename_test.exs | 26 +++++-- apps/server/lib/lexical/server/state.ex | 8 +-- 7 files changed, 75 insertions(+), 115 deletions(-) delete mode 100644 apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs diff --git a/apps/remote_control/lib/lexical/remote_control/api.ex b/apps/remote_control/lib/lexical/remote_control/api.ex index d6f74bc1d..d97924533 100644 --- a/apps/remote_control/lib/lexical/remote_control/api.ex +++ b/apps/remote_control/lib/lexical/remote_control/api.ex @@ -16,12 +16,19 @@ defmodule Lexical.RemoteControl.Api do defdelegate schedule_compile(project, force?), to: Build defdelegate compile_document(project, document), to: Build - def maybe_schedule_compile(project, triggered_file_uri) do - RemoteControl.call(project, Build, :maybe_schedule_compile, [triggered_file_uri]) + def maybe_schedule_compile(project, triggered_file_uri, triggered_message) do + RemoteControl.call(project, Build, :maybe_schedule_compile, [ + triggered_file_uri, + triggered_message + ]) end - def maybe_compile_document(project, document) do - RemoteControl.call(project, Build, :maybe_compile_document, [project, document]) + def maybe_compile_document(project, document, triggered_message) do + RemoteControl.call(project, Build, :maybe_compile_document, [ + project, + document, + triggered_message + ]) end def expand_alias( diff --git a/apps/remote_control/lib/lexical/remote_control/build.ex b/apps/remote_control/lib/lexical/remote_control/build.ex index 02a4ed61f..1819ff3d0 100644 --- a/apps/remote_control/lib/lexical/remote_control/build.ex +++ b/apps/remote_control/lib/lexical/remote_control/build.ex @@ -36,17 +36,17 @@ defmodule Lexical.RemoteControl.Build do :ok end - def maybe_schedule_compile(triggered_file_uri) do + def maybe_schedule_compile(triggered_file_uri, message) do if Rename.in_progress?() do - Rename.mark_saved(triggered_file_uri) + Rename.update_progress(triggered_file_uri, message) else GenServer.cast(__MODULE__, {:compile, false}) end end - def maybe_compile_document(%Project{} = project, %Document{} = document) do + def maybe_compile_document(%Project{} = project, %Document{} = document, message) do if Rename.in_progress?() do - Rename.mark_changed(document.uri) + Rename.update_progress(document.uri, message) else compile_document(project, document) end diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex index 1c6954c35..d3d03df42 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex @@ -3,7 +3,10 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do alias Lexical.Document alias Lexical.Document.Position alias Lexical.Document.Range + alias Lexical.Protocol.Notifications.DidChange + alias Lexical.Protocol.Notifications.DidSave alias Lexical.RemoteControl.Commands + alias __MODULE__ @spec prepare(Analysis.t(), Position.t()) :: @@ -25,35 +28,33 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do defp set_rename_progress(document_changes_list, client_name) do client_name - |> uri_with_operation_counts(document_changes_list) + |> uri_with_expected_operation(document_changes_list) |> Commands.Rename.set_rename_progress() end - defp uri_with_operation_counts("Visual Studio Code", document_changes_list) do + defp uri_with_expected_operation(client_name, document_changes_list) + when client_name in ["Visual Studio Code", "emacs"] do document_changes_list |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> if rename_file do - # first operation is for `DidChange` in the old file, - # Note: `DidSave` won't be received for the old file - # second operation is for `DidSave` in the new file - [{rename_file.old_uri, 1}, {rename_file.new_uri, 1}] + # when the file is renamed, we won't receive `DidSave` for the old file + [{rename_file.old_uri, DidChange}, {rename_file.new_uri, DidSave}] else - [{document.uri, 2}] + [{document.uri, DidSave}] end end) |> Map.new() end - defp uri_with_operation_counts(_, document_changes_list) do + defp uri_with_expected_operation(_, document_changes_list) do document_changes_list |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> if rename_file do - # first operation is for `DidChange` - # second operation is for `DidSave` - [{document.uri, 2}] + [{document.uri, DidSave}] else + # Some editors do not directly save the file after renaming, such as *neovim*. # when the file is not renamed, we'll only received `DidChange` for the old file - [{document.uri, 1}] + [{document.uri, DidChange}] end end) |> Map.new() diff --git a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex index 512ebc81d..7fc8f704b 100644 --- a/apps/remote_control/lib/lexical/remote_control/commands/rename.ex +++ b/apps/remote_control/lib/lexical/remote_control/commands/rename.ex @@ -6,39 +6,28 @@ defmodule Lexical.RemoteControl.Commands.Rename do Therefore, we need this module to make some markings to determine whether it is currently in the process of renaming. """ defmodule State do - defstruct uri_with_operation_counts: %{} + defstruct uri_with_expected_operation: %{} - def new(uri_with_operation_counts) do - %__MODULE__{uri_with_operation_counts: uri_with_operation_counts} + def new(uri_with_expected_operation) do + %__MODULE__{uri_with_expected_operation: uri_with_expected_operation} end - def mark_changed(%__MODULE__{} = state, uri) do - new_uri_with_operation_counts = - delete_key_or_reduce_counts(state.uri_with_operation_counts, uri) + def update_progress(%__MODULE__{} = state, uri, operation_message) do + new_uri_with_expected_operation = + maybe_pop_expected_operation(state.uri_with_expected_operation, uri, operation_message) - %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} - end - - def mark_saved(%__MODULE__{} = state, uri) do - new_uri_with_operation_counts = - delete_key_or_reduce_counts(state.uri_with_operation_counts, uri) - - %__MODULE__{state | uri_with_operation_counts: new_uri_with_operation_counts} + %__MODULE__{state | uri_with_expected_operation: new_uri_with_expected_operation} end def in_progress?(%__MODULE__{} = state) do - state.uri_with_operation_counts != %{} + state.uri_with_expected_operation != %{} end - defp delete_key_or_reduce_counts(uri_with_operation_counts, uri) do - {_, new_map} = - Map.get_and_update(uri_with_operation_counts, uri, fn - nil -> :pop - current_counts when current_counts <= 1 -> :pop - current_counts -> {current_counts, current_counts - 1} - end) - - new_map + def maybe_pop_expected_operation(uri_to_operation, uri, %operation{}) do + case uri_to_operation do + %{^uri => ^operation} -> Map.delete(uri_to_operation, uri) + _ -> uri_to_operation + end end end @@ -53,17 +42,13 @@ defmodule Lexical.RemoteControl.Commands.Rename do {:ok, state} end - @spec set_rename_progress(%{Lexical.uri() => integer()}) :: :ok - def set_rename_progress(uri_with_operation_counts) do - GenServer.cast(__MODULE__, {:set_rename_progress, uri_with_operation_counts}) + @spec set_rename_progress(%{Lexical.uri() => atom()}) :: :ok + def set_rename_progress(uri_with_expected_operation) do + GenServer.cast(__MODULE__, {:set_rename_progress, uri_with_expected_operation}) end - def mark_changed(uri) do - GenServer.cast(__MODULE__, {:mark_changed, uri}) - end - - def mark_saved(uri) do - GenServer.cast(__MODULE__, {:mark_saved, uri}) + def update_progress(uri, operation_message) do + GenServer.cast(__MODULE__, {:update_progress, uri, operation_message}) end def in_progress? do @@ -76,26 +61,14 @@ defmodule Lexical.RemoteControl.Commands.Rename do end @impl true - def handle_cast({:set_rename_progress, uri_with_operation_counts}, _state) do - new_state = State.new(uri_with_operation_counts) - {:noreply, new_state} - end - - @impl true - def handle_cast({:mark_changed, uri}, %State{} = state) do - new_state = State.mark_changed(state, uri) - {:noreply, new_state} - end - - @impl true - def handle_cast({:mark_saved, uri}, %State{} = state) do - new_state = State.mark_saved(state, uri) + def handle_cast({:set_rename_progress, uri_with_expected_operation}, _state) do + new_state = State.new(uri_with_expected_operation) {:noreply, new_state} end @impl true - def handle_cast({:mark_closed, uri}, %State{} = state) do - new_state = State.mark_closed(state, uri) + def handle_cast({:update_progress, uri, message}, state) do + new_state = State.update_progress(state, uri, message) {:noreply, new_state} end end diff --git a/apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs b/apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs deleted file mode 100644 index be479bfae..000000000 --- a/apps/remote_control/test/lexical/remote_control/commands/rename/state_test.exs +++ /dev/null @@ -1,37 +0,0 @@ -defmodule Lexical.RemoteControl.Commands.Rename.StateTest do - alias Lexical.RemoteControl.Commands.Rename.State - use ExUnit.Case - use Patch - - test "it should decrease the change count once marked as changed." do - file_uri = "file://file.ex" - initial_state = State.new(%{file_uri => 2}) - state = State.mark_changed(initial_state, file_uri) - assert state.uri_with_operation_counts == %{file_uri => 1} - end - - test "it should remove the uri from the state once marked as saved." do - file_uri = "file://file.ex" - initial_state = State.new(%{file_uri => 1}) - state = State.mark_saved(initial_state, file_uri) - assert state.uri_with_operation_counts == %{} - end - - test "it should not changed the `in_progress` state when the `uri` not in the state." do - initial_state = State.new(%{}) - state = State.mark_changed(initial_state, "file://file.ex") - refute State.in_progress?(state) - end - - test "it should return true if there are uris with change counts." do - initial_state = State.new(%{"file://file.ex" => 1}) - - assert State.in_progress?(initial_state) - end - - test "it should return false if there are no uris with change counts." do - initial_state = State.new(%{}) - - refute State.in_progress?(initial_state) - end -end diff --git a/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs b/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs index f14307d4c..5a7e285a7 100644 --- a/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs +++ b/apps/remote_control/test/lexical/remote_control/commands/rename_test.exs @@ -1,6 +1,14 @@ defmodule Lexical.RemoteControl.Commands.RenameTest do alias Lexical.RemoteControl.Commands.Rename + defmodule FakeDidChange do + defstruct [:field] + end + + defmodule FakeDidSave do + defstruct [:field] + end + use ExUnit.Case use Patch @@ -10,17 +18,25 @@ defmodule Lexical.RemoteControl.Commands.RenameTest do end test "it should mark the `in_progress` as `true` when a rename is in progress." do - assert :ok = Rename.set_rename_progress(%{"file://file.ex" => 3}) + assert :ok = Rename.set_rename_progress(%{"file://file.ex" => FakeDidChange}) assert Rename.in_progress?() end test "it should mark the `in_progress` as false when a rename is done" do file_uri = "file://file.ex" - Rename.set_rename_progress(%{file_uri => 2}) - - Rename.mark_changed(file_uri) - Rename.mark_saved(file_uri) + Rename.set_rename_progress(%{file_uri => FakeDidSave}) + Rename.update_progress(file_uri, %FakeDidSave{}) refute Rename.in_progress?() end + + test "it should still in progress if there are files yet to be saved." do + uri1 = "file://file1.ex" + uri2 = "file://file2.ex" + + Rename.set_rename_progress(%{uri1 => FakeDidChange, uri2 => FakeDidSave}) + Rename.update_progress(uri1, %FakeDidChange{}) + + assert Rename.in_progress?() + end end diff --git a/apps/server/lib/lexical/server/state.ex b/apps/server/lib/lexical/server/state.ex index 00042a45c..2aa19150f 100644 --- a/apps/server/lib/lexical/server/state.ex +++ b/apps/server/lib/lexical/server/state.ex @@ -143,7 +143,7 @@ defmodule Lexical.Server.State do {:ok, state} end - def apply(%__MODULE__{} = state, %DidChange{lsp: event}) do + def apply(%__MODULE__{} = state, %DidChange{lsp: event} = message) do uri = event.text_document.uri version = event.text_document.version project = state.configuration.project @@ -162,7 +162,7 @@ defmodule Lexical.Server.State do ) Api.broadcast(project, updated_message) - Api.maybe_compile_document(state.configuration.project, updated_source) + Api.maybe_compile_document(state.configuration.project, updated_source, message) {:ok, state} error -> @@ -207,12 +207,12 @@ defmodule Lexical.Server.State do end end - def apply(%__MODULE__{} = state, %DidSave{lsp: event}) do + def apply(%__MODULE__{} = state, %DidSave{lsp: event} = message) do uri = event.text_document.uri case Document.Store.save(uri) do :ok -> - Api.maybe_schedule_compile(state.configuration.project, uri) + Api.maybe_schedule_compile(state.configuration.project, uri, message) {:ok, state} error ->