impl moving items to new files#3181
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds a new refactor.move code action to move a selected top-level Python symbol into a newly created sibling file, while updating both the source module and downstream from <source> import <symbol> consumer imports.
Changes:
- Introduces a non-wasm LSP code action that creates
<symbol>.py, moves the symbol, and returns aWorkspaceEditwithCreateFile+ edits. - Refactors shared move-module-member logic to compute a reusable “move context” and to rewrite direct consumer imports via a transitive rdep walk.
- Adds an LSP interaction test fixture and test case for “move symbol to new file”.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/lsp/non_wasm/move_symbol_new_file.rs | Implements the new “Move <symbol> to new file” code action and WorkspaceEdit construction (including file creation). |
| pyrefly/lib/lsp/non_wasm/server.rs | Wires the new code action into the server’s code-action handler and telemetry. |
| pyrefly/lib/lsp/non_wasm/mod.rs | Exposes the new non-wasm module. |
| pyrefly/lib/state/lsp/quick_fixes/move_module.rs | Refactors move-member logic into a context object and adds consumer-import rewrite edits via transitive rdeps. |
| pyrefly/lib/state/lsp.rs | Re-exports the move context and adds Transaction helpers for the new shared move-symbol routines. |
| pyrefly/lib/test/lsp/lsp_interaction/move_symbol_new_file.rs | Adds an integration test validating create+edit operations for the new code action. |
| pyrefly/lib/test/lsp/lsp_interaction/mod.rs | Registers the new test module. |
| pyrefly/lib/test/lsp/lsp_interaction/test_files/move_symbol_to_new_file/source.py | New test fixture source module. |
| pyrefly/lib/test/lsp/lsp_interaction/test_files/move_symbol_to_new_file/consumer.py | New test fixture consumer module importing the moved symbol. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut moved_aliases = Vec::new(); | ||
| let mut remaining_aliases = Vec::new(); | ||
| for alias in &import_from.names { | ||
| let rendered = render_import_alias(alias); | ||
| if alias.name.id.as_str() == member_name { | ||
| moved_aliases.push(rendered); | ||
| } else { | ||
| remaining_aliases.push(rendered); | ||
| } | ||
| } | ||
| if moved_aliases.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| let Some((indent, _)) = | ||
| line_indent_and_start(module_info.contents(), import_from.range().start()) | ||
| else { | ||
| continue; | ||
| }; | ||
|
|
||
| let mut replacement = String::new(); | ||
| if !remaining_aliases.is_empty() { | ||
| replacement.push_str(&format!( | ||
| "{indent}from {} import {}\n", | ||
| module.id.as_str(), | ||
| remaining_aliases.join(", ") | ||
| )); | ||
| } | ||
| if !has_target_import { | ||
| replacement.push_str(&format!( | ||
| "{indent}from {new_module_text} import {}\n", | ||
| moved_aliases.join(", ") | ||
| )); | ||
| } |
There was a problem hiding this comment.
This consumer-import rewrite logic has multiple important branches (splitting a multi-name import into remaining vs moved aliases, preserving asname, and skipping insertion when a target import already exists). The new LSP interaction test covers only the simplest from source import test case; adding targeted tests for aliasing (from source import test as t) and multi-imports (from source import a, test) would help prevent regressions in these branches.
| let has_target_import = | ||
| has_existing_import(ast.as_ref(), target_handle.module(), member_name); | ||
| let (_, _, new_module_text) = insert_import_edit( | ||
| ast.as_ref(), | ||
| transaction.config_finder(), | ||
| rdep_handle.dupe(), | ||
| target_handle.dupe(), | ||
| member_name, | ||
| import_format, | ||
| ); |
There was a problem hiding this comment.
In build_module_member_consumer_import_updates, insert_import_edit is computed for every transitive rdep before confirming the file actually contains a from <old_module> import <member> that needs rewriting. On large workspaces this can add noticeable overhead during code action computation. Consider first scanning the AST for relevant ImportFrom statements (or computing new_module_text lazily only when moved_aliases is non-empty and !has_target_import).
|
|
||
| let mut edits = Vec::new(); | ||
| for rdep_handle in unique_rdeps { | ||
| if rdep_handle.path() == source_module_info.path() { |
There was a problem hiding this comment.
The self-skip check uses rdep_handle.path() == source_module_info.path(), but ModulePath equality distinguishes Memory vs FileSystem. When the source file is open (in-memory) and you seed the rdep walk with a filesystem handle, this condition won't filter out the source module handle and can lead to redundant work (and potentially edits computed from the wrong backing contents). Consider comparing underlying paths (e.g., as_path()) instead of full ModulePathDetails equality.
| if rdep_handle.path() == source_module_info.path() { | |
| if rdep_handle.path().as_path() == source_module_info.path().as_path() { |
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
looks like tests are failing on windows
asukaminato0721
force-pushed
the
move-symbol
branch
from
48ecfb1 to
6156d47
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
part of #2207
cc @rsheftel
Implemented the new refactor.move code action for “move symbol to new file”.
The new server-side action creates
<symbol>.pybeside the source file, moves the selected top-level symbol there, updates the source file, and rewrites direct consumer imports like from source import test to from test import test.pushed the consumer-import rewrite into the shared move-symbol logic, fixing the reverse-dependency walk to use filesystem handles so workspace updates actually see other files.
Test Plan
add test