From 86a584d8f2f60e29af0fdbc72aa6cc952640287f Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sat, 3 Jan 2026 10:29:14 +0800 Subject: [PATCH 1/6] feat: add module rename support - engine: add CodeMod.Rename module for module renaming with file rename support - expert: add prepareRename and rename LSP handlers - forge: extend Document.Changes to support RenameFile operations --- apps/engine/lib/engine.ex | 4 + apps/engine/lib/engine/code_mod/rename.ex | 46 +++ .../lib/engine/code_mod/rename/entry.ex | 46 +++ .../engine/lib/engine/code_mod/rename/file.ex | 165 +++++++++++ .../lib/engine/code_mod/rename/module.ex | 261 ++++++++++++++++++ .../lib/engine/code_mod/rename/module/diff.ex | 50 ++++ .../lib/engine/code_mod/rename/prepare.ex | 70 +++++ .../code_mod/rename/module/diff_test.exs | 27 ++ .../test/engine/code_mod/rename_test.exs | 193 +++++++++++++ apps/expert/lib/expert.ex | 6 + apps/expert/lib/expert/engine_api.ex | 19 ++ .../provider/handlers/prepare_rename.ex | 68 +++++ .../lib/expert/provider/handlers/rename.ex | 107 +++++++ apps/expert/lib/expert/state.ex | 6 + apps/forge/lib/forge/document/changes.ex | 29 +- apps/forge/lib/forge/engine_api/messages.ex | 4 + 16 files changed, 1096 insertions(+), 5 deletions(-) create mode 100644 apps/engine/lib/engine/code_mod/rename.ex create mode 100644 apps/engine/lib/engine/code_mod/rename/entry.ex create mode 100644 apps/engine/lib/engine/code_mod/rename/file.ex create mode 100644 apps/engine/lib/engine/code_mod/rename/module.ex create mode 100644 apps/engine/lib/engine/code_mod/rename/module/diff.ex create mode 100644 apps/engine/lib/engine/code_mod/rename/prepare.ex create mode 100644 apps/engine/test/engine/code_mod/rename/module/diff_test.exs create mode 100644 apps/engine/test/engine/code_mod/rename_test.exs create mode 100644 apps/expert/lib/expert/provider/handlers/prepare_rename.ex create mode 100644 apps/expert/lib/expert/provider/handlers/rename.ex diff --git a/apps/engine/lib/engine.ex b/apps/engine/lib/engine.ex index 643e636d..8f74c35a 100644 --- a/apps/engine/lib/engine.ex +++ b/apps/engine/lib/engine.ex @@ -63,6 +63,10 @@ defmodule Engine do defdelegate workspace_symbols(query), to: CodeIntelligence.Symbols, as: :for_workspace + defdelegate prepare_rename(analysis, position), to: Engine.CodeMod.Rename, as: :prepare + + defdelegate rename(analysis, position, new_name, client_name), to: Engine.CodeMod.Rename + def list_apps do for {app, _, _} <- :application.loaded_applications(), not Forge.Namespace.Module.prefixed?(app), diff --git a/apps/engine/lib/engine/code_mod/rename.ex b/apps/engine/lib/engine/code_mod/rename.ex new file mode 100644 index 00000000..cee7a359 --- /dev/null +++ b/apps/engine/lib/engine/code_mod/rename.ex @@ -0,0 +1,46 @@ +defmodule Engine.CodeMod.Rename do + @moduledoc """ + Entry point for rename operations. + + This module provides the main API for renaming entities (currently modules) + in Elixir code. It coordinates between the preparation phase and the actual + rename execution. + """ + alias Engine.CodeMod.Rename + alias Forge.Ast.Analysis + alias Forge.Document + alias Forge.Document.Position + alias Forge.Document.Range + + @doc """ + Prepares a rename operation at the given position. + + Returns `{:ok, entity_name, range}` if the entity can be renamed, + `{:ok, nil}` if at an unsupported location, + or `{:error, reason}` if renaming is not possible. + """ + @spec prepare(Analysis.t(), Position.t()) :: + {:ok, String.t(), Range.t()} | {:ok, nil} | {:error, term()} + defdelegate prepare(analysis, position), to: Rename.Prepare + + @rename_mappings %{module: Rename.Module} + + @doc """ + Executes a rename operation. + + Renames the entity at the given position to `new_name`, returning a list + of document changes that should be applied. + + The `client_name` parameter is currently unused but reserved for future + client-specific behavior (e.g., different progress tracking for VSCode vs Neovim). + """ + @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, _client_name) do + with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do + rename_module = Map.fetch!(@rename_mappings, renamable) + results = rename_module.rename(range, new_name, entity) + {:ok, results} + end + end +end diff --git a/apps/engine/lib/engine/code_mod/rename/entry.ex b/apps/engine/lib/engine/code_mod/rename/entry.ex new file mode 100644 index 00000000..14e64a09 --- /dev/null +++ b/apps/engine/lib/engine/code_mod/rename/entry.ex @@ -0,0 +1,46 @@ +defmodule Engine.CodeMod.Rename.Entry do + @moduledoc """ + An entry wrapper for search indexer entries used in rename operations. + + When renaming, we rely on the `Forge.Search.Indexer.Entry`, + and we also need some other fields used exclusively for renaming, such as `edit_range`. + """ + alias Forge.Document.Range + alias Forge.Search.Indexer.Entry, as: IndexerEntry + + @type t :: %__MODULE__{ + id: IndexerEntry.entry_id(), + path: Forge.path(), + subject: IndexerEntry.subject(), + block_range: Range.t() | nil, + range: Range.t(), + edit_range: Range.t(), + subtype: IndexerEntry.entry_subtype() + } + + defstruct [ + :id, + :path, + :subject, + :block_range, + :range, + :edit_range, + :subtype + ] + + @doc """ + Creates a new Entry from an IndexerEntry. + """ + @spec new(IndexerEntry.t()) :: t() + def new(%IndexerEntry{} = indexer_entry) do + %__MODULE__{ + id: indexer_entry.id, + path: indexer_entry.path, + subject: indexer_entry.subject, + subtype: indexer_entry.subtype, + block_range: indexer_entry.block_range, + range: indexer_entry.range, + edit_range: indexer_entry.range + } + end +end diff --git a/apps/engine/lib/engine/code_mod/rename/file.ex b/apps/engine/lib/engine/code_mod/rename/file.ex new file mode 100644 index 00000000..4637f13d --- /dev/null +++ b/apps/engine/lib/engine/code_mod/rename/file.ex @@ -0,0 +1,165 @@ +defmodule Engine.CodeMod.Rename.File do + @moduledoc """ + Handles file renaming logic during module renaming operations. + + Determines if a file should be renamed when its containing module is renamed, + based on conventions and constraints. + """ + alias Engine.CodeMod.Rename.Entry + alias Engine.Search.Indexer + alias Forge.Ast + alias Forge.Document + alias Forge.ProcessCache + alias Forge.Project + alias Forge.Search.Indexer.Entry, as: IndexerEntry + + @doc """ + Determines if a file should be renamed when renaming a module. + + Returns a `Forge.Document.Changes.RenameFile` struct if the file should be renamed, + or `nil` if no file rename is needed. + """ + @spec maybe_rename(Document.t(), Entry.t(), String.t()) :: Document.Changes.RenameFile.t() | nil + 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{} = entry, document) do + entries = + ProcessCache.trans("#{document.uri}-entries", 50, fn -> + with {:ok, entries} <- + Indexer.Source.index_document(document, [Indexer.Extractors.Module]) do + entries + end + end) + + case Enum.filter(entries, &(&1.block_id == :root)) do + [%IndexerEntry{} = root_module] -> + root_module.subject == entry.subject and root_module.block_range == entry.block_range + + _ -> + false + end + end + + defp rename_file(document, %Entry{} = entry, new_suffix) do + root_path = root_path() + relative_path = Path.relative_to(entry.path, root_path) + + 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(entry.subject, relative_path) + + new_path = Path.join([root_path, prefix, "#{suffix}#{extname}"]) + new_uri = Document.Path.ensure_uri(new_path) + + if document.uri != new_uri do + Document.Changes.RenameFile.new(document.uri, new_uri) + end + else + _ -> nil + end + end + + defp root_path do + Project.root_path(Engine.get_project()) + end + + defp fetch_new_name(document, %Entry{} = entry, new_suffix) do + text_edits = [Document.Edit.new(new_suffix, entry.edit_range)] + + with {:ok, edited_document} <- + Document.apply_content_changes(document, document.version + 1, text_edits), + {:ok, %{context: {:alias, alias}}} <- + Ast.surround_context(edited_document, entry.edit_range.start) do + {:ok, to_string(alias)} + else + _ -> :error + end + 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"} + segments = + case Path.split(path) do + ["apps", app_name, "lib" | _] -> ["apps", app_name, "lib"] + ["apps", app_name, "test" | _] -> ["apps", app_name, "test"] + ["lib" | _] -> ["lib"] + ["test" | _] -> ["test"] + _ -> nil + end + + if segments do + {:ok, Path.join(segments)} + else + :error + end + end + + defp maybe_insert_special_phoenix_folder(suffix, subject, relative_path) do + insertions = + cond do + phoenix_controller_module?(subject) -> + "controllers" + + phoenix_liveview_module?(subject) -> + "live" + + phoenix_component_module?(subject) -> + "components" + + true -> + nil + 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 in Path.split(suffix) + old_path_contains_insertions? = insertions in Path.split(relative_path) + + 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) + |> Path.join() + else + suffix + 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 + + defp phoenix_component_module?(module) do + function_exists?(module, :__components__, 0) or + function_exists?(module, :__live__, 0) + end + + defp function_exists?(module, function, arity) do + function_exported?(module, function, arity) + end +end diff --git a/apps/engine/lib/engine/code_mod/rename/module.ex b/apps/engine/lib/engine/code_mod/rename/module.ex new file mode 100644 index 00000000..cd5a58b5 --- /dev/null +++ b/apps/engine/lib/engine/code_mod/rename/module.ex @@ -0,0 +1,261 @@ +defmodule Engine.CodeMod.Rename.Module do + @moduledoc """ + Handles module renaming logic. + + This module is responsible for: + - Recognizing if a position is at a module definition + - Preparing the rename range + - Executing the rename across all references + """ + alias Engine.CodeIntelligence.Entity + alias Engine.CodeMod.Rename + alias Engine.CodeMod.Rename.Entry + alias Engine.CodeMod.Rename.Module + alias Engine.Search.Store + alias Forge.Ast + alias Forge.Ast.Analysis + alias Forge.Document + alias Forge.Document.Edit + alias Forge.Document.Line + alias Forge.Document.Position + alias Forge.Document.Range + alias Forge.Formats + + require Logger + import Line + + @doc """ + Checks if the position is at a module that can be renamed. + """ + @spec recognizes?(Analysis.t(), Position.t()) :: boolean() + def recognizes?(%Analysis{} = analysis, %Position{} = position) do + case resolve(analysis, position) do + {:ok, _, _} -> + true + + _ -> + false + end + end + + @doc """ + Prepares the rename operation by returning the module and the range to be renamed. + """ + @spec prepare(Analysis.t(), Position.t()) :: + {:ok, {:module, atom()}, Range.t()} | {:error, tuple() | atom()} + def prepare(%Analysis{} = analysis, %Position{} = position) do + with {:ok, {:module, _module}, _range} <- resolve(analysis, position) do + {module, range} = surround_the_whole_module(analysis, position) + + if cursor_at_declaration?(module, range) do + {:ok, {:module, module}, range} + else + {:error, {:unsupported_location, :module}} + end + end + end + + @doc """ + Executes the rename operation, returning a list of document changes. + """ + @spec rename(Range.t(), String.t(), atom()) :: [Document.Changes.t()] + def rename(%Range{} = old_range, new_name, module) do + {to_be_renamed, replacement} = old_range |> range_text() |> Module.Diff.diff(new_name) + results = exacts(module, to_be_renamed) ++ descendants(module, to_be_renamed) + + for {uri, entries} <- Enum.group_by(results, &Document.Path.ensure_uri(&1.path)), + result = to_document_changes(uri, entries, replacement), + match?({:ok, _}, result) do + {:ok, document_changes} = result + document_changes + end + end + + defp resolve(%Analysis{} = analysis, %Position{} = position) do + case Entity.resolve(analysis, position) do + {:ok, {module_or_struct, module}, range} when module_or_struct in [:struct, :module] -> + {:ok, {:module, module}, range} + + _ -> + {:error, :not_a_module} + end + end + + defp resolve(path, %Position{} = position) do + uri = Document.Path.ensure_uri(path) + + with {:ok, _} <- Document.Store.open_temporary(uri), + {:ok, _document, analysis} <- Document.Store.fetch(uri, :analysis) do + resolve(analysis, position) + end + end + + defp cursor_at_declaration?(module, rename_range) do + case Store.exact(module, type: :module, subtype: :definition) do + {:ok, [definition]} -> + rename_range == definition.range + + _ -> + false + end + end + + defp surround_the_whole_module(analysis, position) do + # When renaming occurs, we want users to be able to choose any place in the defining module, + # not just the last local module, like: `defmodule |Foo.Bar do` also works. + {:ok, %{end: {_end_line, end_character}}} = Ast.surround_context(analysis, position) + end_position = %{position | character: end_character - 1} + {:ok, {:module, module}, range} = resolve(analysis, end_position) + {module, range} + end + + defp exacts(module, to_be_renamed) do + module + |> query_for_exacts() + |> Enum.filter(&entry_matching?(&1, to_be_renamed)) + |> adjust_range_for_exacts(to_be_renamed) + end + + defp descendants(module, to_be_renamed) do + module + |> query_for_descendants() + |> Enum.filter(&(entry_matching?(&1, to_be_renamed) and has_dots_in_range?(&1))) + |> adjust_range_for_descendants(module, to_be_renamed) + end + + defp query_for_exacts(module) do + module_string = Formats.module(module) + + case Store.exact(module_string, type: :module) do + {:ok, entries} -> Enum.map(entries, &Entry.new/1) + {:error, _} -> [] + end + end + + defp query_for_descendants(module) do + module_string = Formats.module(module) + prefix = "#{module_string}." + + case Store.prefix(prefix, type: :module) do + {:ok, entries} -> Enum.map(entries, &Entry.new/1) + {:error, _} -> [] + end + end + + defp maybe_rename_file(document, entries, replacement) do + entries + |> Enum.map(&Rename.File.maybe_rename(document, &1, replacement)) + # every group should have only one `rename_file` + |> Enum.find(&(not is_nil(&1))) + end + + defp entry_matching?(entry, to_be_renamed) do + entry.range |> range_text() |> String.contains?(to_be_renamed) + end + + defp has_dots_in_range?(entry) do + entry.range |> range_text() |> String.contains?(".") + end + + defp adjust_range_for_exacts(entries, to_be_renamed) do + old_suffix_length = String.length(to_be_renamed) + + for %Entry{} = entry <- entries do + start_character = entry.edit_range.end.character - old_suffix_length + put_in(entry.edit_range.start.character, start_character) + end + end + + defp adjust_range_for_descendants(entries, module, to_be_renamed) do + for %Entry{} = entry <- entries, + range_text = range_text(entry.edit_range), + matches = matches(range_text, to_be_renamed), + result = resolve_module_range(entry, module, matches), + match?({:ok, _}, result) do + {_, range} = result + %{entry | edit_range: range} + end + end + + defp range_text(range) do + line(text: text) = range.end.context_line + String.slice(text, range.start.character - 1, range.end.character - range.start.character) + end + + defp resolve_module_range(_entry, _module, []) do + {:error, :not_found} + end + + defp resolve_module_range(entry, module, [[{start, length}]]) do + range = adjust_range_characters(entry.edit_range, {start, length}) + + with {:ok, {:module, ^module}, _} <- resolve(entry.path, range.end) do + {:ok, range} + end + end + + defp resolve_module_range(entry, module, [[{start, length}] | tail] = _matches) do + # This function is mainly for the duplicated suffixes + # For example, if we have a module named `Foo.Bar.Foo.Bar` and we want to rename it to `Foo.Bar.Baz` + # The `Foo.Bar` will be duplicated in the range text, so we need to resolve the correct range + # and only rename the second occurrence of `Foo.Bar` + start_character = entry.edit_range.start.character + start + position = %{entry.edit_range.start | character: start_character} + + with {:ok, {:module, result}, range} <- resolve(entry.path, position) do + if result == module do + range = adjust_range_characters(range, {start, length}) + {:ok, range} + else + resolve_module_range(entry, module, tail) + end + end + end + + defp matches(range_text, "") do + # When expanding a module, the to_be_renamed is an empty string, + # so we need to scan the module before the period + for [{start, length}] <- Regex.scan(~r/\w+(?=\.)/, range_text, return: :index) do + [{start + length, 0}] + end + end + + defp matches(range_text, to_be_renamed) do + Regex.scan(~r/#{to_be_renamed}/, range_text, return: :index) + end + + defp adjust_range_characters(%Range{} = range, {start, length} = _matched_old_suffix) do + start_character = range.start.character + start + end_character = start_character + length + + range + |> put_in([:start, :character], start_character) + |> put_in([:end, :character], end_character) + end + + defp to_document_changes(uri, entries, replacement) do + with {:ok, document} <- Document.Store.open_temporary(uri) do + rename_file = maybe_rename_file(document, entries, replacement) + + edits = + Enum.map(entries, fn entry -> + reference? = entry.subtype == :reference + + if reference? and not ancestor_is_alias?(document, entry.edit_range.start) do + replacement = replacement |> String.split(".") |> Enum.at(-1) + Edit.new(replacement, entry.edit_range) + else + Edit.new(replacement, entry.edit_range) + end + end) + + {:ok, Document.Changes.new(document, edits, rename_file)} + end + end + + defp ancestor_is_alias?(%Document{} = document, %Position{} = position) do + document + |> Ast.cursor_path(position) + |> Enum.any?(&match?({:alias, _, _}, &1)) + end +end diff --git a/apps/engine/lib/engine/code_mod/rename/module/diff.ex b/apps/engine/lib/engine/code_mod/rename/module/diff.ex new file mode 100644 index 00000000..68a02280 --- /dev/null +++ b/apps/engine/lib/engine/code_mod/rename/module/diff.ex @@ -0,0 +1,50 @@ +defmodule Engine.CodeMod.Rename.Module.Diff do + @moduledoc """ + A module for computing the diff between old and new module names during renaming. + """ + + @doc """ + Computes the parts of the module name that need to be renamed and their replacements. + + Returns a tuple `{to_be_renamed, replacement}` where: + - `to_be_renamed` is the suffix of the old name that needs to be changed + - `replacement` is what it should be changed to + + ## Examples + + iex> diff("TopLevel.Parent.Child", "TopLevel.Renamed.Child") + {"Parent", "Renamed"} + + iex> diff("TopLevel.Parent.Child", "TopLevel.Child") + {"Parent.Child", "Child"} + + iex> diff("Foo.Bar", "Baz.Qux") + {"Foo.Bar", "Baz.Qux"} + """ + @spec diff(String.t(), String.t()) :: {String.t(), String.t()} + def diff(old_name, new_name) do + with [{:eq, eq} | _] <- String.myers_difference(old_name, new_name), + equal_segment <- trim_last_dot_part(eq), + true <- not is_nil(equal_segment) do + to_be_renamed = replace_leading_eq(old_name, equal_segment) + replacement = replace_leading_eq(new_name, equal_segment) + {to_be_renamed, replacement} + else + _ -> + {old_name, new_name} + end + end + + defp trim_last_dot_part(module) do + split = module |> String.reverse() |> String.split(".", parts: 2) + + if length(split) == 2 do + [_, rest] = split + rest |> String.reverse() + end + end + + defp replace_leading_eq(module, eq) do + module |> String.replace(~r"^#{eq}", "") |> String.trim_leading(".") + end +end diff --git a/apps/engine/lib/engine/code_mod/rename/prepare.ex b/apps/engine/lib/engine/code_mod/rename/prepare.ex new file mode 100644 index 00000000..7f976fef --- /dev/null +++ b/apps/engine/lib/engine/code_mod/rename/prepare.ex @@ -0,0 +1,70 @@ +defmodule Engine.CodeMod.Rename.Prepare do + @moduledoc """ + Handles the preparation phase of rename operations. + + The preparation phase determines: + - Whether the entity at the cursor can be renamed + - What the current name is + - What range should be replaced + """ + alias Engine.CodeIntelligence.Entity + alias Engine.CodeMod.Rename + alias Forge.Ast.Analysis + alias Forge.Document.Position + alias Forge.Document.Range + alias Forge.Formats + + require Logger + + @renaming_modules [Rename.Module] + + @doc """ + Prepares a rename operation at the given position. + + Returns `{:ok, module_name_string, range}` if the entity can be renamed, + or an appropriate error. + """ + @spec prepare(Analysis.t(), Position.t()) :: + {:ok, String.t(), Range.t()} | {:ok, nil} | {:error, term()} + def prepare(%Analysis{} = analysis, %Position{} = position) do + case resolve(analysis, position) do + {:ok, {:module, module}, range} -> + {:ok, Formats.module(module), range} + + {:error, {:unsupported_location, _}} -> + {:ok, nil} + + {:error, {:unsupported_entity, entity_type}} -> + {:error, "Renaming #{inspect(entity_type)} is not supported for now"} + + {:error, error} -> + {:error, error} + end + end + + @doc """ + Resolves the entity at the given position for renaming. + + Returns `{:ok, {entity_type, entity}, range}` if the entity can be renamed, + or an error tuple. + """ + @spec resolve(Analysis.t(), Position.t()) :: + {:ok, {atom(), atom()}, Range.t()} | {:error, tuple() | atom()} + def resolve(%Analysis{} = analysis, %Position{} = position) do + prepare_result = + Enum.find_value(@renaming_modules, fn module -> + if module.recognizes?(analysis, position) do + module.prepare(analysis, position) + end + end) + + prepare_result || handle_unsupported_entity(analysis, position) + end + + defp handle_unsupported_entity(analysis, position) do + with {:ok, other, _range} <- Entity.resolve(analysis, position) do + Logger.info("Unsupported entity for renaming: #{inspect(other)}") + {:error, {:unsupported_entity, elem(other, 0)}} + end + end +end diff --git a/apps/engine/test/engine/code_mod/rename/module/diff_test.exs b/apps/engine/test/engine/code_mod/rename/module/diff_test.exs new file mode 100644 index 00000000..d5cc2258 --- /dev/null +++ b/apps/engine/test/engine/code_mod/rename/module/diff_test.exs @@ -0,0 +1,27 @@ +defmodule Engine.CodeMod.Rename.Module.DiffTest do + alias Engine.CodeMod.Rename.Module.Diff + + use ExUnit.Case, async: true + + describe "diff/2" do + test "returns the local module pair if only the local name is changed" do + assert Diff.diff("A.B.C", "A.B.D") == {"C", "D"} + end + + test "returns the local module pair even if the part of local name is changed" do + assert Diff.diff("A.B.CD", "A.B.CC") == {"CD", "CC"} + end + + test "returns the suffix when extending the middle part" do + assert Diff.diff("Foo.Bar", "Foo.Baz.Bar") == {"Bar", "Baz.Bar"} + end + + test "returns the suffix if the middle part is removed" do + assert Diff.diff("Foo.Baz.Bar", "Foo.Bar") == {"Baz.Bar", "Bar"} + end + + test "returns the entire module pair if the change starts from the first module" do + assert Diff.diff("Foo.Bar", "Foa.Bar") == {"Foo.Bar", "Foa.Bar"} + end + end +end diff --git a/apps/engine/test/engine/code_mod/rename_test.exs b/apps/engine/test/engine/code_mod/rename_test.exs new file mode 100644 index 00000000..d188f1af --- /dev/null +++ b/apps/engine/test/engine/code_mod/rename_test.exs @@ -0,0 +1,193 @@ +defmodule Engine.CodeMod.RenameTest do + alias Engine.CodeMod.Rename + alias Engine.Search + alias Engine.Search.Store.Backends + alias Forge.Document + + use ExUnit.Case, async: false + use Patch + + import Forge.Test.CodeSigil + import Forge.Test.CursorSupport + import Forge.Test.Fixtures + import Forge.Test.EventualAssertions + + setup do + project = project() + + Backends.Ets.destroy_all(project) + Engine.set_project(project) + + start_supervised!({Document.Store, derive: [analysis: &Forge.Ast.analyze/1]}) + start_supervised!(Engine.Dispatch) + start_supervised!(Backends.Ets) + + start_supervised!( + {Search.Store, [project, fn _ -> {:ok, []} end, fn _, _ -> {:ok, [], []} end, Backends.Ets]} + ) + + Search.Store.enable() + assert_eventually Search.Store.loaded?(), 1500 + + on_exit(fn -> + Backends.Ets.destroy_all(project) + end) + + {:ok, project: project} + end + + describe "prepare/2" do + test "returns the module name" do + {:ok, result, _} = + ~q[ + defmodule |Foo do + end + ] + |> prepare() + + assert result == "Foo" + end + + test "returns the whole module name" do + {:ok, result, _} = + ~q[ + defmodule TopLevel.|Foo do + end + ] + |> prepare() + + assert result == "TopLevel.Foo" + end + + test "returns the whole module name even if the cursor is not at the end" do + {:ok, result, _} = + ~q[ + defmodule Top|Level.Foo do + end + ] + |> prepare() + + assert result == "TopLevel.Foo" + end + + test "returns `nil` when renaming a module occurs in a reference" do + assert {:ok, nil} = + ~q[ + defmodule Foo do + end + + defmodule Bar do + alias |Foo + end + ] + |> prepare() + end + + test "returns error when the entity is not found" do + assert {:error, "Renaming :variable is not supported for now"} = + ~q[ + x = 1 + |x + ] + |> prepare() + end + end + + describe "rename exact module" do + test "succeeds when the cursor is at the definition" do + {:ok, result} = + ~q[ + defmodule |Foo do + end + ] + |> rename("Renamed") + + assert result =~ ~S[defmodule Renamed do] + end + + test "failed when the cursor is at the alias" do + assert {:error, {:unsupported_location, :module}} == + ~q[ + defmodule Baz do + alias |Foo + end + ] + |> rename("Renamed") + end + + test "succeeds when the module has multiple dots" do + {:ok, result} = + ~q[ + defmodule TopLevel.Foo.|Bar do + end + ] + |> rename("TopLevel.Foo.Renamed") + + assert result =~ ~S[defmodule TopLevel.Foo.Renamed do] + end + + test "succeeds when renaming the middle part of the module" do + {:ok, result} = + ~q[ + defmodule TopLevel.Foo.|Bar do + end + ] + |> rename("TopLevel.Renamed.Bar") + + assert result =~ ~S[defmodule TopLevel.Renamed.Bar do] + end + + test "succeeds when simplifying the module name" do + {:ok, result} = + ~q[ + defmodule TopLevel.Foo.|Bar do + end + ] + |> rename("TopLevel.Bar") + + assert result =~ ~S[defmodule TopLevel.Bar do] + end + end + + defp prepare(code) do + with {position, code} <- pop_cursor(code), + {:ok, _document, analysis} <- index(code) do + Rename.prepare(analysis, position) + end + end + + defp rename(code, new_name) do + with {position, code} <- pop_cursor(code), + {:ok, document, analysis} <- index(code), + {:ok, results} <- Rename.rename(analysis, position, new_name, nil) do + case results do + [%Document.Changes{edits: edits, document: doc}] -> + {:ok, edited_doc} = + Document.apply_content_changes(doc, doc.version + 1, edits) + + {:ok, Document.to_string(edited_doc)} + + [] -> + {:ok, Document.to_string(document)} + end + end + end + + defp index(code) do + project = project() + uri = module_uri(project) + + with :ok <- Document.Store.open(uri, code, 1), + {:ok, document, analysis} <- Document.Store.fetch(uri, :analysis), + {:ok, entries} <- Engine.Search.Indexer.Quoted.index(analysis) do + Search.Store.replace(entries) + {:ok, document, analysis} + end + end + + defp module_uri(project) do + project + |> file_path(Path.join("lib", "my_module.ex")) + |> Document.Path.ensure_uri() + end +end diff --git a/apps/expert/lib/expert.ex b/apps/expert/lib/expert.ex index dc193d73..a207c860 100644 --- a/apps/expert/lib/expert.ex +++ b/apps/expert/lib/expert.ex @@ -274,6 +274,12 @@ defmodule Expert do %GenLSP.Requests.WorkspaceSymbol{} -> {:ok, Handlers.WorkspaceSymbol} + %GenLSP.Requests.TextDocumentPrepareRename{} -> + {:ok, Handlers.PrepareRename} + + %GenLSP.Requests.TextDocumentRename{} -> + {:ok, Handlers.Rename} + %request_module{} -> {:error, {:unhandled, request_module}} end diff --git a/apps/expert/lib/expert/engine_api.ex b/apps/expert/lib/expert/engine_api.ex index 46aa28e3..0ba54c61 100644 --- a/apps/expert/lib/expert/engine_api.ex +++ b/apps/expert/lib/expert/engine_api.ex @@ -147,5 +147,24 @@ defmodule Expert.EngineApi do call(project, Engine, :workspace_symbols, [query]) end + def prepare_rename(%Project{} = project, %Analysis{} = analysis, %Position{} = position) do + call(project, Engine, :prepare_rename, [analysis, position]) + end + + def rename( + %Project{} = project, + %Analysis{} = analysis, + %Position{} = position, + new_name, + client_name + ) do + call(project, Engine, :rename, [ + analysis, + position, + new_name, + client_name + ]) + end + defdelegate stop(project), to: EngineNode end diff --git a/apps/expert/lib/expert/provider/handlers/prepare_rename.ex b/apps/expert/lib/expert/provider/handlers/prepare_rename.ex new file mode 100644 index 00000000..5d988562 --- /dev/null +++ b/apps/expert/lib/expert/provider/handlers/prepare_rename.ex @@ -0,0 +1,68 @@ +defmodule Expert.Provider.Handlers.PrepareRename do + @moduledoc """ + Handler for textDocument/prepareRename requests. + + This handler determines if the entity at the cursor can be renamed + and returns the range and placeholder text for the rename operation. + """ + alias Expert.Configuration + alias Expert.EngineApi + alias Forge.Ast + alias Forge.Document + alias GenLSP.Structures + + require Logger + + def handle( + %GenLSP.Requests.TextDocumentPrepareRename{ + params: %Structures.PrepareRenameParams{} = params + }, + %Configuration{} = config + ) do + document = Forge.Document.Container.context_document(params, nil) + + case Document.Store.fetch(document.uri, :analysis) do + {:ok, _document, %Ast.Analysis{valid?: true} = analysis} -> + prepare_rename(config.project, analysis, params.position) + + _ -> + {:error, :request_failed, "Document cannot be analyzed"} + end + end + + defp prepare_rename(project, analysis, position) do + case EngineApi.prepare_rename(project, analysis, position) do + {:ok, placeholder, range} when is_binary(placeholder) -> + # PrepareRenameResult is a type alias in GenLSP, return as map + result = %{ + placeholder: placeholder, + range: to_lsp_range(range) + } + + {:ok, result} + + {:ok, nil} -> + {:ok, nil} + + {:error, error} when is_binary(error) -> + {:error, :request_failed, error} + + {:error, error} -> + {:error, :request_failed, inspect(error)} + end + end + + defp to_lsp_range(%Forge.Document.Range{} = range) do + %Structures.Range{ + start: to_lsp_position(range.start), + end: to_lsp_position(range.end) + } + end + + defp to_lsp_position(%Forge.Document.Position{} = position) do + %Structures.Position{ + line: position.line - 1, + character: position.character - 1 + } + end +end diff --git a/apps/expert/lib/expert/provider/handlers/rename.ex b/apps/expert/lib/expert/provider/handlers/rename.ex new file mode 100644 index 00000000..8b489d47 --- /dev/null +++ b/apps/expert/lib/expert/provider/handlers/rename.ex @@ -0,0 +1,107 @@ +defmodule Expert.Provider.Handlers.Rename do + @moduledoc """ + Handler for textDocument/rename requests. + + This handler executes the rename operation and returns the workspace edit + containing all the text edits and file renames needed. + """ + alias Expert.Configuration + alias Expert.EngineApi + alias Forge.Ast + alias Forge.Document + alias Forge.Document.Changes + alias GenLSP.Structures + + require Logger + + def handle( + %GenLSP.Requests.TextDocumentRename{ + params: %Structures.RenameParams{} = params + }, + %Configuration{} = config + ) do + document = Forge.Document.Container.context_document(params, nil) + + case Document.Store.fetch(document.uri, :analysis) do + {:ok, _document, %Ast.Analysis{valid?: true} = analysis} -> + rename(config, analysis, params.position, params.new_name) + + _ -> + {:error, :request_failed, "Document cannot be analyzed"} + end + end + + defp rename(%Configuration{} = config, analysis, position, new_name) do + %Configuration{project: project, client_name: client_name} = config + + case EngineApi.rename(project, analysis, position, new_name, client_name) do + {:ok, []} -> + {:ok, nil} + + {:ok, results} -> + document_changes = + Enum.flat_map(results, fn + %Changes{rename_file: %Changes.RenameFile{}} = changes -> + [to_text_document_edit(changes), to_rename_file(changes.rename_file)] + + %Changes{} = changes -> + [to_text_document_edit(changes)] + end) + + workspace_edit = %Structures.WorkspaceEdit{document_changes: document_changes} + {:ok, workspace_edit} + + {:error, {:unsupported_entity, entity}} -> + Logger.info("Cannot rename entity: #{inspect(entity)}") + {:ok, nil} + + {:error, reason} -> + {:error, :request_failed, inspect(reason)} + end + end + + defp to_text_document_edit(%Changes{} = changes) do + %Changes{document: document, edits: edits} = changes + + text_document = + %Structures.OptionalVersionedTextDocumentIdentifier{ + uri: document.uri, + version: document.version + } + + %Structures.TextDocumentEdit{ + edits: Enum.map(edits, &to_text_edit/1), + text_document: text_document + } + end + + defp to_text_edit(%Document.Edit{} = edit) do + %Structures.TextEdit{ + new_text: edit.text, + range: to_lsp_range(edit.range) + } + end + + defp to_lsp_range(%Document.Range{} = range) do + %Structures.Range{ + start: to_lsp_position(range.start), + end: to_lsp_position(range.end) + } + end + + defp to_lsp_position(%Document.Position{} = position) do + %Structures.Position{ + line: position.line - 1, + character: position.character - 1 + } + end + + defp to_rename_file(%Changes.RenameFile{} = rename_file) do + %Structures.RenameFile{ + kind: "rename", + new_uri: rename_file.new_uri, + old_uri: rename_file.old_uri, + options: %Structures.RenameFileOptions{overwrite: true} + } + end +end diff --git a/apps/expert/lib/expert/state.ex b/apps/expert/lib/expert/state.ex index 2ba810b4..b2edb2af 100644 --- a/apps/expert/lib/expert/state.ex +++ b/apps/expert/lib/expert/state.ex @@ -221,6 +221,11 @@ defmodule Expert.State do trigger_characters: CodeIntelligence.Completion.trigger_characters() } + rename_options = + %Structures.RenameOptions{ + prepare_provider: true + } + server_capabilities = %Structures.ServerCapabilities{ code_action_provider: code_action_options, @@ -232,6 +237,7 @@ defmodule Expert.State do execute_command_provider: command_options, hover_provider: true, references_provider: true, + rename_provider: rename_options, text_document_sync: sync_options, workspace_symbol_provider: true } diff --git a/apps/forge/lib/forge/document/changes.ex b/apps/forge/lib/forge/document/changes.ex index 2911dd80..d7fedf9c 100644 --- a/apps/forge/lib/forge/document/changes.ex +++ b/apps/forge/lib/forge/document/changes.ex @@ -9,23 +9,42 @@ defmodule Forge.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] + defstruct [:document, :edits, :rename_file] alias Forge.Document use Forge.StructAccess + defmodule RenameFile do + @moduledoc """ + Represents a file rename operation during module renaming. + """ + defstruct [:old_uri, :new_uri] + + @type t :: %__MODULE__{ + old_uri: Forge.uri(), + new_uri: Forge.uri() + } + + @spec new(Forge.uri(), Forge.uri()) :: t() + def new(old_uri, new_uri) do + %__MODULE__{old_uri: old_uri, new_uri: new_uri} + end + end + @type edits :: Document.Edit.t() | [Document.Edit.t()] + @type rename_file :: RenameFile.t() | nil @type t :: %__MODULE__{ document: Document.t(), - edits: edits + edits: edits, + rename_file: rename_file } @doc """ Creates a new Changes struct given a document and edits. """ - @spec new(Document.t(), edits()) :: t() - def new(document, edits) do - %__MODULE__{document: document, edits: edits} + @spec new(Document.t(), edits(), rename_file()) :: t() + def new(document, edits, rename_file \\ nil) do + %__MODULE__{document: document, edits: edits, rename_file: rename_file} end end diff --git a/apps/forge/lib/forge/engine_api/messages.ex b/apps/forge/lib/forge/engine_api/messages.ex index 1adfb96a..4c6741dc 100644 --- a/apps/forge/lib/forge/engine_api/messages.ex +++ b/apps/forge/lib/forge/engine_api/messages.ex @@ -27,6 +27,8 @@ defmodule Forge.EngineApi.Messages do defrecord :file_deleted, project: nil, uri: nil + defrecord :file_saved, uri: nil + defrecord :module_updated, file: nil, name: nil, functions: [], macros: [], struct: nil defrecord :project_diagnostics, project: nil, build_number: 0, diagnostics: [] @@ -91,6 +93,8 @@ defmodule Forge.EngineApi.Messages do elapsed_ms: non_neg_integer ) + @type file_saved :: record(:file_saved, uri: Forge.uri()) + @type module_updated :: record(:module_updated, name: module(), From a38652ef9b9d2897c453cb6793d72c457af4c05b Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sun, 4 Jan 2026 10:35:18 +0800 Subject: [PATCH 2/6] test(rename): expand test coverage with realistic scenarios - Refactor test descriptions to be concise - Replace generic Foo/Bar/Baz with domain names (Users, Accounts, etc.) - Add tests for references, descendants, structs, edge cases, and file renaming --- .../test/engine/code_mod/rename_test.exs | 787 +++++++++++++++++- 1 file changed, 751 insertions(+), 36 deletions(-) diff --git a/apps/engine/test/engine/code_mod/rename_test.exs b/apps/engine/test/engine/code_mod/rename_test.exs index d188f1af..48a9a48f 100644 --- a/apps/engine/test/engine/code_mod/rename_test.exs +++ b/apps/engine/test/engine/code_mod/rename_test.exs @@ -27,7 +27,7 @@ defmodule Engine.CodeMod.RenameTest do ) Search.Store.enable() - assert_eventually Search.Store.loaded?(), 1500 + assert_eventually(Search.Store.loaded?(), 1500) on_exit(fn -> Backends.Ets.destroy_all(project) @@ -37,53 +37,53 @@ defmodule Engine.CodeMod.RenameTest do end describe "prepare/2" do - test "returns the module name" do + test "returns module name" do {:ok, result, _} = ~q[ - defmodule |Foo do + defmodule |Users do end ] |> prepare() - assert result == "Foo" + assert result == "Users" end - test "returns the whole module name" do + test "returns full module name" do {:ok, result, _} = ~q[ - defmodule TopLevel.|Foo do + defmodule MyApp.|Users do end ] |> prepare() - assert result == "TopLevel.Foo" + assert result == "MyApp.Users" end - test "returns the whole module name even if the cursor is not at the end" do + test "returns full module name when cursor is in the middle" do {:ok, result, _} = ~q[ - defmodule Top|Level.Foo do + defmodule My|App.Users do end ] |> prepare() - assert result == "TopLevel.Foo" + assert result == "MyApp.Users" end - test "returns `nil` when renaming a module occurs in a reference" do + test "returns nil for module reference" do assert {:ok, nil} = ~q[ - defmodule Foo do + defmodule MyApp.Users do end - defmodule Bar do - alias |Foo + defmodule MyApp.Accounts do + alias |MyApp.Users end ] |> prepare() end - test "returns error when the entity is not found" do + test "returns error for unsupported entity" do assert {:error, "Renaming :variable is not supported for now"} = ~q[ x = 1 @@ -93,62 +93,743 @@ defmodule Engine.CodeMod.RenameTest do end end - describe "rename exact module" do - test "succeeds when the cursor is at the definition" do + describe "rename/4 basic" do + test "renames at definition" do {:ok, result} = ~q[ - defmodule |Foo do + defmodule |Users do end ] - |> rename("Renamed") + |> rename("Accounts") - assert result =~ ~S[defmodule Renamed do] + assert result =~ ~S[defmodule Accounts do] end - test "failed when the cursor is at the alias" do + test "fails at alias reference" do assert {:error, {:unsupported_location, :module}} == ~q[ - defmodule Baz do - alias |Foo + defmodule MyApp.Accounts do + alias |MyApp.Users end ] - |> rename("Renamed") + |> rename("Members") end - test "succeeds when the module has multiple dots" do + test "fails at module call reference" do + assert {:error, {:unsupported_location, :module}} == + ~q[ + defmodule MyApp.Accounts do + alias MyApp.Users + Use|rs.list() + end + ] + |> rename("Members") + end + + test "renames multi-part module" do + {:ok, result} = + ~q[ + defmodule MyApp.Auth.|Session do + end + ] + |> rename("MyApp.Auth.Token") + + assert result =~ ~S[defmodule MyApp.Auth.Token do] + end + + test "renames middle segment" do + {:ok, result} = + ~q[ + defmodule MyApp.Auth.|Session do + end + ] + |> rename("MyApp.Identity.Session") + + assert result =~ ~S[defmodule MyApp.Identity.Session do] + end + + test "simplifies module name" do + {:ok, result} = + ~q[ + defmodule MyApp.Auth.|Session do + end + ] + |> rename("MyApp.Session") + + assert result =~ ~S[defmodule MyApp.Session do] + end + + test "renames nested module definition" do + {:ok, result} = + ~q[ + defmodule MyApp.Users do + defmodule |Query do + end + end + + defmodule MyApp.UsersTest do + alias MyApp.Users.Query + end + ] + |> rename("Filter") + + assert result == ~q[ + defmodule MyApp.Users do + defmodule Filter do + end + end + + defmodule MyApp.UsersTest do + alias MyApp.Users.Filter + end + ] + end + + test "renames multi-alias syntax" do + {:ok, result} = + ~q[ + defmodule MyApp.|Accounts do + end + + defmodule MyApp.Web do + alias MyApp.{ + Users, Accounts, + Auth.Session + } + end + ] + |> rename("MyApp.Members") + + assert result =~ ~S[ Users, Members,] + end + end + + describe "rename/4 with references" do + test "does not rename similar module names" do + {:ok, result} = + ~q[ + defmodule |MyApp.Users do + end + + defmodule MyApp.UsersTest do + end + ] + |> rename("MyApp.Accounts") + + assert result =~ ~S[defmodule MyApp.UsersTest do] + end + + test "renames local references when local name changes" do + {:ok, result} = + ~q[ + defmodule |MyApp.Users do + end + + defmodule MyApp.UsersTest do + alias MyApp.Users + Users.list() + end + ] + |> rename("MyApp.Accounts") + + assert result =~ ~S[alias MyApp.Accounts] + assert result =~ ~S[ Accounts.list()] + end + + test "keeps local reference when local name unchanged" do + {:ok, result} = + ~q[ + defmodule |MyApp.Users do + end + + defmodule MyApp.Admin do + alias MyApp.Users + + Users.list() # no change + end + ] + |> rename("MyApp.Core.Users") + + assert result =~ ~S[defmodule MyApp.Core.Users do] + assert result =~ ~S[alias MyApp.Core.Users] + assert result =~ ~S[ Users.list() # no change] + end + + test "keeps local reference when only prefix changes" do + {:ok, result} = + ~q[ + defmodule |MyApp.Core.Utils do + end + + defmodule MyApp.Web do + alias MyApp.Core.Utils + + Utils.format() # no change + end + ] + |> rename("MyApp.Shared.Utils") + + assert result =~ ~S[defmodule MyApp.Shared.Utils do] + assert result =~ ~S[alias MyApp.Shared.Utils] + assert result =~ ~S[ Utils.format() # no change] + end + end + + describe "rename/4 descendants" do + test "renames descendants" do + {:ok, result} = + ~q[ + defmodule MyApp.|Users do + end + + defmodule MyApp.Users.Query do + end + ] + |> rename("MyApp.Accounts") + + assert result =~ ~S[defmodule MyApp.Accounts.Query] + assert result =~ ~S[defmodule MyApp.Accounts do] + end + + test "renames descendants when expanding name" do + {:ok, result} = + ~q[ + defmodule MyApp.|Users do + alias MyApp.Users.Query + end + + defmodule MyApp.Users.Query do + end + ] + |> rename("MyApp.Core.Users") + + assert result =~ ~S[defmodule MyApp.Core.Users] + assert result =~ ~S[alias MyApp.Core.Users.Query] + assert result =~ ~S[defmodule MyApp.Core.Users.Query do] + end + + test "renames descendants with multi-part expansion" do + {:ok, result} = + ~q[ + defmodule MyApp.|Auth do + end + + defmodule MyApp.Auth.Session do + end + + defmodule MyApp.AuthTest do + alias MyApp.Auth + alias MyApp.Auth.Session + end + ] + |> rename("MyApp.Auth.OAuth") + + assert result =~ ~S[defmodule MyApp.Auth.OAuth do] + assert result =~ ~S[alias MyApp.Auth.OAuth] + assert result =~ ~S[alias MyApp.Auth.OAuth.Session] + end + + test "handles repeated module name in path" do + {:ok, result} = + ~q[ + defmodule MyApp.Core.MyApp.|Core do + end + + defmodule MyApp.Core.MyApp.Core.Utils do + end + + defmodule MyApp.Web do + alias MyApp.Core.MyApp.Core.Utils + end + ] + |> rename("MyApp.Core.MyApp.Shared") + + assert result =~ ~S[defmodule MyApp.Core.MyApp.Shared do] + assert result =~ ~S[defmodule MyApp.Core.MyApp.Shared.Utils do] + assert result =~ ~S[alias MyApp.Core.MyApp.Shared.Utils] + end + + test "skips same-named nested modules" do + {:ok, result} = + ~q[ + defmodule MyApp.|Users do + defmodule Users do # skip this + end + end + + defmodule MyApp.Admin do + alias MyApp.Users.Users + end + ] + |> rename("MyApp.Accounts") + + assert result =~ ~S[defmodule MyApp.Accounts do] + assert result =~ ~S[defmodule Users do # skip this] + assert result =~ ~S[alias MyApp.Accounts.Users] + end + + test "renames when removing middle segment" do + {:ok, result} = + ~q[ + defmodule |MyApp.Core.Users do + end + + defmodule MyApp.Core.Users.Query do + alias MyApp.Core.Users.Query + end + ] + |> rename("MyApp.Users") + + assert result =~ ~S[defmodule MyApp.Users.Query do] + assert result =~ ~S[alias MyApp.Users.Query] + end + + test "does not rename modules containing old suffix as substring" do + {:ok, result} = + ~q[ + defmodule |MyApp.Ast do + end + + defmodule MyApp.Parser do + alias MyApp.Ast.Detection + + Detection.Bitstring.detected?() # Bitstring contains the old suffix: `st` + end + ] + |> rename("MyApp.AST") + + refute result =~ ~S[Detection.BitSTring.detected?()] + end + end + + describe "rename/4 struct" do + test "renames struct references" do + {:ok, result} = + ~q[ + defmodule |MyApp.User do + defstruct name: nil, email: nil + end + + defmodule MyApp.Accounts do + def new do + %MyApp.User{} + end + end + ] + |> rename("MyApp.Account") + + assert result =~ ~S[defmodule MyApp.Account do] + assert result =~ ~S[%MyApp.Account{}] + end + + test "renames struct in pattern matching" do + {:ok, result} = + ~q[ + defmodule |MyApp.User do + defstruct name: nil, email: nil + end + + defmodule MyApp.Accounts do + def get_name(%MyApp.User{name: name}), do: name + end + ] + |> rename("MyApp.Account") + + assert result =~ ~S[def get_name(%MyApp.Account{name: name})] + end + + test "renames struct update syntax" do + {:ok, result} = + ~q[ + defmodule |MyApp.User do + defstruct name: nil, email: nil + end + + defmodule MyApp.Accounts do + def update(user, name) do + %MyApp.User{user | name: name} + end + end + ] + |> rename("MyApp.Account") + + assert result =~ ~S[%MyApp.Account{user | name: name}] + end + end + + describe "rename/4 edge cases" do + test "does not rename module with similar prefix" do + # MyApp.User should not affect MyApp.UserProfile + {:ok, result} = + ~q[ + defmodule |MyApp.User do + end + + defmodule MyApp.UserProfile do + end + + defmodule MyApp.Consumer do + alias MyApp.User + alias MyApp.UserProfile + end + ] + |> rename("MyApp.Account") + + assert result =~ ~S[defmodule MyApp.Account do] + # UserProfile should NOT be renamed + assert result =~ ~S[defmodule MyApp.UserProfile do] + assert result =~ ~S[alias MyApp.UserProfile] + end + + test "does not rename module that contains old name as substring" do + # Renaming MyApp.API should not affect MyApp.SomeAPIService {:ok, result} = ~q[ - defmodule TopLevel.Foo.|Bar do + defmodule |MyApp.API do + end + + defmodule MyApp.SomeAPIService do end ] - |> rename("TopLevel.Foo.Renamed") + |> rename("MyApp.Gateway") - assert result =~ ~S[defmodule TopLevel.Foo.Renamed do] + assert result =~ ~S[defmodule MyApp.Gateway do] + # SomeAPIService should NOT be renamed to SomeGatewayService + assert result =~ ~S[defmodule MyApp.SomeAPIService do] end - test "succeeds when renaming the middle part of the module" do + test "correctly renames when old and new names share common prefix" do + # MyApp.User -> MyApp.UserAccount (extending the name) {:ok, result} = ~q[ - defmodule TopLevel.Foo.|Bar do + defmodule |MyApp.User do + end + + defmodule MyApp.Consumer do + alias MyApp.User end ] - |> rename("TopLevel.Renamed.Bar") + |> rename("MyApp.UserAccount") - assert result =~ ~S[defmodule TopLevel.Renamed.Bar do] + assert result =~ ~S[defmodule MyApp.UserAccount do] + assert result =~ ~S[alias MyApp.UserAccount] end - test "succeeds when simplifying the module name" do + test "correctly renames when new name is shorter" do + # MyApp.UserAccount -> MyApp.User (shortening the name) {:ok, result} = ~q[ - defmodule TopLevel.Foo.|Bar do + defmodule |MyApp.UserAccount do + end + + defmodule MyApp.Consumer do + alias MyApp.UserAccount end ] - |> rename("TopLevel.Bar") + |> rename("MyApp.User") - assert result =~ ~S[defmodule TopLevel.Bar do] + assert result =~ ~S[defmodule MyApp.User do] + assert result =~ ~S[alias MyApp.User] + end + + test "handles renaming module with single segment name" do + {:ok, result} = + ~q[ + defmodule |Users do + end + + defmodule Consumer do + alias Users + end + ] + |> rename("Accounts") + + assert result =~ ~S[defmodule Accounts do] + assert result =~ ~S[alias Accounts] + end + + test "handles renaming to completely different module path" do + # MyApp.Users -> OtherApp.Accounts (different prefix entirely) + {:ok, result} = + ~q[ + defmodule |MyApp.Users do + end + + defmodule MyApp.Consumer do + alias MyApp.Users + end + ] + |> rename("OtherApp.Accounts") + + assert result =~ ~S[defmodule OtherApp.Accounts do] + assert result =~ ~S[alias OtherApp.Accounts] + end + + test "renames deeply nested module correctly" do + {:ok, result} = + ~q[ + defmodule |MyApp.Core.Domain.Users.Query do + end + + defmodule MyApp.Web do + alias MyApp.Core.Domain.Users.Query + end + ] + |> rename("MyApp.Core.Domain.Users.Filter") + + assert result =~ ~S[defmodule MyApp.Core.Domain.Users.Filter do] + assert result =~ ~S[alias MyApp.Core.Domain.Users.Filter] + end + + test "does not corrupt nearby code when renaming" do + {:ok, result} = + ~q[ + defmodule |MyApp.Users do + @moduledoc "Users module" + + def list, do: :ok + end + + defmodule MyApp.UsersTest do + use ExUnit.Case + end + ] + |> rename("MyApp.Accounts") + + # Verify the module is renamed + assert result =~ ~S[defmodule MyApp.Accounts do] + # Verify other code is preserved + assert result =~ ~S[@moduledoc "Users module"] + assert result =~ ~S[def list, do: :ok] + # UsersTest should NOT be renamed (different module) + assert result =~ ~S[defmodule MyApp.UsersTest do] end end + describe "rename/4 file renaming" do + setup do + patch(Engine.CodeMod.Rename.File, :function_exists?, false) + :ok + end + + test "does not rename file with parent module", %{project: project} do + {:ok, {_applied, nil}} = + ~q[ + defmodule MyApp.Server do + defmodule |State do + end + end + ] + |> rename_with_file("Config", "lib/my_app/server.ex", project) + end + + test "does not rename file with sibling modules", %{project: project} do + assert {:ok, {_applied, nil}} = + ~q[ + defmodule |MyApp.Users do + end + + defmodule MyApp.Accounts do + end + ] + |> rename_with_file("Members", "lib/my_app/users.ex", project) + end + + test "does not rename file for non-conventional path", %{project: project} do + assert {:ok, {_applied, nil}} = + ~q[ + defmodule |MyApp.MixProject do + end + ] + |> rename_with_file("MyApp.Renamed", "mix.exs", project) + end + + test "renames lib file", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |MyApp.Users do + end + ] + |> rename_with_file("MyApp.Accounts", "lib/my_app/users.ex", project) + + assert rename_file.new_uri == subject_uri(project, "lib/my_app/accounts.ex") + end + + test "does not rename file when only case changes", %{project: project} do + assert {:ok, {_applied, nil}} = + ~q[ + defmodule |MyApp.Users do + end + ] + |> rename_with_file("MyApp.USERS", "lib/my_app/users.ex", project) + end + + test "renames umbrella app file", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |MyApp.Users do + end + ] + |> rename_with_file("MyApp.Accounts", "apps/my_app/lib/my_app/users.ex", project) + + assert rename_file.new_uri == subject_uri(project, "apps/my_app/lib/my_app/accounts.ex") + end + + test "renames umbrella app nested file", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |Engine.CodeMod do + end + ] + |> rename_with_file( + "Engine.Refactor", + "apps/engine/lib/engine/code_mod.ex", + project + ) + + assert rename_file.new_uri == + subject_uri(project, "apps/engine/lib/engine/refactor.ex") + end + + test "renames test file", %{project: project} do + {:ok, {_applied, rename_file}} = + ~q[ + defmodule |MyApp.UsersTest do + end + ] + |> rename_with_file("MyApp.AccountsTest", "test/my_app/users_test.exs", project) + + assert rename_file.new_uri == subject_uri(project, "test/my_app/accounts_test.exs") + end + + test "preserves components folder for phoenix component", %{project: project} do + patch(Engine.CodeMod.Rename.File, :phoenix_component_module?, fn MyAppWeb.IconComponent -> + true + end) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule MyAppWeb.|IconComponent do + end + ] + |> rename_with_file( + "MyAppWeb.BadgeComponent", + "lib/my_app_web/components/icon_component.ex", + project + ) + + assert rename_file.new_uri == + subject_uri(project, "lib/my_app_web/components/badge_component.ex") + end + + test "preserves components folder for nested component", %{project: project} do + patch( + Engine.CodeMod.Rename.File, + :phoenix_component_module?, + fn MyAppWeb.Admin.IconComponent -> true end + ) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule MyAppWeb.Admin.|IconComponent do + end + ] + |> rename_with_file( + "MyAppWeb.Admin.BadgeComponent", + "lib/my_app_web/components/admin/icon_component.ex", + project + ) + + assert rename_file.new_uri == + subject_uri(project, "lib/my_app_web/components/admin/badge_component.ex") + end + + test "preserves components folder when Components in module name", %{project: project} do + patch( + Engine.CodeMod.Rename.File, + :phoenix_component_module?, + fn MyAppWeb.Components.Icons -> + true + end + ) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule MyAppWeb.Components.|Icons do + end + ] + |> rename_with_file( + "MyAppWeb.Components.Badges", + "lib/my_app_web/components/icons.ex", + project + ) + + assert rename_file.new_uri == + subject_uri(project, "lib/my_app_web/components/badges.ex") + end + + test "preserves controllers folder", %{project: project} do + patch( + Engine.CodeMod.Rename.File, + :phoenix_controller_module?, + fn MyAppWeb.UserController -> true end + ) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule MyAppWeb.|UserController do + end + ] + |> rename_with_file( + "MyAppWeb.AccountController", + "lib/my_app_web/controllers/user_controller.ex", + project + ) + + assert rename_file.new_uri == + subject_uri(project, "lib/my_app_web/controllers/account_controller.ex") + end + + test "preserves controllers folder for JSON module", %{project: project} do + patch( + Engine.CodeMod.Rename.File, + :phoenix_controller_module?, + fn MyAppWeb.UserController.JSON -> true end + ) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule MyAppWeb.UserController.|JSON do + end + ] + |> rename_with_file( + "MyAppWeb.UserController.API", + "lib/my_app_web/controllers/user_controller/json.ex", + project + ) + + assert rename_file.new_uri == + subject_uri(project, "lib/my_app_web/controllers/user_controller/api.ex") + end + + test "preserves live folder", %{project: project} do + patch(Engine.CodeMod.Rename.File, :phoenix_liveview_module?, fn MyAppWeb.UserLive -> + true + end) + + {:ok, {_applied, rename_file}} = + ~q[ + defmodule MyAppWeb.|UserLive do + end + ] + |> rename_with_file("MyAppWeb.AccountLive", "lib/my_app_web/live/user_live.ex", project) + + assert rename_file.new_uri == subject_uri(project, "lib/my_app_web/live/account_live.ex") + end + end + + # Helpers + defp prepare(code) do with {position, code} <- pop_cursor(code), {:ok, _document, analysis} <- index(code) do @@ -173,6 +854,23 @@ defmodule Engine.CodeMod.RenameTest do end end + defp rename_with_file(code, new_name, path, project) do + uri = subject_uri(project, path) + + with {position, text} <- pop_cursor(code), + {:ok, document} <- open_document(uri, text), + {:ok, entries} <- Engine.Search.Indexer.Source.index_document(document), + :ok <- Search.Store.replace(entries), + {:ok, _document, analysis} <- Document.Store.fetch(uri, :analysis), + {: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) + rename_file = document_changes |> Enum.map(& &1.rename_file) |> List.first() + + {:ok, {applied, rename_file}} + end + end + defp index(code) do project = project() uri = module_uri(project) @@ -190,4 +888,21 @@ defmodule Engine.CodeMod.RenameTest do |> file_path(Path.join("lib", "my_module.ex")) |> Document.Path.ensure_uri() end + + defp subject_uri(project, path) do + project + |> file_path(path) + |> Document.Path.ensure_uri() + end + + defp open_document(uri, content) do + with :ok <- Document.Store.open(uri, content, 0) do + Document.Store.fetch(uri) + end + end + + defp apply_edits(document, text_edits) do + {:ok, edited_document} = Document.apply_content_changes(document, 1, text_edits) + Document.to_string(edited_document) + end end From 10afb296f8969429fad60510473302c611c9b953 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sun, 4 Jan 2026 15:20:16 +0800 Subject: [PATCH 3/6] fix(rename): add progress tracking to trigger reindex after rename Without reindexing after a rename operation, the search index still contains old module names. This causes subsequent renames to fail - e.g., renaming `Dummy.Accounts` to `Dummy.Accounts2` works, but renaming back to `Dummy.Accounts` fails to update `Dummy.Accounts2.User` because the index doesn't know about the new module names. Add Commands.Rename GenServer to track file_changed/file_saved events and trigger reindexing once all rename operations complete. --- apps/engine/lib/engine.ex | 4 + apps/engine/lib/engine/application.ex | 1 + apps/engine/lib/engine/code_mod/rename.ex | 88 +++++++- apps/engine/lib/engine/commands/rename.ex | 206 ++++++++++++++++++ .../lib/engine/commands/rename_supervisor.ex | 58 +++++ apps/engine/lib/engine/progress.ex | 43 ++++ .../test/engine/commands/rename_test.exs | 152 +++++++++++++ apps/expert/lib/expert/engine_api.ex | 10 + apps/expert/lib/expert/state.ex | 5 +- apps/forge/lib/forge/engine_api/messages.ex | 21 ++ 10 files changed, 584 insertions(+), 4 deletions(-) create mode 100644 apps/engine/lib/engine/commands/rename.ex create mode 100644 apps/engine/lib/engine/commands/rename_supervisor.ex create mode 100644 apps/engine/test/engine/commands/rename_test.exs diff --git a/apps/engine/lib/engine.ex b/apps/engine/lib/engine.ex index 8f74c35a..416365a2 100644 --- a/apps/engine/lib/engine.ex +++ b/apps/engine/lib/engine.ex @@ -67,6 +67,10 @@ defmodule Engine do defdelegate rename(analysis, position, new_name, client_name), to: Engine.CodeMod.Rename + defdelegate maybe_update_rename_progress(triggered_message), + to: Engine.Commands.Rename, + as: :update_progress + def list_apps do for {app, _, _} <- :application.loaded_applications(), not Forge.Namespace.Module.prefixed?(app), diff --git a/apps/engine/lib/engine/application.ex b/apps/engine/lib/engine/application.ex index 014aaf60..431fd847 100644 --- a/apps/engine/lib/engine/application.ex +++ b/apps/engine/lib/engine/application.ex @@ -10,6 +10,7 @@ defmodule Engine.Application do [ Engine.Api.Proxy, Engine.Commands.Reindex, + Engine.Commands.RenameSupervisor, Engine.Module.Loader, Engine.Dispatch, Engine.ModuleMappings, diff --git a/apps/engine/lib/engine/code_mod/rename.ex b/apps/engine/lib/engine/code_mod/rename.ex index cee7a359..272125e3 100644 --- a/apps/engine/lib/engine/code_mod/rename.ex +++ b/apps/engine/lib/engine/code_mod/rename.ex @@ -7,11 +7,15 @@ defmodule Engine.CodeMod.Rename do rename execution. """ alias Engine.CodeMod.Rename + alias Engine.Commands + alias Engine.Progress alias Forge.Ast.Analysis alias Forge.Document alias Forge.Document.Position alias Forge.Document.Range + import Forge.EngineApi.Messages + @doc """ Prepares a rename operation at the given position. @@ -31,16 +35,94 @@ defmodule Engine.CodeMod.Rename do Renames the entity at the given position to `new_name`, returning a list of document changes that should be applied. - The `client_name` parameter is currently unused but reserved for future - client-specific behavior (e.g., different progress tracking for VSCode vs Neovim). + The `client_name` parameter is used to determine client-specific behavior + for progress tracking (e.g., VSCode sends different events than Neovim). """ @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, _client_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 = Map.fetch!(@rename_mappings, renamable) results = rename_module.rename(range, new_name, entity) + set_rename_progress(results, client_name) {:ok, results} end end + + defp set_rename_progress(document_changes_list, client_name) do + # Progress tracking is optional - if the infrastructure isn't running + # (e.g., in tests), we just skip it + try do + do_set_rename_progress(document_changes_list, client_name) + rescue + _ -> :ok + catch + :exit, _ -> :ok + end + end + + defp do_set_rename_progress(document_changes_list, client_name) do + uri_to_expected_operation = + uri_to_expected_operation(client_name, document_changes_list) + + {paths_to_delete, paths_to_reindex} = + for %Document.Changes{rename_file: rename_file, document: document} <- document_changes_list do + if rename_file do + {rename_file.old_uri, rename_file.new_uri} + else + {nil, document.uri} + end + end + |> Enum.unzip() + + paths_to_delete = Enum.reject(paths_to_delete, &is_nil/1) + renaming_operation_count = Enum.count(uri_to_expected_operation) + + total_operation_count = + renaming_operation_count + length(paths_to_delete) + length(paths_to_reindex) + + {on_update_progress, on_complete} = + Progress.begin_percent("Renaming", total_operation_count) + + Commands.RenameSupervisor.start_renaming( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) + end + + # VSCode sends both file_changed and file_saved events + defp uri_to_expected_operation(client_name, document_changes_list) + when client_name in ["Visual Studio Code"] do + document_changes_list + |> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} -> + if rename_file do + # when the file is renamed, we won't receive `DidSave` for the old file + [ + {rename_file.old_uri, file_changed(uri: rename_file.old_uri)}, + {rename_file.new_uri, file_saved(uri: rename_file.new_uri)} + ] + else + [{document.uri, file_saved(uri: document.uri)}] + end + end) + |> Map.new() + end + + # Other editors (like Neovim) may only send file_changed events + defp uri_to_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 + [{rename_file.new_uri, file_saved(uri: rename_file.new_uri)}] + 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, file_changed(uri: document.uri)}] + end + end) + |> Map.new() + end end diff --git a/apps/engine/lib/engine/commands/rename.ex b/apps/engine/lib/engine/commands/rename.ex new file mode 100644 index 00000000..98a1dad9 --- /dev/null +++ b/apps/engine/lib/engine/commands/rename.ex @@ -0,0 +1,206 @@ +defmodule Engine.Commands.Rename do + @moduledoc """ + Tracks rename progress and triggers reindexing after rename operations complete. + + This GenServer is started when a rename operation begins. It tracks expected file + operations (file_changed, file_saved) and when all operations are received, it + reindexes the modified files and clears old entries from the search index. + + This is necessary because after a rename, the search index still contains the old + module names. Without reindexing, subsequent renames won't find the new entries. + """ + + alias Engine.Commands.Reindex + alias Engine.Search.Store + alias Forge.EngineApi.Messages + + require Logger + import Messages + + use GenServer + + defmodule State do + @moduledoc false + + @type uri_to_expected_operation :: %{ + Forge.uri() => Messages.file_changed() | Messages.file_saved() + } + + @type t :: %__MODULE__{ + uri_to_expected_operation: uri_to_expected_operation(), + paths_to_reindex: list(Forge.uri()), + paths_to_delete: list(Forge.uri()), + on_update_progress: (integer(), String.t() -> :ok), + on_complete: (-> :ok) + } + + defstruct uri_to_expected_operation: %{}, + paths_to_reindex: [], + paths_to_delete: [], + on_update_progress: nil, + on_complete: nil + + def new( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) do + %__MODULE__{ + uri_to_expected_operation: uri_to_expected_operation, + paths_to_reindex: paths_to_reindex, + paths_to_delete: paths_to_delete, + on_update_progress: on_update_progress, + on_complete: on_complete + } + end + + def update_progress(%__MODULE__{} = state, file_changed(uri: uri)) do + update_progress(state, uri, file_changed(uri: uri)) + end + + def update_progress(%__MODULE__{} = state, file_saved(uri: uri)) do + update_progress(state, uri, file_saved(uri: uri)) + end + + defp update_progress(%__MODULE__{} = state, uri, message) do + new_uri_with_expected_operation = + maybe_pop_expected_operation( + state.uri_to_expected_operation, + uri, + message, + state.on_update_progress + ) + + if Enum.empty?(new_uri_with_expected_operation) do + reindex_all_modified_files(state) + state.on_complete.() + end + + %__MODULE__{state | uri_to_expected_operation: new_uri_with_expected_operation} + end + + def in_progress?(%__MODULE__{} = state) do + state.uri_to_expected_operation != %{} + end + + defp maybe_pop_expected_operation(uri_to_operation, uri, message, on_update_progress) do + case uri_to_operation do + %{^uri => ^message} -> + on_update_progress.(1, "") + Map.delete(uri_to_operation, uri) + + _ -> + uri_to_operation + end + end + + defp reindex_all_modified_files(%__MODULE__{} = state) do + Enum.each(state.paths_to_reindex, fn uri -> + Reindex.uri(uri) + state.on_update_progress.(1, "reindexing") + end) + + Enum.each(state.paths_to_delete, fn uri -> + Store.clear(uri) + state.on_update_progress.(1, "deleting old index") + end) + end + end + + @spec child_spec( + %{Forge.uri() => Messages.file_changed() | Messages.file_saved()}, + list(Forge.uri()), + list(Forge.uri()), + (integer(), String.t() -> :ok), + (-> :ok) + ) :: Supervisor.child_spec() + def child_spec( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) do + %{ + id: __MODULE__, + start: + {__MODULE__, :start_link, + [ + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ]}, + restart: :transient + } + end + + def start_link( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) do + state = + State.new( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) + + GenServer.start_link(__MODULE__, state, name: __MODULE__) + end + + @impl true + def init(state) do + {:ok, state, {:continue, :start_buffering}} + end + + @doc """ + Updates the rename progress with a file operation message. + + This should be called when files are changed or saved during rename. + Returns `:ok` if the message was processed, `{:error, :not_in_rename_progress}` + if no rename is in progress. + """ + @spec update_progress(Messages.file_changed() | Messages.file_saved()) :: + :ok | {:error, :not_in_rename_progress} + def update_progress(message) do + pid = Process.whereis(__MODULE__) + + if pid && Process.alive?(pid) do + GenServer.cast(__MODULE__, {:update_progress, message}) + else + {:error, :not_in_rename_progress} + end + end + + @impl true + def handle_continue(:start_buffering, state) do + Engine.Api.Proxy.start_buffering() + {:noreply, state} + end + + @impl true + def handle_call(:in_progress?, _from, state) do + {:reply, State.in_progress?(state), state} + end + + @impl true + def handle_cast({:update_progress, message}, state) do + new_state = State.update_progress(state, message) + + if State.in_progress?(new_state) do + {:noreply, new_state} + else + Logger.info("Rename process completed.") + {:stop, :normal, new_state} + end + end +end diff --git a/apps/engine/lib/engine/commands/rename_supervisor.ex b/apps/engine/lib/engine/commands/rename_supervisor.ex new file mode 100644 index 00000000..7a72fdcb --- /dev/null +++ b/apps/engine/lib/engine/commands/rename_supervisor.ex @@ -0,0 +1,58 @@ +defmodule Engine.Commands.RenameSupervisor do + @moduledoc """ + DynamicSupervisor for managing rename progress tracking GenServers. + + Each rename operation spawns a transient child that tracks progress + and shuts down when the rename is complete. + """ + + alias Engine.Commands.Rename + + use DynamicSupervisor + + def child_spec(_) do + %{ + id: __MODULE__, + start: {__MODULE__, :start_link, []} + } + end + + def start_link do + DynamicSupervisor.start_link(__MODULE__, [], name: __MODULE__) + end + + @doc """ + Starts a new rename progress tracker. + + ## Parameters + + - `uri_to_expected_operation` - Map of URIs to expected messages (file_changed/file_saved) + - `paths_to_reindex` - List of URIs that need to be reindexed after rename + - `paths_to_delete` - List of URIs whose entries should be deleted from the index + - `on_update_progress` - Callback function receiving (increment, message) + - `on_complete` - Callback function called when rename is complete + """ + def start_renaming( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) do + DynamicSupervisor.start_child( + __MODULE__, + Rename.child_spec( + uri_to_expected_operation, + paths_to_reindex, + paths_to_delete, + on_update_progress, + on_complete + ) + ) + end + + @impl true + def init(_init_arg) do + DynamicSupervisor.init(strategy: :one_for_one) + end +end diff --git a/apps/engine/lib/engine/progress.ex b/apps/engine/lib/engine/progress.ex index 98a5640f..3c4f4a02 100644 --- a/apps/engine/lib/engine/progress.ex +++ b/apps/engine/lib/engine/progress.ex @@ -7,6 +7,14 @@ defmodule Engine.Progress do alias Engine.Dispatch + import Forge.EngineApi.Messages + + @type label :: String.t() + @type message :: String.t() + @type delta :: pos_integer() + @type on_complete_callback :: (-> any()) + @type report_progress_callback :: (delta(), message() -> any()) + @impl true def begin(title, opts \\ []) when is_list(opts) do Dispatch.erpc_call(Expert.Progress, :begin, [title, opts]) @@ -29,4 +37,39 @@ defmodule Engine.Progress do Dispatch.erpc_cast(Expert.Progress, :complete, [token, opts]) :ok end + + @doc """ + Begins a percent-based progress operation. + + Returns a tuple of `{report_progress_callback, on_complete_callback}` that can + be used to report progress and signal completion. + + ## Parameters + + - `label` - The label/title for the progress + - `max` - The maximum value (total number of operations) + + ## Example + + {report_progress, on_complete} = Progress.begin_percent("Renaming", 10) + report_progress.(1, "Processing file...") + on_complete.() + """ + @spec begin_percent(label(), pos_integer()) :: + {report_progress_callback(), on_complete_callback()} + def begin_percent(label, max) do + Engine.broadcast(percent_progress(label: label, max: max, stage: :begin)) + + report_progress = fn delta, message -> + Engine.broadcast( + percent_progress(label: label, message: message, delta: delta, stage: :report) + ) + end + + complete = fn -> + Engine.broadcast(percent_progress(label: label, stage: :complete)) + end + + {report_progress, complete} + end end diff --git a/apps/engine/test/engine/commands/rename_test.exs b/apps/engine/test/engine/commands/rename_test.exs new file mode 100644 index 00000000..1f5d3162 --- /dev/null +++ b/apps/engine/test/engine/commands/rename_test.exs @@ -0,0 +1,152 @@ +defmodule Engine.Commands.RenameTest do + alias Engine.Api.Proxy + alias Engine.Commands.Rename + alias Engine.Commands.RenameSupervisor + alias Engine.Search.Store + + import Forge.EngineApi.Messages + import Forge.Test.EventualAssertions + + use ExUnit.Case + use Patch + + setup do + start_supervised!(RenameSupervisor) + :ok + end + + setup do + pid = self() + + on_report_progress = fn delta, message -> update_progress(pid, delta, message) end + on_complete = fn -> complete_progress(pid) end + + patch(Proxy, :start_buffering, :ok) + %{on_report_progress: on_report_progress, on_complete: on_complete} + end + + test "it should start buffering when a rename is in progress", %{ + on_report_progress: on_report_progress, + on_complete: on_complete + } do + uri = "file://file.ex" + uri_with_expected_operation = %{uri => file_changed(uri: uri)} + paths_to_reindex = [uri] + + {:ok, _pid} = + RenameSupervisor.start_renaming( + uri_with_expected_operation, + paths_to_reindex, + [], + on_report_progress, + on_complete + ) + + assert_called(Proxy.start_buffering()) + end + + test "it should complete and shutdown the process when rename is done", %{ + on_report_progress: on_report_progress, + on_complete: on_complete + } do + uri = "file://file.ex" + paths_to_reindex = [uri] + + {:ok, _pid} = + RenameSupervisor.start_renaming( + %{uri => file_saved(uri: uri)}, + paths_to_reindex, + [], + on_report_progress, + on_complete + ) + + Rename.update_progress(file_saved(uri: uri)) + + assert_receive {:update_progress, 1, ""} + assert_receive :complete_progress + + refute_eventually Process.whereis(Rename) + end + + test "it should still be in progress if there are files yet to be saved", %{ + on_report_progress: on_report_progress, + on_complete: on_complete + } do + uri1 = "file://file1.ex" + old_uri = "file://file2.ex" + new_uri = "file://file3.ex" + + uri_with_expected_operation = %{ + uri1 => file_changed(uri: uri1), + new_uri => file_saved(uri: new_uri) + } + + paths_to_reindex = [uri1, new_uri] + paths_to_delete = [old_uri] + + {:ok, _pid} = + RenameSupervisor.start_renaming( + uri_with_expected_operation, + paths_to_reindex, + paths_to_delete, + on_report_progress, + on_complete + ) + + Rename.update_progress(file_changed(uri: uri1)) + assert_receive {:update_progress, 1, ""} + refute_receive :complete_progress + end + + test "it should reindex new files and delete old entries when all files are modified", %{ + on_report_progress: on_report_progress, + on_complete: on_complete + } do + patch(Store, :clear, :ok) + old_uri = "file://old_file.ex" + new_uri = "file://new_file.ex" + another_uri = "file://file.ex" + + uri_with_expected_operation = %{ + old_uri => file_changed(uri: old_uri), + new_uri => file_saved(uri: new_uri) + } + + paths_to_reindex = [new_uri, another_uri] + paths_to_delete = [old_uri] + + {:ok, _pid} = + RenameSupervisor.start_renaming( + uri_with_expected_operation, + paths_to_reindex, + paths_to_delete, + on_report_progress, + on_complete + ) + + Rename.update_progress(file_changed(uri: old_uri)) + assert_receive {:update_progress, 1, ""} + refute_receive :complete_progress + + Rename.update_progress(file_saved(uri: new_uri)) + assert_receive {:update_progress, 1, ""} + + assert_receive {:update_progress, 1, "reindexing"} + assert_receive {:update_progress, 1, "deleting old index"} + assert_receive :complete_progress + end + + test "it should return :error when updating progress if no rename is in progress" do + assert {:error, :not_in_rename_progress} = + Rename.update_progress(file_changed(uri: "file://file.ex")) + end + + defp update_progress(pid, delta, message) do + send(pid, {:update_progress, delta, message}) + end + + defp complete_progress(pid) do + send(pid, :complete_progress) + end +end diff --git a/apps/expert/lib/expert/engine_api.ex b/apps/expert/lib/expert/engine_api.ex index 0ba54c61..62a9c9e7 100644 --- a/apps/expert/lib/expert/engine_api.ex +++ b/apps/expert/lib/expert/engine_api.ex @@ -166,5 +166,15 @@ defmodule Expert.EngineApi do ]) end + @doc """ + Updates the rename progress with a triggered message. + + This should be called when files are changed or saved during a rename operation. + The message is used to track which file operations have been completed. + """ + def maybe_update_rename_progress(%Project{} = project, message) do + call(project, Engine, :maybe_update_rename_progress, [message]) + end + defdelegate stop(project), to: EngineNode end diff --git a/apps/expert/lib/expert/state.ex b/apps/expert/lib/expert/state.ex index b2edb2af..1973155d 100644 --- a/apps/expert/lib/expert/state.ex +++ b/apps/expert/lib/expert/state.ex @@ -109,6 +109,7 @@ defmodule Expert.State do EngineApi.broadcast(project, updated_message) EngineApi.compile_document(state.configuration.project, updated_source) + EngineApi.maybe_update_rename_progress(project, updated_message) {:ok, state} error -> @@ -155,10 +156,12 @@ defmodule Expert.State do def apply(%__MODULE__{} = state, %GenLSP.Notifications.TextDocumentDidSave{params: params}) do uri = params.text_document.uri + project = state.configuration.project case Document.Store.save(uri) do :ok -> - EngineApi.schedule_compile(state.configuration.project, false) + EngineApi.schedule_compile(project, false) + EngineApi.maybe_update_rename_progress(project, file_saved(uri: uri)) {:ok, state} error -> diff --git a/apps/forge/lib/forge/engine_api/messages.ex b/apps/forge/lib/forge/engine_api/messages.ex index 4c6741dc..b90d302b 100644 --- a/apps/forge/lib/forge/engine_api/messages.ex +++ b/apps/forge/lib/forge/engine_api/messages.ex @@ -43,6 +43,10 @@ defmodule Forge.EngineApi.Messages do defrecord :project_reindexed, project: nil, elapsed_ms: 0, status: :success + defrecord :project_progress, label: nil, stage: :begin + + defrecord :percent_progress, label: nil, message: nil, delta: 0, max: 0, stage: :begin + @type compile_status :: :successful | :error @type name_and_arity :: {atom, non_neg_integer} @type field_list :: Keyword.t() | [atom] @@ -129,4 +133,21 @@ defmodule Forge.EngineApi.Messages do elapsed_ms: non_neg_integer(), status: :success | {:error, term()} ) + + @type progress_stage :: :begin | :report | :complete + + @type project_progress :: + record(:project_progress, + label: String.t(), + stage: progress_stage() + ) + + @type percent_progress :: + record(:percent_progress, + label: String.t(), + message: String.t() | nil, + delta: non_neg_integer(), + max: non_neg_integer(), + stage: progress_stage() + ) end From 2879e003857b62589ba594dd44762f3dd2f91a28 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sun, 4 Jan 2026 15:42:11 +0800 Subject: [PATCH 4/6] fix(reindex): open temporary document when file not in store after rename After a file rename, the new file URI may not be open in Document.Store when reindexing is triggered. This caused "not_open" errors and the index was never updated, breaking references and go-to-definition for renamed modules. Add ensure_open/1 to temporarily open the file from disk if needed. --- apps/engine/lib/engine/code_mod/rename.ex | 2 +- apps/engine/lib/engine/commands/reindex.ex | 17 +++++++++++++++-- apps/engine/lib/engine/commands/rename.ex | 5 +++++ apps/expert/lib/expert/engine_api.ex | 11 ++++++++++- 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/apps/engine/lib/engine/code_mod/rename.ex b/apps/engine/lib/engine/code_mod/rename.ex index 272125e3..def5d6bd 100644 --- a/apps/engine/lib/engine/code_mod/rename.ex +++ b/apps/engine/lib/engine/code_mod/rename.ex @@ -51,7 +51,7 @@ defmodule Engine.CodeMod.Rename do defp set_rename_progress(document_changes_list, client_name) do # Progress tracking is optional - if the infrastructure isn't running - # (e.g., in tests), we just skip it + # (e.g., in tests), we just skip it silently try do do_set_rename_progress(document_changes_list, client_name) rescue diff --git a/apps/engine/lib/engine/commands/reindex.ex b/apps/engine/lib/engine/commands/reindex.ex index 1a9d8a9a..1a0d726d 100644 --- a/apps/engine/lib/engine/commands/reindex.ex +++ b/apps/engine/lib/engine/commands/reindex.ex @@ -54,8 +54,7 @@ defmodule Engine.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_open(uri), {:ok, entries} <- Indexer.Quoted.index_with_cleanup(analysis) do {:ok, document.path, entries} else @@ -64,6 +63,20 @@ defmodule Engine.Commands.Reindex do error end end + + defp ensure_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.Store.open_temporary(uri) do + Document.Store.fetch(uri, :analysis) + end + end + end end @moduledoc """ diff --git a/apps/engine/lib/engine/commands/rename.ex b/apps/engine/lib/engine/commands/rename.ex index 98a1dad9..e8149624 100644 --- a/apps/engine/lib/engine/commands/rename.ex +++ b/apps/engine/lib/engine/commands/rename.ex @@ -145,6 +145,11 @@ defmodule Engine.Commands.Rename do on_update_progress, on_complete ) do + Logger.info( + "Starting rename tracking: #{map_size(uri_to_expected_operation)} operations, " <> + "#{length(paths_to_reindex)} to reindex, #{length(paths_to_delete)} to delete" + ) + state = State.new( uri_to_expected_operation, diff --git a/apps/expert/lib/expert/engine_api.ex b/apps/expert/lib/expert/engine_api.ex index 62a9c9e7..43f4f31f 100644 --- a/apps/expert/lib/expert/engine_api.ex +++ b/apps/expert/lib/expert/engine_api.ex @@ -171,9 +171,18 @@ defmodule Expert.EngineApi do This should be called when files are changed or saved during a rename operation. The message is used to track which file operations have been completed. + + This function is intentionally fire-and-forget - failures don't affect + the main document change/save flow. """ def maybe_update_rename_progress(%Project{} = project, message) do - call(project, Engine, :maybe_update_rename_progress, [message]) + try do + call(project, Engine, :maybe_update_rename_progress, [message]) + rescue + _ -> {:error, :not_in_rename_progress} + catch + :exit, _ -> {:error, :not_in_rename_progress} + end end defdelegate stop(project), to: EngineNode From 8d72473bfc5c83cef8cdeadb4ab06de4987e5c17 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sun, 11 Jan 2026 10:07:48 +0800 Subject: [PATCH 5/6] Add integration tests for editor-specific rename reindex behavior --- .../expert/state/rename_progress_test.exs | 213 ++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 apps/expert/test/expert/state/rename_progress_test.exs diff --git a/apps/expert/test/expert/state/rename_progress_test.exs b/apps/expert/test/expert/state/rename_progress_test.exs new file mode 100644 index 00000000..967981c0 --- /dev/null +++ b/apps/expert/test/expert/state/rename_progress_test.exs @@ -0,0 +1,213 @@ +defmodule Expert.State.RenameProgressTest do + @moduledoc """ + Integration tests for rename progress tracking with different editors. + + Different editors send different events after rename operations: + - VSCode: sends both file_changed and file_saved events for renamed files + - Neovim: sends only file_changed for non-renamed files, file_saved for renamed files + + These tests verify the full rename flow from State → EngineApi → Engine. + """ + + alias Expert.Configuration + alias Expert.EngineApi + alias Expert.State + alias Engine.Commands.Rename + alias Engine.Commands.RenameSupervisor + alias Forge.Document + + import Forge.EngineApi.Messages + import Forge.Test.Fixtures + + use ExUnit.Case, async: false + use Patch + + setup do + start_supervised!(Expert.Application.document_store_child_spec()) + start_supervised!(RenameSupervisor) + + patch(Engine.Api.Proxy, :start_buffering, :ok) + patch(Engine.Commands.Reindex, :uri, fn _uri -> :ok end) + patch(Engine.Search.Store, :clear, fn _uri -> :ok end) + + if pid = Process.whereis(Rename) do + Process.exit(pid, :kill) + Process.sleep(10) + end + + :ok + end + + describe "VSCode editor - non-file-rename (edits only)" do + test "reindex triggers after file_saved (VSCode expects save after edit)" do + uri = "file:///test/lib/foo.ex" + editor = vscode_editor() + open_document(uri, "defmodule Foo do\nend") + expect_events_before_reindex(%{uri => file_saved(uri: uri)}, reindex: [uri]) + + simulate_did_change(editor, uri) + refute_reindex_triggered() + + simulate_did_save(editor, uri) + assert_reindex_triggered(reindex: [uri], delete: []) + end + end + + describe "Neovim editor - non-file-rename (edits only)" do + test "reindex triggers after file_changed only (Neovim doesn't auto-save)" do + uri = "file:///test/lib/foo.ex" + editor = neovim_editor() + open_document(uri, "defmodule Foo do\nend") + expect_events_before_reindex(%{uri => file_changed(uri: uri)}, reindex: [uri]) + + simulate_did_change(editor, uri) + + assert_reindex_triggered(reindex: [uri], delete: []) + end + end + + describe "VSCode editor - file rename" do + test "reindex triggers after receiving file_changed for old + file_saved for new" do + old_uri = "file:///test/lib/old_module.ex" + new_uri = "file:///test/lib/new_module.ex" + editor = vscode_editor() + + open_document(old_uri, "defmodule OldModule do\nend") + open_document(new_uri, "defmodule NewModule do\nend") + + expect_events_before_reindex( + %{old_uri => file_changed(uri: old_uri), new_uri => file_saved(uri: new_uri)}, + reindex: [new_uri], + delete: [old_uri] + ) + + simulate_did_change(editor, old_uri) + refute_reindex_triggered() + + simulate_did_save(editor, new_uri) + assert_reindex_triggered(reindex: [new_uri], delete: [old_uri]) + end + end + + describe "Neovim editor - file rename" do + test "reindex triggers after file_saved for new file only" do + old_uri = "file:///test/lib/old_neovim.ex" + new_uri = "file:///test/lib/new_neovim.ex" + editor = neovim_editor() + + open_document(new_uri, "defmodule NewNeovim do\nend") + + expect_events_before_reindex( + %{new_uri => file_saved(uri: new_uri)}, + reindex: [new_uri], + delete: [old_uri] + ) + + simulate_did_save(editor, new_uri) + + assert_reindex_triggered(reindex: [new_uri], delete: [old_uri]) + end + end + + # ============================================================================ + # Test DSL - Editor simulation + # ============================================================================ + + defp vscode_editor do + build_editor("Visual Studio Code") + end + + defp neovim_editor do + build_editor("Neovim") + end + + defp build_editor(client_name) do + project = project() + config = Configuration.new(project: project, client_name: client_name) + state = %State{configuration: config, initialized?: true} + + patch(EngineApi, :broadcast, fn ^project, _msg -> :ok end) + patch(EngineApi, :compile_document, fn ^project, _doc -> :ok end) + patch(EngineApi, :schedule_compile, fn ^project, _ -> :ok end) + + patch(EngineApi, :maybe_update_rename_progress, fn ^project, msg -> + Rename.update_progress(msg) + end) + + %{state: state, version: 1} + end + + defp open_document(uri, content) do + :ok = Document.Store.open(uri, content, 1) + end + + defp simulate_did_change(editor, uri) do + new_version = editor.version + 1 + notification = build_did_change(uri, new_version) + {:ok, new_state} = State.apply(editor.state, notification) + %{editor | state: new_state, version: new_version} + end + + defp simulate_did_save(editor, uri) do + notification = build_did_save(uri) + {:ok, new_state} = State.apply(editor.state, notification) + %{editor | state: new_state} + end + + # ============================================================================ + # Test DSL - Expectations + # ============================================================================ + + defp expect_events_before_reindex(uri_to_expected, opts) do + paths_to_reindex = Keyword.get(opts, :reindex, []) + paths_to_delete = Keyword.get(opts, :delete, []) + test_pid = self() + + on_complete = fn -> + send(test_pid, {:rename_complete, paths_to_reindex, paths_to_delete}) + end + + {:ok, _} = + RenameSupervisor.start_renaming( + uri_to_expected, + paths_to_reindex, + paths_to_delete, + fn _delta, _msg -> :ok end, + on_complete + ) + end + + defp assert_reindex_triggered(opts) do + reindex = Keyword.fetch!(opts, :reindex) + delete = Keyword.fetch!(opts, :delete) + assert_receive {:rename_complete, ^reindex, ^delete} + end + + defp refute_reindex_triggered do + refute_receive {:rename_complete, _, _}, 50 + end + + # ============================================================================ + # LSP notification builders + # ============================================================================ + + defp build_did_change(uri, version) do + %GenLSP.Notifications.TextDocumentDidChange{ + params: %GenLSP.Structures.DidChangeTextDocumentParams{ + text_document: %GenLSP.Structures.VersionedTextDocumentIdentifier{ + uri: uri, + version: version + }, + content_changes: [%{text: "defmodule Renamed do\nend"}] + } + } + end + + defp build_did_save(uri) do + %GenLSP.Notifications.TextDocumentDidSave{ + params: %GenLSP.Structures.DidSaveTextDocumentParams{ + text_document: %GenLSP.Structures.TextDocumentIdentifier{uri: uri} + } + } + end +end From 189e3aa14aeca380907f9c21bbbdc3e020351248 Mon Sep 17 00:00:00 2001 From: Scott Ming Date: Sun, 11 Jan 2026 20:41:09 +0800 Subject: [PATCH 6/6] fix(didSave): handle race condition when file is closed before save during batch rename During batch renames, didClose and didSave notifications can arrive nearly simultaneously. If didClose is processed first, the file is removed from Document.Store, causing didSave to fail with :not_open error. This is a follow-up fix to 2879e00 which only addressed the reindex flow. Now didSave gracefully handles :not_open by still updating rename progress tracking, ensuring the rename operation completes successfully. --- apps/expert/lib/expert/state.ex | 8 ++++++++ .../test/expert/state/rename_progress_test.exs | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/apps/expert/lib/expert/state.ex b/apps/expert/lib/expert/state.ex index 1973155d..1814b75c 100644 --- a/apps/expert/lib/expert/state.ex +++ b/apps/expert/lib/expert/state.ex @@ -164,6 +164,14 @@ defmodule Expert.State do EngineApi.maybe_update_rename_progress(project, file_saved(uri: uri)) {:ok, state} + {:error, :not_open} -> + # File was closed before save notification was processed. + # This can happen during batch renames when didClose and didSave + # arrive nearly simultaneously. Still update rename progress tracking. + Logger.debug("Save received for already-closed file: #{uri}") + EngineApi.maybe_update_rename_progress(project, file_saved(uri: uri)) + {:ok, state} + error -> Logger.error("Save failed for uri #{uri} error was #{inspect(error)}") error diff --git a/apps/expert/test/expert/state/rename_progress_test.exs b/apps/expert/test/expert/state/rename_progress_test.exs index 967981c0..8b091e2b 100644 --- a/apps/expert/test/expert/state/rename_progress_test.exs +++ b/apps/expert/test/expert/state/rename_progress_test.exs @@ -53,6 +53,24 @@ defmodule Expert.State.RenameProgressTest do end end + describe "race condition - didClose before didSave" do + test "rename progress is updated even when file is already closed" do + # This can happen during batch renames when didClose and didSave + # arrive nearly simultaneously and didClose is processed first + uri = "file:///test/lib/race.ex" + editor = vscode_editor() + open_document(uri, "defmodule Race do\nend") + expect_events_before_reindex(%{uri => file_saved(uri: uri)}, reindex: [uri]) + + # Simulate didClose being processed before didSave + :ok = Document.Store.close(uri) + + # didSave should still succeed and update rename progress + simulate_did_save(editor, uri) + assert_reindex_triggered(reindex: [uri], delete: []) + end + end + describe "Neovim editor - non-file-rename (edits only)" do test "reindex triggers after file_changed only (Neovim doesn't auto-save)" do uri = "file:///test/lib/foo.ex"