Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support for Renaming module #636

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add Support for Renaming module #636

wants to merge 13 commits into from

Conversation

scottming
Copy link
Collaborator

@scottming scottming commented Mar 3, 2024

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.

@scottming scottming force-pushed the rename-module-v2 branch 3 times, most recently from 1ce421a to 28bf64e Compare March 3, 2024 07:45
Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to talk more about the ergonomics of this PR and whether and how we do file renames. They can be tricky.

@scottming scottming changed the base branch from prefix-search-api to main March 12, 2024 12:06
@scottming scottming force-pushed the rename-module-v2 branch 3 times, most recently from 2c6f7bb to af600cc Compare March 16, 2024 02:24
@scottming scottming marked this pull request as draft March 16, 2024 08:04
@scottming scottming force-pushed the rename-module-v2 branch 7 times, most recently from c5f8027 to c5d4b46 Compare March 17, 2024 06:02
@scottming scottming marked this pull request as ready for review March 17, 2024 07:17
@scottming scottming requested a review from scohen March 17, 2024 07:21
@scottming scottming force-pushed the rename-module-v2 branch 3 times, most recently from e86d49e to dc10439 Compare March 20, 2024 13:03
@scohen
Copy link
Collaborator

scohen commented Mar 20, 2024

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
Copy link
Collaborator

scohen commented Mar 25, 2024

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
Copy link
Collaborator Author

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.

@scottming scottming force-pushed the rename-module-v2 branch 5 times, most recently from 96d3ffb to d72a46f Compare June 15, 2024 08:21
@scohen
Copy link
Collaborator

scohen commented Jun 28, 2024

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
Copy link
Collaborator Author

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."

@scohen
Copy link
Collaborator

scohen commented Jul 8, 2024

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

Can you elaborate? After the rename, we should have knowledge of what the aliases at a given line are. You should look for the module you want to replace in the aliases at the line, and if it's present, use the alias, if not, use the full module.

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

Then it doesn't seem to be work, the index was most definitely out of date.

@scottming scottming force-pushed the rename-module-v2 branch 2 times, most recently from 2910c54 to 8ad087d Compare July 13, 2024 02:42
@scottming
Copy link
Collaborator Author

scottming commented Jul 14, 2024

Can you elaborate? After the rename, we should have knowledge of what the aliases at a given line are. You should look for the module you want to replace in the aliases at the line, and if it's present, use the alias, if not, use the full module.

@scohen The reason I say this issue is difficult to handle is because renaming encompasses many scenarios, such as:

  1. Renaming the last module: Foo.Bar -> Foo.Renamed. This is also the one I use most frequently.
  2. Adding some modules in the middle: Foo.Bar -> Foo.Baz.Bar. This is the situation you mentioned that is prone to errors, mainly regarding references in aliased places.
  3. Removing some modules in the middle: Foo.Baz.Bar -> Foo.Bar.
  4. Replacing most of the modules in the suffix: Foo.Baz.Bar -> Foo.Renamed.
  5. Other possible scenarios.

For the second scenario, if there is an alias Foo.Bar in a file, then indeed, references to Bar in other places will become Baz.Bar. This is unnecessary, but the problem is that I don't know of a good way to accurately distinguish the second renaming method from the others. Given that such errors will be discovered after compilation, if there isn't a good way to identify them, I don't want to add more complexity.

To thoroughly resolve this type of issue, what we really need is to apply different modification strategies to lines like alias Foo.Baz.Bar and Bar.call(...).

EDIT: My latest commit: 2898e78 should help with the issue you mentioned.

@scottming scottming force-pushed the rename-module-v2 branch 4 times, most recently from e4bb866 to 2898e78 Compare July 17, 2024 02:07
scottming and others added 13 commits September 7, 2024 07:17
Enhanced the application by integrating rename functionality, increasing its editing capabilities. This includes supporting the preparation and execution of rename operations in various modules such as the lexical AST module, protocol request and response modules, remote control API, and server provider handling. Key additions entail aliasing for easier reference, handling new rename-related requests and responses, and implementing server state updates to acknowledge rename capabilities. The changes underscore our dedication to improving the tool's utility and ensuring a more dynamic and responsive user experience in code manipulation scenarios.

Filter out the `:struct` type references

Add some handler tests for `rename`

Add doc for `local_module_name`

Move `rename` from `code_intelligence` to `code_mod`

Move the prepare logic to a individual module

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.

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

Surround the whole module when renaming happens

Remove logic related to the rename function.

Apply some code review suggestions

Fix a bug when expanding the module

Fix a merge error

Apply a format module usage suggestion

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

Co-authored-by: Steve Cohen <scohen@scohen.org>

Apply the `defdelegate` suggestion

Fix a bug when the descendant containing the old suffix
* 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 <scohen@scohen.org>

* 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 <scohen@scohen.org>

Add `WorkDoneProgress` for the rename expected_operation

Apply suggestions from code review

Co-authored-by: Steve Cohen <scohen@scohen.org>

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

Co-authored-by: Steve Cohen <scohen@scohen.org>

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

Co-authored-by: Zach Allaun <zach.allaun@gmail.com>

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
Co-authored-by: Steve Cohen <scohen@scohen.org>
When the entity at the cursor supports renaming but is at the wrong
location, return nil. When the entity is of an unsupported type, return
an appropriate error.
…aming non-alias reference modules.

This can fix some errors that occur when renaming without changing the local name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants