fix Renaming a kwarg does not work across files. #1583#2484
fix Renaming a kwarg does not work across files. #1583#2484asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes #1583 by extending workspace rename/reference discovery so that keyword-argument usages are included even when the parameter definition is in a different module, and adds an LSP interaction test/fixtures covering the cross-file kwarg rename scenario.
Changes:
- Update cross-module reference collection to include kwarg references tied back to a parameter definition in another file.
- Add a new rename interaction test plus a minimal multi-file fixture (
defs.py/uses.py) for kwarg renames across files.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pyrefly/lib/state/lsp.rs |
Extends external-definition reference collection to also find/refine kwarg references against the definition module AST. |
pyrefly/lib/test/lsp/lsp_interaction/rename.rs |
Adds an integration-style LSP rename test for cross-file kwarg renames. |
pyrefly/lib/test/lsp/lsp_interaction/test_files/rename_kwargs_across_files/defs.py |
Defines a function with a parameter intended to be renamed. |
pyrefly/lib/test/lsp/lsp_interaction/test_files/rename_kwargs_across_files/uses.py |
Calls the function using keyword arguments to validate cross-file rename edits. |
pyrefly/lib/test/lsp/lsp_interaction/test_files/rename_kwargs_across_files/pyrefly.toml |
Fixture config to make the local directory importable for the test scenario. |
Comments suppressed due to low confidence (1)
pyrefly/lib/test/lsp/lsp_interaction/rename.rs:210
- This test opens both
defs.pyanduses.pybefore issuing the rename. If the intended behavior is to support workspace-wide rename for kwarg references in files that aren’t currently opened (which is how other rename tests validate cross-file edits), consider adding a variant that does notdid_open("uses.py")to ensure the new kwarg reference logic works when the referencing module’s AST isn’t already resident.
interaction.client.did_open("defs.py");
interaction.client.did_open("uses.py");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let keyword_args = self.collect_local_keyword_arguments_by_name(handle, expected_name); | ||
| let mut references = Vec::new(); | ||
| if keyword_args.is_empty() { | ||
| return Some(Vec::new()); | ||
| } |
There was a problem hiding this comment.
keyword_argument_references_from_parameter_definition ultimately depends on collect_local_keyword_arguments_by_name(handle, ...), which returns an empty list when get_ast(handle) is unavailable. In this codebase, ASTs can be evicted after later pipeline steps (see ComputeGuard::evict_ast in state/module.rs), so for unopened modules (or modules whose AST was evicted) cross-file kwarg references will still be missed and rename won’t update those files. Consider adding the same fallback used for definition_ast: if get_ast(handle) is None, re-parse the referencing module from get_module_info(handle)?.contents() and use that AST to collect keyword args, so rename works even when the file wasn’t opened.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
This pull request has been automatically marked as stale because it has not had recent activity for more than 2 weeks. If you are still working on this this pull request, please add a comment or push new commits to keep it active. Otherwise, please unassign yourself and allow someone else to take over. Thank you for your contributions! |
Summary
Fixes #1583
Updated cross-module rename logic so keyword-argument references are included when the parameter definition lives in a different file. This now resolves keyword args by name, then refines against the definition module’s AST to match the exact parameter.
Test Plan
Added a cross-file rename test and fixtures to cover kwarg renames across modules