Skip to content

fix Renaming a kwarg does not work across files. #1583#2484

Open
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:1583
Open

fix Renaming a kwarg does not work across files. #1583#2484
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:1583

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

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

@meta-cla meta-cla bot added the cla signed label Feb 21, 2026
@asukaminato0721 asukaminato0721 marked this pull request as ready for review February 21, 2026 19:08
Copilot AI review requested due to automatic review settings February 21, 2026 19:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.py and uses.py before 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 not did_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.

Comment on lines 2781 to +2784
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());
}
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
@github-actions

This comment has been minimized.

@jvansch1 jvansch1 self-assigned this Feb 25, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the stale label Mar 23, 2026
@github-actions
Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@github-actions
Copy link
Copy Markdown

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!

@github-actions github-actions bot added the stale label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Renaming a kwarg does not work across files.

3 participants