impl move refactor #364 #2101
impl move refactor #364 #2101
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements refactor.move actions for Python code, adding two main capabilities: (1) moving top-level module members to sibling modules with automatic import insertion, and (2) lifting local functions and methods to module scope. For methods, the implementation creates class-body shims (e.g., foo = foo, bar = staticmethod(bar)) to maintain existing call sites.
Changes:
- Added
move_module.rswith logic for moving module members and making local functions/methods top-level - Integrated new refactoring actions into LSP code action pipeline
- Added comprehensive tests for module member moves, local function lifting, and method/staticmethod lifting
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/state/lsp/quick_fixes/move_module.rs | New module implementing move refactoring logic with helper functions for finding targets, building edits, and handling reindentation |
| pyrefly/lib/state/lsp/quick_fixes/mod.rs | Adds module declaration for new move_module module |
| pyrefly/lib/state/lsp.rs | Exposes public Transaction methods for move refactoring actions |
| pyrefly/lib/lsp/non_wasm/server.rs | Integrates move refactoring actions into LSP code action request handling |
| pyrefly/lib/test/lsp/code_actions.rs | Adds test helper functions and three test cases covering module member moves and local function/method lifting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn method_wrapper_kind(function_def: &StmtFunctionDef) -> Option<MethodWrapper> { | ||
| let mut kind = MethodWrapper::None; | ||
| for decorator in &function_def.decorator_list { | ||
| if decorator_matches_name(&decorator.expression, "staticmethod") { | ||
| if kind != MethodWrapper::None { | ||
| return None; | ||
| } | ||
| kind = MethodWrapper::Staticmethod; | ||
| } else if decorator_matches_name(&decorator.expression, "classmethod") { | ||
| if kind != MethodWrapper::None { | ||
| return None; | ||
| } | ||
| kind = MethodWrapper::Classmethod; | ||
| } else { | ||
| return None; | ||
| } | ||
| } | ||
| Some(kind) | ||
| } |
There was a problem hiding this comment.
The method_wrapper_kind function returns None if any decorator other than staticmethod or classmethod is found on the method. This means methods with other decorators like property, abstractmethod, or custom decorators cannot be made top-level, which may be overly restrictive. While this behavior may be intentional for safety, it's worth considering whether methods with additional decorators should still be allowed to be made top-level with those decorators preserved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
asukaminato0721
force-pushed
the
364-5
branch
from
9a8b808 to
2f3c855
Compare
|
Diff from mypy_primer, showing the effect of this PR on open source code: pandas (https://github.com/pandas-dev/pandas)
- ::error file=pandas/core/indexing.py,line=1470,col=9,endLine=1470,endColumn=28,title=Pyrefly bad-override::Class member `_LocIndexer._convert_to_indexer` overrides parent class `_LocationIndexer` in an inconsistent manner%0A `_LocIndexer._convert_to_indexer` has type `BoundMethod[_LocIndexer, (self: _LocIndexer, key: Unknown, axis: int) -> int | ndarray[tuple[int], dtype[Any]] | ndarray[tuple[Any, ...], dtype[signedinteger[_NBitIntP]]] | ndarray[tuple[Any, ...], dtype[Any]] | slice[int, int, Any] | slice[Any, Any, Any] | dict[str, int | integer[Any]] | dict[str, tuple[Unknown, ...]] | dict[str, Unknown] | Unknown]`, which is not assignable to `BoundMethod[_LocIndexer, (self: _LocIndexer, key: Unknown, axis: int) -> Never]`, the type of `_LocationIndexer._convert_to_indexer`
+ ::error file=pandas/core/indexing.py,line=1470,col=9,endLine=1470,endColumn=28,title=Pyrefly bad-override::Class member `_LocIndexer._convert_to_indexer` overrides parent class `_LocationIndexer` in an inconsistent manner%0A `_LocIndexer._convert_to_indexer` has type `BoundMethod[_LocIndexer, (self: _LocIndexer, key: Unknown, axis: int) -> ndarray[tuple[Any, ...], dtype[signedinteger[_NBitIntP]]] | ndarray[tuple[Any, ...], dtype[Any]] | dict[str, int | integer[Any]] | dict[str, tuple[Unknown, ...]] | dict[str, Unknown] | Unknown]`, which is not assignable to `BoundMethod[_LocIndexer, (self: _LocIndexer, key: Unknown, axis: int) -> Never]`, the type of `_LocationIndexer._convert_to_indexer`
|
grievejia
left a comment
grievejia
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
4 participants
Summary
Added new refactor.move actions for moving module members and for making local functions/methods top‑level, then wired them into the LSP code action pipeline. The new logic moves a top‑level definition into a sibling module and inserts an import back into the source module to keep references working.
For local functions/methods, it lifts the function to module scope and replaces methods with a class‑body shim (foo = foo / staticmethod / classmethod) so existing call sites still work. New tests cover module member move, local function lift, and method/staticmethod lift.
Fixes part of #364
https://www.jetbrains.com/help/pycharm/move-refactorings.html
Test Plan
Added tests in pyrefly/lib/test/lsp/code_actions.rs