Skip to content

Conversation

@scottming
Copy link

@scottming scottming commented Jan 4, 2026

Context

While AI-assisted renaming is convenient, it's still slower than a native Language Server implementation. This PR ports the module rename feature from Lexical PR #636, which I've been using reliably for almost two years. Given that the Lexical repository is now archived, I wanted to bring this practical and well-tested feature to Expert.

Changes

Core Implementation (engine)

  • Add Engine.CodeMod.Rename module with submodules: Prepare, Module, Entry, File, and Diff
  • Add Engine.Commands.Rename GenServer for tracking rename progress and triggering reindex
  • Add Engine.Commands.RenameSupervisor for managing rename command lifecycle

LSP Integration (expert)

  • Add PrepareRename and Rename handlers for LSP protocol
  • Extend Expert.EngineApi with prepare_rename/3 and rename/5 APIs
  • Register rename_provider with prepare_provider: true in server capabilities

Infrastructure (forge)

  • Add RenameFile struct to Forge.Document.Changes for file rename operations
  • Add file_changed and file_saved message types to Forge.EngineApi.Messages

Key Differences from Lexical

  • Uses :erpc.call via Expert.EngineApi.call/4 instead of RemoteControl.call
  • Added reindexing mechanism after rename to keep search index consistent (see "Why These Changes")
  • Namespace mapping: Lexical.RemoteControl.*Engine.*, Lexical.Server.*Expert.*

Why These Changes

  • Progress tracking & reindex (59953157): Without reindexing after rename, the search index retains old module names. This causes subsequent renames to fail - e.g., renaming AccountsAccounts2 works, but renaming back fails because the index doesn't reflect the new names.

  • Temporary document opening (f08fd155): After a file rename, the new URI may not exist in Document.Store, causing "not_open" errors during reindex. Added ensure_open/1 to temporarily open files from disk when needed.

  • Expanded test coverage (a38652ef): Added comprehensive tests covering references, descendants, structs, edge cases, and file renaming with realistic module names.

Extra

A demo video is attached showing the smooth rename workflow - renaming a module and then renaming it back. Tested successfully in both VSCode and Neovim.

CleanShot.2026-01-04.at.15.43.19.mp4

@scottming scottming force-pushed the rename-module branch 2 times, most recently from dba3fa9 to 8d72473 Compare January 11, 2026 02:11
Copy link
Collaborator

@doorgan doorgan left a comment

Choose a reason for hiding this comment

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

Some notes(I'm testing on a brand new mix phx.new rename_test project:

  • If I am on module RenameTestWeb.Router and I rename it to ExpertRenameTestWeb then

    • No other module prefixed with RenameTestWeb is renamed
    • The use RenameTestWeb, :router isn't renamed either
  • If I create lib/rename_test/nested/foo.ex defining RenameTest.Nested.Foo and I rename RenameTest in lib/rename_test.ex, then the child modules are renamed and the files are renamed too, but I end up with an empty lib/rename_test/nested folder

  • When I try to rename a function(I know this is not supported by this PR, but still) I get this error:

2026-01-14 16:34:27.690 [info] [Error - 4:34:27 PM] ** (UndefinedFunctionError) function XPGenLSP.ErrorResponse.fetch/2 is undefined (XPGenLSP.ErrorResponse does not implement the Access behaviour

You can use the "struct.field" syntax to access struct fields. You can also use Access.key!/1 to access struct fields dynamically inside get_in/put_in/update_in)
    (xp_gen_lsp 0.11.2) XPGenLSP.ErrorResponse.fetch(%XPGenLSP.ErrorResponse{message: "Failed to handle textDocument/prepareRename, {:error, :request_failed, \"Renaming :call is not supported for now\"}", code: -32603, data: nil}, :placeholder)
    (elixir 1.17.2) lib/access.ex:322: Access.get/3
    (xp_schematic 0.2.1) lib/schematic.ex:608: anonymous fn/5 in XPSchematic.map/1
    (elixir 1.17.2) lib/enum.ex:2531: Enum."-reduce/3-lists^foldl/2-0-"/3
    (xp_schematic 0.2.1) lib/schematic.ex:583: anonymous fn/3 in XPSchematic.map/1
    (xp_schematic 0.2.1) lib/schematic.ex:1001: anonymous fn/4 in XPSchematic.telemetry_wrap/3
    (xp_telemetry 1.3.0) /Users/dorgan/dev/expert/apps/expert/deps/telemetry/src/telemetry.erl:324: :xp_telemetry.span/3
    (xp_schematic 0.2.1) lib/schematic.ex:947: anonymous fn/3 in XPSchematic.oneof/1

Also we should have some progress reported, I'm testing on VSCode(it's easier for me to trigger these actions from this editor) and I don't see progress reported there, neither in the status bar nor in the Expert logs there

@scottming
Copy link
Author

Hi, @doorgan Thanks for the detailed feedback! Let me address each point:

1. Renaming RenameTestWeb.Router to ExpertRenameTestWeb

This is actually the expected behavior by design. The current rename implementation renames:

  • The exact module and all its references
  • Descendant modules (e.g., MyApp.Users.Query when renaming MyApp.Users)

When you rename RenameTestWeb.Router, other modules like RenameTestWeb.PageController are not descendants of Router, so they won't be renamed. Similarly, use RenameTestWeb, :router references the RenameTestWeb module, not RenameTestWeb.Router.

If you want to rename the RenameTestWeb prefix across all modules, you should trigger the rename on the defmodule RenameTestWeb definition itself.

2. Empty folder left after renaming

Good catch! This is a known limitation. The file rename operation is actually executed by the editor (VSCode/Neovim) via LSP's RenameFile - the editor handles creating parent directories automatically, but it doesn't clean up empty source directories afterward.

To fix this, we'd need to detect which directories become empty after the rename and send additional DeleteFile operations for them. Given that the rename feature is already quite complex (handling module references, descendants, file path conventions, Phoenix special folders, progress tracking, etc.), I'll note this as a potential future improvement, but it's lower priority for now.

3. Error when trying to rename a function

You're right - even though function renaming isn't supported yet, it should return a friendly error message like "Renaming :function is not supported for now" rather than throwing an UndefinedFunctionError. I'll look into this over the weekend since this is a higher priority issue.

4. Progress reporting in VSCode

Thanks for flagging this. We do have progress reporting implemented with VSCode-specific handling, but it seems something might be off. I'll investigate this as well.

@scottming scottming force-pushed the rename-module branch 2 times, most recently from 9fa6111 to 92f6d91 Compare February 7, 2026 03:59
- 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
- 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
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.
…name

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.
…uring 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.
…rename progress

begin_percent was a leftover from the old Lexical codebase that broadcast
percent_progress messages via Engine.broadcast, but no listener on the Expert
side consumed them, so rename progress never reached the LSP client.

Replacing it with Progress.begin/report/complete uses the same erpc channel
that the rest of the progress infrastructure (e.g. compilation, indexing) relies
on, routing notifications through Expert.Progress to the editor. Also removed
begin_percent and its associated types since it has no remaining callers.
The original design returned {:error, "Renaming :call is not supported for
now"} for unsupported entities (e.g. functions), which the handler converts
to {:error, :request_failed, message}. This was intended to surface a
user-friendly message through the LSP error response mechanism — per the
spec, prepareRename can return an error response with a custom message that
the editor displays to the user.

However, gen_lsp 0.11.x has a serialization bug that makes this crash:
Schematic.oneof in the result() schema tries each branch sequentially, and
when the first non-ErrorResponse branch (e.g. PrepareRenameResult.schema())
attempts Access.get on the ErrorResponse struct, it raises
UndefinedFunctionError instead of returning {:error, ...}. Since oneof
doesn't rescue exceptions, the error propagates before the ErrorResponse
branch gets a chance to match.

This affects all request types, not just rename — any handler returning
ErrorResponse through the generic handle_request path will crash.

Fall back to returning {:ok, nil} for now, which tells the editor "this
element can't be renamed". The entity type is still logged via Logger.info
in handle_unsupported_entity/2 for debugging. We can revisit this once
gen_lsp fixes the ErrorResponse serialization in oneof.
@scottming
Copy link
Author

Hi, @doorgan — the third and fourth issues you mentioned should both be fixed now. Please take a look at the two latest commits; they include detailed explanations of the problems and the solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants