Skip to content

Commit

Permalink
Rename file when renaming module (lexical-lsp#651)
Browse files Browse the repository at this point in the history
* Rename the file while renaming the module.

It first determines whether the module meets certain hard constraints, such as having no parent, then performs some conventional checks. If both are satisfied, it proceeds to return the file renaming.

* Maybe insert special folder for PhoenixWeb's module

* Apply some code review changes for using `Document.Changes`

Use `Document.Changes` instead of `CodeMod.Rename.DocumentChanges`

Make `Document.Changes.RenameFile` a struct and use `root_module?`
instead of `has_silibings?` and `has_parent?`

* Use `rename progress marker` instead of suspending build

* Use `file_changed` instead of `file_compile_requested` for reindexing

* `uri_with_operations_counts` by client name

* Apply some code review suggestions for `rename/file`

* Apply suggestions from code review

Co-authored-by: Steve Cohen <[email protected]>

* Do not need to care about the `DidClose` message

* Apply some code review suggestions

1. Move the `if-else` logic to the `Commands.Rename` module
2. Add `file_opened` message type
3. modify the format error message

* Shouldn't returns `RenameFile` if the file name not changed

* Change `uri_with_operation_counts` to `uri_with_expected_operation`

and special some message type for emacs

---------

Co-authored-by: Steve Cohen <[email protected]>

Add `WorkDoneProgress` for the rename expected_operation

Apply suggestions from code review

Co-authored-by: Steve Cohen <[email protected]>

Update apps/remote_control/lib/lexical/remote_control/code_mod/rename/module.ex

Co-authored-by: Steve Cohen <[email protected]>

Update apps/remote_control/lib/lexical/remote_control/commands/rename.ex

Co-authored-by: Zach Allaun <[email protected]>

Use record message instead of Protocol structs

Replace the asynchronous subscription method of `dispatch` with synchronous `updates`.

In the previous commit, we tried the method of registering and subscribing to messages via `dispatch`. I found that this method has significant issues, as it causes the message triggering compilation, the modification of rename status, and the judgment of progress to be out of sync, leading to rename failures and crashes.

Apply another code review suggestions
  • Loading branch information
scottming committed Jul 12, 2024
1 parent 5bc85ac commit 8206357
Show file tree
Hide file tree
Showing 20 changed files with 856 additions and 105 deletions.
36 changes: 36 additions & 0 deletions apps/remote_control/lib/lexical/remote_control/api.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ defmodule Lexical.RemoteControl.Api do
alias Lexical.Project
alias Lexical.RemoteControl
alias Lexical.RemoteControl.CodeIntelligence
alias Lexical.RemoteControl.CodeMod

require Logger

Expand All @@ -18,6 +19,18 @@ defmodule Lexical.RemoteControl.Api do
RemoteControl.call(project, RemoteControl, :compile_document, [document])
end

def maybe_schedule_compile(project, triggered_message) do
RemoteControl.call(project, Build, :maybe_schedule_compile, [triggered_message])
end

def maybe_compile_document(project, document, updated_message) do
RemoteControl.call(project, Build, :maybe_compile_document, [
project,
document,
updated_message
])
end

def expand_alias(
%Project{} = project,
segments_or_module,
Expand Down Expand Up @@ -54,6 +67,29 @@ defmodule Lexical.RemoteControl.Api do
])
end

def prepare_rename(
%Project{} = project,
%Analysis{} = analysis,
%Position{} = position
) do
RemoteControl.call(project, CodeMod.Rename, :prepare, [analysis, position])
end

def rename(
%Project{} = project,
%Analysis{} = analysis,
%Position{} = position,
new_name,
client_name
) do
RemoteControl.call(project, CodeMod.Rename, :rename, [
analysis,
position,
new_name,
client_name
])
end

def complete(%Project{} = project, %Env{} = env) do
Logger.info("Completion for #{inspect(env.position)}")
RemoteControl.call(project, RemoteControl, :complete, [env])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ defmodule Lexical.RemoteControl.Api.Messages do
diagnostics: [],
elapsed_ms: 0

defrecord :file_saved, uri: nil

defrecord :file_deleted, project: nil, uri: nil

defrecord :module_updated, file: nil, name: nil, functions: [], macros: [], struct: nil
Expand Down Expand Up @@ -95,6 +97,8 @@ defmodule Lexical.RemoteControl.Api.Messages do
elapsed_ms: non_neg_integer
)

@type file_saved :: record(:file_saved, uri: Lexical.uri())

@type module_updated ::
record(:module_updated,
name: module(),
Expand Down
17 changes: 17 additions & 0 deletions apps/remote_control/lib/lexical/remote_control/build.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ defmodule Lexical.RemoteControl.Build do
alias Lexical.RemoteControl
alias Lexical.RemoteControl.Build.Document.Compilers.HEEx
alias Lexical.RemoteControl.Build.State
alias Lexical.RemoteControl.Commands.Rename
alias Lexical.VM.Versions

require Logger
Expand Down Expand Up @@ -35,6 +36,22 @@ defmodule Lexical.RemoteControl.Build do
:ok
end

def maybe_schedule_compile(message) do
if Rename.in_progress?() do
Rename.update_progress(message)
else
GenServer.cast(__MODULE__, {:compile, false})
end
end

def maybe_compile_document(%Project{} = project, %Document{} = document, message) do
if Rename.in_progress?() do
Rename.update_progress(message)
else
compile_document(project, document)
end
end

# this is for testing
def force_compile_document(%Document{} = document) do
with false <- Path.absname(document.path) == "mix.exs",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,22 @@ defmodule Lexical.RemoteControl.CodeIntelligence.Entity do
)
end

@spec phoenix_controller_module?(module()) :: boolean()
def phoenix_controller_module?(module) do
function_exists?(module, :call, 2) and function_exists?(module, :action, 2)
end

@spec phoenix_liveview_module?(module()) :: boolean()
def phoenix_liveview_module?(module) do
function_exists?(module, :mount, 3) and function_exists?(module, :render, 1)
end

@spec phoenix_component_module?(module()) :: boolean()
def phoenix_component_module?(module) do
function_exists?(module, :__components__, 0) and
function_exists?(module, :__phoenix_component_verify__, 1)
end

defp check_commented(%Analysis{} = analysis, %Position{} = position) do
if Analysis.commented?(analysis, position) do
:error
Expand Down Expand Up @@ -294,14 +310,6 @@ defmodule Lexical.RemoteControl.CodeIntelligence.Entity do
end
end

defp phoenix_controller_module?(module) do
function_exists?(module, :call, 2) and function_exists?(module, :action, 2)
end

defp phoenix_liveview_module?(module) do
function_exists?(module, :mount, 3) and function_exists?(module, :render, 1)
end

# Take only the segments at and before the cursor, e.g.
# Foo|.Bar.Baz -> Foo
# Foo.|Bar.Baz -> Foo.Bar
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ defmodule Lexical.RemoteControl.CodeMod.Format do
alias Lexical.RemoteControl
alias Lexical.RemoteControl.Build
alias Lexical.RemoteControl.CodeMod.Diff
alias Lexical.RemoteControl.Commands.Rename

require Logger

Expand Down Expand Up @@ -99,13 +100,22 @@ defmodule Lexical.RemoteControl.CodeMod.Format do
def edits(%Document{} = document) do
project = RemoteControl.get_project()

with :ok <- Build.compile_document(project, document),
with :ok <- ensure_not_renaming(),
:ok <- Build.compile_document(project, document),
{:ok, formatted} <- do_format(project, document) do
edits = Diff.diff(document, formatted)
{:ok, Changes.new(document, edits)}
end
end

defp ensure_not_renaming do
if Rename.in_progress?() do
{:error, :rename_in_progress}
else
:ok
end
end

defp do_format(%Project{} = project, %Document{} = document) do
project_path = Project.project_path(project)

Expand Down
65 changes: 58 additions & 7 deletions apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex
Original file line number Diff line number Diff line change
@@ -1,22 +1,73 @@
defmodule Lexical.RemoteControl.CodeMod.Rename do
alias Lexical.Ast.Analysis
alias Lexical.Document.Edit
alias Lexical.Document
alias Lexical.Document.Position
alias Lexical.Document.Range
alias Lexical.RemoteControl.Commands
alias Lexical.RemoteControl.Progress

import Lexical.RemoteControl.Api.Messages

alias __MODULE__

@spec prepare(Analysis.t(), Position.t()) ::
{:ok, {atom(), String.t()}, Range.t()} | {:error, term()}
defdelegate prepare(analysis, position), to: Rename.Prepare

@rename_mapping %{module: Rename.Module}
@rename_mappings %{module: Rename.Module}

@spec rename(Analysis.t(), Position.t(), String.t()) ::
{:ok, %{Lexical.uri() => [Edit.t()]}} | {:error, term()}
def rename(%Analysis{} = analysis, %Position{} = position, new_name) do
@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 = @rename_mapping[renamable]
{:ok, rename_module.rename(range, new_name, entity)}
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
uri_with_expected_operation =
uri_with_expected_operation(client_name, document_changes_list)

progress_notification_functions =
Progress.begin_percent("Renaming", Enum.count(uri_with_expected_operation))

Commands.Rename.set_rename_progress(
uri_with_expected_operation,
progress_notification_functions
)
end

defp uri_with_expected_operation(client_name, document_changes_list)
when client_name in ["Visual Studio Code", "emacs"] do
document_changes_list
|> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} ->
if rename_file do
# 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

defp uri_with_expected_operation(_, document_changes_list) do
document_changes_list
|> Enum.flat_map(fn %Document.Changes{document: document, rename_file: rename_file} ->
if rename_file do
[{document.uri, file_saved(uri: document.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
137 changes: 137 additions & 0 deletions apps/remote_control/lib/lexical/remote_control/code_mod/rename/file.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
defmodule Lexical.RemoteControl.CodeMod.Rename.File do
alias Lexical.Ast
alias Lexical.Document
alias Lexical.ProcessCache
alias Lexical.Project
alias Lexical.RemoteControl
alias Lexical.RemoteControl.CodeIntelligence.Entity
alias Lexical.RemoteControl.Search.Indexer
alias Lexical.RemoteControl.Search.Indexer.Entry

@spec maybe_rename(Document.t(), Entry.t(), String.t()) :: Document.Changes.rename_file()
def maybe_rename(%Document{} = document, %Entry{} = entry, new_suffix) do
if root_module?(entry, document) do
rename_file(document, entry, new_suffix)
end
end

defp root_module?(entry, 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
[root_module] ->
root_module.subject == entry.subject and root_module.block_range == entry.block_range

_ ->
false
end
end

defp rename_file(document, 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(RemoteControl.get_project())
end

defp fetch_new_name(document, entry, new_suffix) do
text_edits = [Document.Edit.new(new_suffix, entry.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.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
Entity.phoenix_controller_module?(subject) ->
"controllers"

Entity.phoenix_liveview_module?(subject) ->
"live"

Entity.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
end
Loading

0 comments on commit 8206357

Please sign in to comment.