lexical-lsp / lexical

Lexical is a next-generation elixir language server
781 stars 77 forks source link

Add Support for Renaming module #636

Open scottming opened 4 months ago

scottming commented 4 months ago

This PR is primarily copied from #374, but compared to the previous implementation, we have made two changes:

  1. We constrained the scope of renaming, meaning that renaming a module is only allowed at its definition. This simplification reduces a lot of computations.
  2. Based on the first point, our preparation no longer returns just a single local module, but instead, the entire module. This means that module renaming has become more powerful. We can not only rename a single local module but also simplify a module. For example, renaming TopLevel.Parent.Child to TopLevel.Child, or renaming some middle parts, like TopLevel.Parent.Child to TopLevel.Renamed.Child.

CleanShot 2024-03-17 at 13 55 15@2x

I personally really like this change, especially the second point, which makes module renaming much more practical.

scohen commented 3 months ago

I played around with this a little bit, here are my observations:

I did a big rename, renaming Lexical.Ast to Lexical.Aste for no particular reason. This absolutely crushed emacs, mostly due to emacs having to open a bunch of files, and doing the edits. By the time it came to close them, the expiration of the temporary open had elapsed, and I think this crashed the server. Emacs is famously bad at file I/O, so I don't think there's a ton to do other than open the documents permanently rather than temporarily. Something would need to track these opened files and close them when the operation was done.

One troubling bug that I noticed is that it renamed usages of Lexical.Ast.Detection in ast/env.ex to Detectione, which is strange.

VSCode fared much better, the rename was more or less instantaneous, and the rename completed successfully with no compile errors.However, the server crashed with the following

** (MatchError) no match of right hand side value: ["{\"jsonrpc\"", "\"2.0\",\"method\"", "\"textDocument/didOpen\",\"params\"", "{\"textDocument\"", "{\"uri\"", "\"file", "///Users/steve/Projects/lexical/apps/remote_control/lib/lexical/remote_control/search/indexer/extractors/struct_reference.ex\",\"languageId\"", "\"elixir\",\"version\"", "1,\"text\"", "\"defmodule Lexical.RemoteControl.Search.Indexer.Extractors.StructReference do\\n  alias Lexical.Ast\\n  alias Lexical.RemoteControl.Analyzer\\n  alias Lexical.RemoteControl.Search.Indexer.Entry\\n  alias Lexical.RemoteControl.Search.Indexer.Source.Reducer\\n  alias Lexical.RemoteControl.Search.Subject\\n\\n  require Logger\\n\\n  @struct_fn_names [", "struct, ", "struct!]\\n\\n  # Handles usages via an alias, e.g. x = %MyStruct{...} or %__MODULE__{...}\\n  def extract(\\n        {", "%, _, [struct_alias, {", "%{}, _, _struct_args}]} = reference,\\n        %Reducer{} = reducer\\n      ) do\\n    case expand_alias(struct_alias, reducer) do\\n      {", "ok, struct_module} ->\\n        {", "ok, entry(reducer, struct_module, reference)}\\n\\n      _ ->\\n        ", "ignored\\n    end\\n  end\\n\\n  # Call to Kernel.struct with a fully qualified module e.g. Kernel.struct(MyStruct, ...)\\n  def extract(\\n        {{", "., _, [kernel_alias, struct_fn_name]}, _, [struct_alias | _]} = reference,\\n        %Reducer{} = reducer\\n      )\\n      when struct_fn_name in @struct_fn_names do\\n    with {", "ok, Kernel} <- expand_alias(kernel_alias, reducer),\\n         {", "ok, struct_module} <- expand_alias(struct_alias, reducer) do\\n      {", "ok, entry(reducer, struct_module, reference)}\\n    else\\n      _ ->\\n        ", "ignored\\n    end\\n  end\\n\\n  # handles calls to Kernel.struct e.g. struct(MyModule) or struct(MyModule, foo", " 3)\\n  def extract({struct_fn_name, _, [struct_alias | _] = args} = reference, %Reducer{} = reducer)\\n      when struct_fn_name in @struct_fn_names do\\n    reducer_position = Reducer.position(reducer)\\n    imports = Analyzer.imports_at(reducer.analysis, reducer_position)\\n    arity = length(args)\\n\\n    with true <- Enum.member?(imports, {Kernel, struct_fn_name, arity}),\\n         {", "ok, struct_module} <- expand_alias(struct_alias, reducer) do\\n      {", "ok, entry(reducer, struct_module, reference)}\\n    else\\n      _ ->\\n        ", "ignored\\n    end\\n  end\\n\\n  def extract(_, _) do\\n    ", "ignored\\n  end\\n\\n  defp entry(%Reducer{} = reducer, struct_module, reference) do\\n    document = reducer.analysis.document\\n    block = Reducer.current_block(reducer)\\n    subject = Subject.module(struct_module)\\n\\n    Entry.reference(\\n      document.path,\\n      block,\\n      subject,\\n      ", "struct,\\n      Ast.Range.fetch!(reference, document),\\n      Application.get_application(struct_module)\\n    )\\n  end\\n\\n  defp expand_alias({", "__aliases__, _, struct_alias}, %Reducer{} = reducer) do\\n    Analyzer.expand_alias(struct_alias, reducer.analysis, Reducer.position(reducer))\\n  end\\n\\n  defp expand_alias({", "__MODULE__, _, _}, %Reducer{} = reducer) do\\n    Analyzer.current_module(reducer.analysis, Reducer.position(reducer))\\n  end\\n\\n  defp expand_alias(alias, %Reducer{} = reducer) do\\n    {line, column} = reducer.position\\n\\n    Logger.error(\\n      \\\"Could not expand alias", " \#{inspect(alias)} at \#{reducer.analysis.document.path} \#{line}", "\#{column}\\\"\\n    )\\n\\n    ", "error\\n  end\\nend\\n\"}}}Content-Length", " 2625\n"]
    (lx_server 0.5.0) lib/lexical/server/transport/std_io.ex:104: LXical.Server.Transport.StdIO.parse_header/1
    (elixir 1.14.5) lib/enum.ex:1658: Enum."-map/2-lists^map/1-0-"/2
    (lx_server 0.5.0) lib/lexical/server/transport/std_io.ex:60: LXical.Server.Transport.StdIO.loop/3
    (stdlib 4.3.1.2) proc_lib.erl:240: :proc_lib.init_p_do_apply/3
Initial Call: LXical.Server.Transport.StdIO.init/1
Ancestors: [LXical.Server.Supervisor, #PID<0.143.0>]
Message Queue Length: 0
scohen commented 3 months ago

I think i've found the issue that caused the hard crash, I'll write up a PR for that soon.

I've tried this a couple times, renaming Lexical.Ast to Lexical.AST which I think is a pretty standard operation, and it fails in both emacs and vscode. Once you get that working, tag for a re-review.

Of course, you'll have to wait until after my other fix.

scottming commented 3 months ago

I've tried this a couple times, renaming Lexical.Ast to Lexical.AST which I think is a pretty standard operation, and it fails in both emacs and vscode. Once you get that working, tag for a re-review.

@scohen I tried merging 663, and then everything seems to be working well now.

scohen commented 3 months ago

This still failed in vscode. I got a bunch of the following errors, which caused a supervisor to shut down:

09:06:22.383 [error] Failed to handle Elixir.LXical.Protocol.Notifications.DidChange, {{:error, :not_open}, %LXical.Server.State{configuration: %LXical.Server.Configuration{project: %LXical.Project{root_uri: "file:///Users/steve/Projects/lexical", mix_exs_uri: "file:///Users/steve/Projects/lexical/mix.exs", mix_project?: true, mix_env: nil, mix_target: nil, env_variables: %{}, project_module: nil, entropy: 49917, project_config: []}, support: %LXical.Server.Configuration.Support{code_action_dynamic_registration: true, hierarchical_symbols: true, snippet: true, deprecated: true, tags: #Protocol.Types.Completion.ClientCapabilities.TagSupport<[value_set: [:deprecated]]>, signature_help: #Protocol.Types.SignatureHelp.ClientCapabilities<[context_support: true, dynamic_registration: true, signature_information: #Protocol.Types.SignatureHelp.ClientCapabilities.SignatureInformation<[active_parameter_support: true, documentation_format: [:markdown, :plain_text], parameter_information: #Protocol.Types.SignatureHelp.ClientCapabilities.ParameterInformation<[label_offset_support: true]>]>]>, work_done_progress: true}, client_name: "Visual Studio Code", additional_watched_extensions: nil, dialyzer_enabled?: false}, initialized?: true, shutdown_received?: false, in_flight_requests: %{}}}
09:06:22.383 [error] Could not handle message LXical.Protocol.Notifications.DidChange :ok
09:06:22.395 [error] Failed to handle Elixir.LXical.Protocol.Notifications.DidChange, {{:error, :not_open}, %LXical.Server.State{configuration: %LXical.Server.Configuration{project: %LXical.Project{root_uri: "file:///Users/steve/Projects/lexical", mix_exs_uri: "file:///Users/steve/Projects/lexical/mix.exs", mix_project?: true, mix_env: nil, mix_target: nil, env_variables: %{}, project_module: nil, entropy: 49917, project_config: []}, support: %LXical.Server.Configuration.Support{code_action_dynamic_registration: true, hierarchical_symbols: true, snippet: true, deprecated: true, tags: #Protocol.Types.Completion.ClientCapabilities.TagSupport<[value_set: [:deprecated]]>, signature_help: #Protocol.Types.SignatureHelp.ClientCapabilities<[context_support: true, dynamic_registration: true, signature_information: #Protocol.Types.SignatureHelp.ClientCapabilities.SignatureInformation<[active_parameter_support: true, documentation_format: [:markdown, :plain_text], parameter_information: #Protocol.Types.SignatureHelp.ClientCapabilities.ParameterInformation<[label_offset_support: true]>]>]>, work_done_progress: true}, client_name: "Visual Studio Code", additional_watched_extensions: nil, dialyzer_enabled?: false}, initialized?: true, shutdown_received?: false, in_flight_requests: %{}}}
09:06:22.395 [error] Could not handle message LXical.Protocol.Notifications.DidChange :ok
09:06:22.417 [error] Failed to handle Elixir.LXical.Protocol.Notifications.DidChange, {{:error, :not_open}, %LXical.Server.State{configuration: %LXical.Server.Configuration{project: %LXical.Project{root_uri: "file:///Users/steve/Projects/lexical", mix_exs_uri: "file:///Users/steve/Projects/lexical/mix.exs", mix_project?: true, mix_env: nil, mix_target: nil, env_variables: %{}, project_module: nil, entropy: 49917, project_config: []}, support: %LXical.Server.Configuration.Support{code_action_dynamic_registration: true, hierarchical_symbols: true, snippet: true, deprecated: true, tags: #Protocol.Types.Completion.ClientCapabilities.TagSupport<[value_set: [:deprecated]]>, signature_help: #Protocol.Types.SignatureHelp.ClientCapabilities<[context_support: true, dynamic_registration: true, signature_information: #Protocol.Types.SignatureHelp.ClientCapabilities.SignatureInformation<[active_parameter_support: true, documentation_format: [:markdown, :plain_text], parameter_information: #Protocol.Types.SignatureHelp.ClientCapabilities.ParameterInformation<[label_offset_support: true]>]>]>, work_done_progress: true}, client_name: "Visual Studio Code", additional_watched_extensions: nil, dialyzer_enabled?: false}, initialized?: true, shutdown_received?: false, in_flight_requests: %{}}}

Seems like we get a DidChange before we get a DidOpen, or we have an open_temporary that expired

FYI, this was a rename of Lexical.Ast -> Lexical.AST. I have not gotten that to succeed on either emacs or vscode. I would suggest trying it in vscode until it works.

Note: I had format-on-save turned on in both editors.

scottming commented 3 months ago

Seems like we get a DidChange before we get a DidOpen, or we have an open_temporary that expired

Yes, The reason this issue arises is that we don't open a document for the exact entry, and vscode first sends DidChange, then DidSave, and finally DidClose. It's possible that some DidChange events are sent before DidOpen. We solved this issue in the rename_file PR(#651 ) when converting Document.Changes.

Can you try this operation on that branch?

scottming commented 3 months ago

@scohen However, with such changes involving hundreds of files, vscode sends hundreds of DidSave messages. Do we need to identify this situation and only execute the first mix compile while ignoring the others? Currently, this makes my m3 max scream for a few minutes (due to CPU overload).

scohen commented 3 months ago

Can you try this operation on that branch?

Will do.

However, with such changes involving hundreds of files, vscode sends hundreds of DidSave messages

Yes, i saw this as well. We should definitely disable the compilers during the rename operation, and resume it when it's done.

scohen commented 3 months ago

@scottming I think you can use Build.with_lock to disable builds during the rename. You might also need to do something similar for formatting (it doesn't presently have a lock)

scottming commented 3 months ago

@scottming I think you can use Build.with_lock to disable builds during the rename. You might also need to do something similar for formatting (it doesn't presently have a lock)

Can you explain exactly how to do it?

I found that simply using Build.with_lock doesn't work. It might be because these build messages are received after the rename operation in the LS backend (what I mean is, we instantly return the rename result to the client, but the VScode client takes some time to complete these operations and sends a lot of DidSave requests during this period). So, simply locking the rename operation is likely ineffective.

diff --git a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex
index 8a6adade..27f2bc60 100644
--- a/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex
+++ b/apps/remote_control/lib/lexical/remote_control/code_mod/rename.ex
@@ -3,6 +3,7 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do
   alias Lexical.Document
   alias Lexical.Document.Position
   alias Lexical.Document.Range
+  alias Lexical.RemoteControl.Build
   alias __MODULE__

   @spec prepare(Analysis.t(), Position.t()) ::
@@ -14,9 +15,11 @@ defmodule Lexical.RemoteControl.CodeMod.Rename do
   @spec rename(Analysis.t(), Position.t(), String.t()) ::
           {:ok, [Document.Changes.t()]} | {:error, term()}
   def rename(%Analysis{} = analysis, %Position{} = position, new_name) do
-    with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do
-      rename_module = @rename_mapping[renamable]
-      {:ok, rename_module.rename(range, new_name, entity)}
-    end
+    Build.with_lock(fn ->
+      with {:ok, {renamable, entity}, range} <- Rename.Prepare.resolve(analysis, position) do
+        rename_module = @rename_mapping[renamable]
+        {:ok, rename_module.rename(range, new_name, entity)}
+      end
+    end)
   end
 end
scottming commented 3 months ago

@scohen Given that DidSave is an operation after rename, so I think locking is almost unfeasible, and client operations are almost instantaneous. Therefore, in the commit: 26b9fb902247e37c2ea8b2dd3e2955b5e63dd957 of this PR #651 , I tried the method of temporarily stopping compilation for one second. This approach works very well on my computer.

If you have a better idea, I'd be glad to hear it.

scohen commented 1 month ago

This should pause until we get the proxy done.

scohen commented 6 days ago

I got a chance to test this out extensively today.

  1. I had to switch to vscode, emacs was just too unreliable. Pity. I think the problems belong to emacs and not lexical. Vscode is fast and accurate.
  2. It seems to rename modules that are aliased incorrectly, going through and first replacing the alias with the new module name, but then going through each alias and expanding it. so if I alias Foo.Bar.Utils and later in the module call Utils.blah() and rename Foo.Bar.Utils to Quux.Utils the alias is correctly expanded to alias Quux.Utils but the call is also expanded to Quux.Utils.blah().
  3. I was renaming modules in test/support. Modules had no previous structure, and I was adding Company.Testing.OldName. It placed the results in test/company/OldName.ex, i would have preferred to keep them in test/support. I think this is due to the path detection not recognizing test/support as a root.
  4. To ensure the indexer was up to date, i had to reindex after each rename. It would be better to wait for the rename to end, then reindex the files that have changed.
  5. It doesn't seem to handle renaming of modules in multi-module aliases. For example given alias Company.{A, B} and you alias A to Company.Something.A the alias won't be affected. It might be because the alias wash Company.{A, B} and the rename was to CompanyWeb.Testing.A
  6. Tough: Renaming a module often will mean it needs to be reordered in its context. It would be cool if it's in an alias / import / require / use group if it could be alphabetized.

It still saved me hours of tedious, error-prone work.

scottming commented 6 days ago

It seems to rename modules that are aliased incorrectly, going through and first replacing the alias with the new module name, but then going through each alias and expanding it. so if I alias Foo.Bar.Utils and later in the module call Utils.blah() and rename Foo.Bar.Utils to Quux.Utils the alias is correctly expanded to alias Quux.Utils but the call is also expanded to Quux.Utils.blah().

This issue you mentioned do exist and are indeed difficult to handle.

To ensure the indexer was up to date, i had to reindex after each rename. It would be better to wait for the rename to end, then reindex the files that have changed.

This is what we're currently doing; you don't need to reindex every time before renaming."